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

Commit

Permalink
Avoid serializing Function in array template
Browse files Browse the repository at this point in the history
fix #13421
fix #11576

Auditors: @bridiver

Test Plan:
1. Set `sessionSaveInterval` to `1` in js/constants/appConfig.js
2. Playing around with bookmark toolbar folder
3. Playing around with auotfill menu(fill with entry and clear)
4. Playing around with hamburger menu
5. Playing around with back/forward long pressed menu
6. There shouldn't be any exceptions about "function can be cloned"
  • Loading branch information
darkdh authored and bsclifton committed Mar 16, 2018
1 parent d3938a0 commit abbfedf
Show file tree
Hide file tree
Showing 6 changed files with 39 additions and 17 deletions.
12 changes: 8 additions & 4 deletions app/common/state/contextMenuState.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,38 +5,42 @@
const Immutable = require('immutable')
const assert = require('assert')
const { makeImmutable, isMap } = require('./immutableUtil')
const uuid = require('uuid')

const validateState = function (state) {
state = makeImmutable(state)
assert.ok(isMap(state), 'state must be an Immutable.Map')
return state
}

let contextMenuDetail = Immutable.Map()

const api = {
setContextMenu: (windowState, detail) => {
detail = makeImmutable(detail)
windowState = validateState(windowState)

if (!detail) {
if (windowState.getIn(['contextMenuDetail', 'type']) === 'hamburgerMenu') {
if (contextMenuDetail.get('type') === 'hamburgerMenu') {
windowState = windowState.set('hamburgerMenuWasOpen', true)
} else {
windowState = windowState.set('hamburgerMenuWasOpen', false)
}
contextMenuDetail = Immutable.Map()
windowState = windowState.delete('contextMenuDetail')
} else {
if (!(detail.get('type') === 'hamburgerMenu' && windowState.get('hamburgerMenuWasOpen'))) {
windowState = windowState.set('contextMenuDetail', detail)
contextMenuDetail = detail
windowState = windowState.set('contextMenuDetail', uuid())
}
windowState = windowState.set('hamburgerMenuWasOpen', false)
}

return windowState
},

getContextMenu: (windowState) => {
windowState = validateState(windowState)
return windowState.get('contextMenuDetail', Immutable.Map())
return contextMenuDetail
},

selectedIndex: (windowState) => {
Expand Down
5 changes: 4 additions & 1 deletion app/renderer/components/common/contextMenu/contextMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ const windowActions = require('../../../../../js/actions/windowActions')
// Constants
const keyCodes = require('../../../../common/constants/keyCodes')

// State
const contextMenuState = require('../../../../common/state/contextMenuState')

// Utils
const frameStateUtil = require('../../../../../js/state/frameStateUtil')
const {separatorMenuItem} = require('../../../../common/commonMenu')
Expand Down Expand Up @@ -218,7 +221,7 @@ class ContextMenu extends React.Component {
const currentWindow = state.get('currentWindow')
const activeFrame = frameStateUtil.getActiveFrame(currentWindow) || Immutable.Map()
const selectedIndex = currentWindow.getIn(['ui', 'contextMenu', 'selectedIndex'], null)
const contextMenuDetail = currentWindow.get('contextMenuDetail', Immutable.Map())
const contextMenuDetail = contextMenuState.getContextMenu(currentWindow)

const props = {}
props.lastZoomPercentage = activeFrame.get('lastZoomPercentage')
Expand Down
3 changes: 2 additions & 1 deletion app/renderer/components/frame/frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const windowStore = require('../../../../js/stores/windowStore')
const appStoreRenderer = require('../../../../js/stores/appStoreRenderer')

// State
const contextMenuState = require('../../../common/state/contextMenuState')
const siteSettings = require('../../../../js/state/siteSettings')
const siteSettingsState = require('../../../common/state/siteSettingsState')
const tabState = require('../../../common/state/tabState')
Expand Down Expand Up @@ -866,7 +867,7 @@ class Frame extends React.Component {
const allSiteSettings = siteSettingsState.getAllSiteSettings(state, isPrivate)
const frameSiteSettings = siteSettings.getSiteSettingsForURL(allSiteSettings, location) || Immutable.Map()

const contextMenu = currentWindow.get('contextMenuDetail')
const contextMenu = contextMenuState.getContextMenu(currentWindow)
const tab = tabId && tabId > -1 && tabState.getByTabId(state, tabId)

const props = {}
Expand Down
2 changes: 1 addition & 1 deletion docs/state.md
Original file line number Diff line number Diff line change
Expand Up @@ -677,7 +677,7 @@ WindowStore
},
cleanedOnShutdown: boolean, // whether app data was successfully cleared on shutdown
closedFrames: [], // holds the same type of frame objects as frames
contextMenuDetail: {
contextMenuDetail: { // currently using uuid hack to avoid serializing click function in template
bottom: number, // the bottom position of the context menu
left: number, // the left position of the context menu
maxHeight: number, // the maximum height of the context menu
Expand Down
9 changes: 8 additions & 1 deletion js/actions/windowActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -550,9 +550,16 @@ const windowActions = {
* @param {Object} detail - The context menu detail
*/
setContextMenuDetail: function (detail) {
// TODO(darkdh): This is a hack to prevent dispatch from serializing
// click function in template. `contextMenuDetail` is just a uuid to trigger
// state update for new menu
const Immutable = require('immutable')
const contextMenuState = require('../../app/common/state/contextMenuState')
let state = contextMenuState.setContextMenu(Immutable.Map(), detail)
const contextMenuDetail = state.get('contextMenuDetail')
dispatch({
actionType: windowConstants.WINDOW_SET_CONTEXT_MENU_DETAIL,
detail
contextMenuDetail
})
},

Expand Down
25 changes: 16 additions & 9 deletions js/stores/windowStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -480,21 +480,28 @@ const doAction = (action) => {
windowState = windowState.delete('bookmarkFolderDetail')
break
case windowConstants.WINDOW_AUTOFILL_SELECTION_CLICKED:
windowState = contextMenuState.setContextMenu(windowState)
ipc.send('autofill-selection-clicked', action.tabId, action.value, action.frontEndId, action.index)
windowState = windowState.delete('contextMenuDetail')
break
case windowConstants.WINDOW_AUTOFILL_POPUP_HIDDEN:
if (!action.detail &&
windowState.getIn(['contextMenuDetail', 'type']) === 'autofill' &&
windowState.getIn(['contextMenuDetail', 'tabId']) === action.tabId) {
windowState = windowState.delete('contextMenuDetail')
if (action.notify) {
ipc.send('autofill-popup-hidden', action.tabId)
{
const contextMenuDetail = contextMenuState.getContextMenu(windowState)
if (!action.detail &&
contextMenuDetail.get('type') === 'autofill' &&
contextMenuDetail.get('tabId') === action.tabId) {
windowState = contextMenuState.setContextMenu(windowState)
if (action.notify) {
ipc.send('autofill-popup-hidden', action.tabId)
}
}
break
}
break
case windowConstants.WINDOW_SET_CONTEXT_MENU_DETAIL:
windowState = contextMenuState.setContextMenu(windowState, action.detail)
if (action.contextMenuDetail) {
windowState = windowState.set('contextMenuDetail', action.contextMenuDetail)
} else {
windowState = windowState.delete('contextMenuDetail')
}
break
case windowConstants.WINDOW_SET_POPUP_WINDOW_DETAIL:
if (!action.detail) {
Expand Down

0 comments on commit abbfedf

Please sign in to comment.