Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Commit

Permalink
perf: memoize notification bar state selectors
Browse files Browse the repository at this point in the history
  • Loading branch information
petemill committed Nov 27, 2017
1 parent 2831ee5 commit 3114644
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 48 deletions.
69 changes: 52 additions & 17 deletions app/common/state/notificationBarState.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,38 +2,73 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

const assert = require('assert')
const Immutable = require('immutable')
const {makeImmutable, isMap} = require('./immutableUtil')
const { createSelector } = require('reselect')
const frameStateUtil = require('../../../js/state/frameStateUtil')
const { getOrigin } = require('../../../js/lib/urlutil')
const { generateWindowStateSelector } = require('./windowState')

const api = {
validateState: function (state) {
state = makeImmutable(state)
assert.ok(isMap(state), 'state must be an Immutable.Map')
return state
}
}
// the portion of the appStore state that concerns notifications
const selectNotificationState = state => state.get('notifications', Immutable.List())

const notificationBarState = {
/**
* Gets an immutable list of notifications
* @param {Map} appState - The app state object
* @return {List} - immutable list of notifications
*/
getNotifications: (state) => {
state = api.validateState(state)
return state.get('notifications', Immutable.List())
},
getNotifications: selectNotificationState,

/**
* Gets an immutable list of ledger notifications
* @param {Map} appState - The app state object
* @return {List} - immutable list of ledger notifications
*/
getLedgerNotifications: (state) => {
const notifications = notificationBarState.getNotifications(state)
return notifications.filter(item => item.get('from') === 'ledger')
}
getLedgerNotifications: createSelector(
selectNotificationState,
notifications => notifications.filter(item => item.get('from') === 'ledger')
),

/**
* Get notifications which should be considered 'active',
* that is - notifications for the active frame origin,
* and all other notifications that are not from the ledger.
* Limited to the last 3 notifications.
*/
getActiveNotifications: createSelector(
// select active frame origin
// since the active frame data changes extremely frequently,
// but the origin does not, do not perform the notifications
// filter on every change of every frame property, just the origin
createSelector(
// generateWindowStateSelector is required
// to convert to state.currentWindow since some selectors require appState
// and some require windowState
generateWindowStateSelector(
frameStateUtil.getActiveFrame
),
// will only be re-computed if the active frame state changes (frequently)
activeFrame => getOrigin((activeFrame || Immutable.Map()).get('location'))
),
// select all notifications
selectNotificationState,
// if either active frame origin, or all notifications change,
// then reevaluate what 'active notifications' are
(activeFrameOrigin, notifications) => {
console.log('re-eval active notifications')
return notifications
.filter((item) => {
const notificationFrameOrigin = item.get('frameOrigin')
return notificationFrameOrigin
? activeFrameOrigin === notificationFrameOrigin
// TODO: this should filter all cases
// for notifications that are not per-tab
// and not only ledger
: item.get('from') !== 'ledger'
})
.takeLast(3)
}
)
}

module.exports = notificationBarState
15 changes: 14 additions & 1 deletion app/common/state/windowState.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

const { makeImmutable, isMap, isList } = require('./immutableUtil')
const Immutable = require('immutable')
const assert = require('assert')
const { createSelector } = require('reselect')
const { makeImmutable, isMap, isList } = require('./immutableUtil')

// TODO(bridiver) - make these generic validation functions
const validateId = function (propName, id) {
Expand Down Expand Up @@ -183,6 +184,18 @@ const api = {
!windowState.getIn(['ui', 'noScriptInfo', 'isVisible']) &&
frame && !frame.getIn(['security', 'loginRequiredDetail']) &&
!windowState.getIn(['ui', 'menubar', 'selectedIndex'])
},

/**
* Allows a selector to be created from appStore state, that can call
* a child selector which expects windowStore state. Must be called from renderer
* component, which stores windowState on appState.currentWindow
*/
generateWindowStateSelector: function generateWindowStateSelector (resultFunc) {
return createSelector(
state => state.get('currentWindow'),
resultFunc
)
}
}

