Skip to content
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

chore: drop unresponsive suppressor for menus #35498

Merged
merged 3 commits into from Aug 31, 2022

Conversation

nornagon
Copy link
Member

Description of Change

This no longer seems to be necessary since #20114 moved the menu popup loop off
the UI thread.

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes

Release Notes

Notes: none

@nornagon nornagon added no-backport semver/patch backwards-compatible bug fixes labels Aug 29, 2022
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 29, 2022
@nornagon nornagon changed the title chore: drop unresponsive suppressor for menu_mac chore: drop unresponsive suppressor for menus Aug 29, 2022
@nornagon
Copy link
Member Author

Wanna ensure we don't reintroduce #2947

@nornagon
Copy link
Member Author

I tested this PR on Windows 10 and was unable to reproduce #2947.

@nornagon
Copy link
Member Author

cc @zcbenz in case there's anything I missed—this code has a long heritage!

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 30, 2022
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The menu popup loop still runs on UI thread, #20114 didn't change that. But code had been added to allow nested tasks in menu popup loop so the unresponsive problem may have been solved by it.

I think we should still do testing on macOS.

@@ -156,7 +155,6 @@
base::mac::ScopedSendingEvent sendingEventScoper;

// Don't emit unresponsive event when showing menu.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be removed too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #35507

@nornagon
Copy link
Member Author

I did test on macOS and wasn't able to cause an unresponsive event to happen by showing a menu.

@nornagon
Copy link
Member Author

mac test failures are flake; merging.

@nornagon nornagon merged commit 9cdc8bf into main Aug 31, 2022
@nornagon nornagon deleted the no-menu-unresponsive-suppressor branch August 31, 2022 17:25
@release-clerk
Copy link

release-clerk bot commented Aug 31, 2022

No Release Notes

kyrylo-hrechykhin pushed a commit to kyrylo-hrechykhin/electron that referenced this pull request Sep 1, 2022
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants