New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Don't crash on `setApplicationMenu(null)` #11055

Merged
merged 4 commits into from Nov 11, 2017

Conversation

Projects
None yet
5 participants
@felixrieseberg
Member

felixrieseberg commented Nov 7, 2017

We found a little bug that snuck in during the ES6 transition. This PR ensures that calling setApplicationMenu(null) (which removes the current menu) doesn't crash.

Also: A spec!

Fixes #11052

@felixrieseberg felixrieseberg requested a review from codebytere Nov 7, 2017

@felixrieseberg felixrieseberg requested a review from electron/reviewers as a code owner Nov 7, 2017

@codebytere

thanks :)

@@ -133,7 +133,7 @@ Menu.sendActionToFirstResponder = bindings.sendActionToFirstResponder
// set application menu with a preexisting menu
Menu.setApplicationMenu = function (menu) {
if (!(menu || menu.constructor === Menu)) {
if (!(!menu || menu.constructor === Menu)) {

This comment has been minimized.

@poiru

poiru Nov 8, 2017

Member

I think this would be easier to read as menu && menu.constructor !== Menu

@@ -133,7 +133,7 @@ Menu.sendActionToFirstResponder = bindings.sendActionToFirstResponder
// set application menu with a preexisting menu
Menu.setApplicationMenu = function (menu) {
if (!(menu || menu.constructor === Menu)) {
if (menu && menu.constructor !== Menu)) {

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Nov 8, 2017

Member

There are one to many closing brackets here 😄

This comment has been minimized.

@felixrieseberg

felixrieseberg Nov 8, 2017

Member

That's what you get for trying to fix this on the phone 🗡

@ckerr

ckerr approved these changes Nov 8, 2017

Looks good, and 👍 on adding the extra spec too

@felixrieseberg

This comment has been minimized.

Member

felixrieseberg commented Nov 8, 2017

@ckerr

This comment has been minimized.

Member

ckerr commented Nov 10, 2017

I'm thinking we should merge this as-is since the test failures don't seem to be related to this patch.

Anyone opposed?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Nov 11, 2017

macOS failures I fixed in master, linux failure appears to be flake. Merging 😄

@MarshallOfSound MarshallOfSound merged commit ba754cf into master Nov 11, 2017

5 of 8 checks passed

electron-mas-x64 Build #5773 failed in 6 min 25 sec
Details
electron-osx-x64 Build #5748 failed in 7 min 4 sec
Details
ci/circleci: electron-linux-x64 Your tests failed on CircleCI
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MarshallOfSound MarshallOfSound deleted the fix-menu-bug branch Nov 11, 2017

@ckerr ckerr restored the fix-menu-bug branch Dec 19, 2017

@ckerr ckerr deleted the fix-menu-bug branch Dec 19, 2017

@ckerr

This comment has been minimized.

Member

ckerr commented Dec 19, 2017

This was merged into master before the 1-8-x branch was created, so both already have this fix; no need to backport to 1-8-x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment