-
Notifications
You must be signed in to change notification settings - Fork 974
Only store menu in appState on Windows #9991
Conversation
app/browser/menu.js
Outdated
if (isWindows) { | ||
const menuDataOnly = JSON.parse(JSON.stringify(template)) | ||
appActions.setMenubarTemplate(Immutable.fromJS(menuDataOnly)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not ideal- could use some help finding a better way to do this
app/common/lib/platformUtil.js
Outdated
} | ||
|
||
module.exports.isWindows = () => { | ||
return process.platform === 'win32' || | ||
(navigator && navigator.platform === 'Win32') | ||
(typeof navigator !== 'undefined' && navigator.platform === 'Win32') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed because navigator
is undefined in the browser process (which causes a crash).
I can make an issue for this too (if it makes sense) |
OK made some tweaks, ready for re-review @bridiver @bbondy Here are some important notes:
Some notes about existing behavior
|
Could you post or link up an issue for this? Thanks. |
- methods are stripped out (will no longer be serialized) - with a few thousand bookmarks, the initial stringify/parse takes ~60ms and is never done again - menu (if present) is now deleted when saving session Fixes #10016 Some notes about existing behavior The menu is modified in memory when: - tab is closed (history menu rebuilt - serialization added w/ this PR taking 0 ms) - menu item goes from checked to unchecked (or similar) Auditors: @bridiver, @bbondy
@@ -602,7 +606,10 @@ const createMenu = (state) => { | |||
}) | |||
} | |||
|
|||
appActions.setMenubarTemplate(Immutable.fromJS(template)) | |||
if (isWindows) { | |||
const menuTemplate = JSON.parse(JSON.stringify(template)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I talked to Clifton, the handlers aren't actually used in app state and this is only called once.
Only store menu in appState on Windows
Only store menu in appState on Windows
Only store menu in appState on Windows
... and even then, store only the data (don't serialize functions, etc).
Auditors: @bridiver, @bbondy
Submitter Checklist:
git rebase -i
to squash commits (if needed).Reviewer Checklist:
Tests