Expand Down
9 changes: 1 addition & 8 deletions app/renderer/components/main/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,6 @@ class Main extends React.Component {
const activeTabId = activeFrame.get('tabId', tabState.TAB_ID_NONE)
const nonPinnedFrames = frameStateUtil.getNonPinnedFrames(currentWindow)
const tabsPerPage = Number(getSetting(settings.TABS_PER_PAGE))
const activeOrigin = !activeFrame.isEmpty() ? urlUtil.getOrigin(activeFrame.get('location')) : null
const widevinePanelDetail = currentWindow.get('widevinePanelDetail', Immutable.Map())
const loginRequiredDetails = basicAuthState.getLoginRequiredDetail(state, activeTabId)
const focused = isFocused(state)
Expand Down Expand Up @@ -568,8 +567,6 @@ class Main extends React.Component {
props.shouldAllowWindowDrag = windowState.shouldAllowWindowDrag(state, currentWindow, activeFrame, focused)
props.isSinglePage = nonPinnedFrames.size <= tabsPerPage
props.showTabPages = nonPinnedFrames.size > tabsPerPage
props.showNotificationBar = activeOrigin && state.get('notifications').filter((item) =>
item.get('frameOrigin') ? activeOrigin === item.get('frameOrigin') : true).size > 0
props.showFindBar = activeFrame.get('findbarShown') && !activeFrame.get('isFullScreen')
props.sortedFrames = frameStateUtil.getSortedFrameKeys(currentWindow)
props.showDownloadBar = currentWindow.getIn(['ui', 'downloadsToolbar', 'isVisible']) &&
Expand Down Expand Up @@ -722,11 +719,7 @@ class Main extends React.Component {
</div>
}
<TabsToolbar key='tab-bar' />
{
this.props.showNotificationBar
? <NotificationBar />
: null
}
<NotificationBar />
{
this.props.showFindBar
? <FindBar />
Expand Down
25 changes: 3 additions & 22 deletions app/renderer/components/main/notificationBar.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,12 @@

const React = require('react')
const {StyleSheet, css} = require('aphrodite/no-important')
const Immutable = require('immutable')

// Components
const ReduxComponent = require('../reduxComponent')
const NotificationItem = require('./notificationItem')

// Utils
const {getOrigin} = require('../../../../js/lib/urlutil')
const frameStateUtil = require('../../../../js/state/frameStateUtil')
const notificationBarState = require('../../../common/state/notificationBarState')

// Styles
Expand All @@ -21,29 +18,14 @@ const globalStyles = require('../styles/global')

class NotificationBar extends React.Component {
mergeProps (state, ownProps) {
const currentWindow = state.get('currentWindow')
const activeFrame = frameStateUtil.getActiveFrame(currentWindow) || Immutable.Map()
const activeOrigin = getOrigin(activeFrame.get('location'))
const notifications = notificationBarState.getNotifications(state)

const props = {}
props.activeNotifications = notifications
.filter((item) => {
return item.get('frameOrigin')
? activeOrigin === item.get('frameOrigin')
// TODO: this should filter all cases
// for notifications that are not per-tab
// and not only ledger
: item.get('from') !== 'ledger'
})
.takeLast(3)
props.showNotifications = !props.activeNotifications.isEmpty()
props.activeNotifications = notificationBarState.getActiveNotifications(state)
return props
}

render () {
// Avoid rendering an empty notification wrapper
if (!this.props.showNotifications) {
if (this.props.activeNotifications.isEmpty()) {
return null
}
return <div className={css(commonStyles.notificationBar)} data-test-id='notificationBar'>
Expand All @@ -64,13 +46,12 @@ class BraveNotificationBar extends React.Component {
// TODO: for now we cover only ledger notifications in this method
// this should cover all notifications that are global
props.notifications = notificationBarState.getLedgerNotifications(state)
props.showNotifications = !props.notifications.isEmpty()
return props
}

render () {
// Avoid rendering an empty notification wrapper
if (!this.props.showNotifications) {
if (this.props.notifications.isEmpty()) {
return null
}
return (
Expand Down

0 comments on commit 3114644

Please sign in to comment.