Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2852233002:
Mac[Views]: Make native menus more responsive by pumping private runloop modes. (Closed)
DescriptionMac[Views]: Make native menus more responsive by pumping private runloop modes.
Native NSMenus run a UI-thread-blocking animation to flash the selected
item and fade out. This takes 300ms or more and events are only pumped
in private run loop modes. This can make menus feel really laggy -
<select> popups in particular.
A menu run by a native NSPopUpButton feels better since it uses a
different menu close animation which is opaque. Since it's opaque you
don't see how laggy the interface is behind it whilst the menu becomes
transparent. Using this animation is possible in MacViews, but its style
is fixed with a blue bar on the right, to match the native NSPopUpButton
appearance, so it looks weird. Changing any of the NSPopUpButtonCell's
style will just revert to a fade-out menu animation.
To get the menus feeling more responsive, "peek" at the processing on
the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is
sent early in the event processing and before any animations start
blocking the UI thread. MenuController can peek at this and (optionally)
post the action handling as a task. Then, observe the private run loop
modes that AppKit uses during menu fade and "un" highlight of the menu
bar. This allows the posted task to execute and compositing tasks to run
in parallel with the menu fade.
BUG=602914, 640466
TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark
star. Changing the Folder: should be immediately visible while the menu
is fading out.
Review-Url: https://codereview.chromium.org/2852233002
Cr-Commit-Position: refs/heads/master@{#470769}
Committed: https://chromium.googlesource.com/chromium/src/+/335d78a0e0299bda3465cb7e303b79e043f680da
Patch Set 1 : With the NSPopUpMenu animation #Patch Set 2 : Remove NSPopUpMenu hacks #Patch Set 3 : nits #Patch Set 4 : Soooooo nice #Patch Set 5 : Tweaks #Patch Set 6 : Ignore 10.9, 10.10. Works in 10.11. More test, self target #Patch Set 7 : Pump private run loops #
Total comments: 6
Patch Set 8 : fix tests, respond to comments, selfnits #
Total comments: 4
Patch Set 9 : base::CancelableClosure #Patch Set 10 : Merge private categories #
Total comments: 12
Patch Set 11 : respond to comments #
Messages
Total messages: 89 (76 generated)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Pump the compositor for frames before closing context menus BUG=602914 ========== to ========== Pump the compositor for frames before closing context menus. Native NSMenus start a threaded animation to flash the selected item and fade out. They then block the UI thread waiting for it to complete without pumping events. This means any work done after popUpMenuPositioningItem returns is delayed. Instead, do work within -[MenuController itemSelected:] after notifying the MenuModel. This allows the compositor work to be interleaved with the ~100ms menu fade animation rather than happening after it. BUG=602914 ==========
tapted@chromium.org changed reviewers: + rsesek@chromium.org
tapted@chromium.org changed reviewers: - rsesek@chromium.org
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Pump the compositor for frames before closing context menus. Native NSMenus start a threaded animation to flash the selected item and fade out. They then block the UI thread waiting for it to complete without pumping events. This means any work done after popUpMenuPositioningItem returns is delayed. Instead, do work within -[MenuController itemSelected:] after notifying the MenuModel. This allows the compositor work to be interleaved with the ~100ms menu fade animation rather than happening after it. BUG=602914 ========== to ========== MacViews: Make native menus more responsive. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and there is no opportunity to pump events on the UI thread. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it as it becomes transparent. Using this animation is possible, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. Allow a subclass of ui/base's MenuController to peek at this and act on the menu item before the menu starts to fade out. BUG=602914 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. ==========
tapted@chromium.org changed reviewers: + rsesek@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:140001) has been deleted
The CQ bit was unchecked by tapted@chromium.org
Description was changed from ========== MacViews: Make native menus more responsive. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and there is no opportunity to pump events on the UI thread. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it as it becomes transparent. Using this animation is possible, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. Allow a subclass of ui/base's MenuController to peek at this and act on the menu item before the menu starts to fade out. BUG=602914 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. ========== to ========== MacViews: Make native menus more responsive. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and there is no opportunity to pump events on the UI thread. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it whilst the menu becomes transparent. Using this animation is possible in MacViews, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance, so it looks weird. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. Allow a subclass of ui/base's MenuController to peek at this and act on the menu item before the menu starts to fade out. BUG=602914 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. ==========
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #11 (id:220001) has been deleted
Patchset #10 (id:200001) has been deleted
Patchset #9 (id:180001) has been deleted
Patchset #8 (id:160001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== MacViews: Make native menus more responsive. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and there is no opportunity to pump events on the UI thread. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it whilst the menu becomes transparent. Using this animation is possible in MacViews, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance, so it looks weird. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. Allow a subclass of ui/base's MenuController to peek at this and act on the menu item before the menu starts to fade out. BUG=602914 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. ========== to ========== MacViews: Make native menus more responsive. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and there is no opportunity to pump events on the UI thread. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it whilst the menu becomes transparent. Using this animation is possible in MacViews, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance, so it looks weird. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. Allow a subclass of ui/base's MenuController to peek at this and act on the menu item before the menu starts to fade out. BUG=602914 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. ==========
Hi Robert - please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== MacViews: Make native menus more responsive. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and there is no opportunity to pump events on the UI thread. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it whilst the menu becomes transparent. Using this animation is possible in MacViews, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance, so it looks weird. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. Allow a subclass of ui/base's MenuController to peek at this and act on the menu item before the menu starts to fade out. BUG=602914 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. ========== to ========== Mac[Views]: Make native menus more responsive. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and events are only pumped in private run loop modes. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it whilst the menu becomes transparent. Using this animation is possible in MacViews, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance, so it looks weird. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. MenuController can peek at this and (optionally) post the action handling as a task. Then, observe the private run loop modes that AppKit uses during menu fade and "un" highlight of the menu bar. This allows the posted task to execute and compositing tasks to run in parallel with the menu fade. BUG=602914 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. ==========
Description was changed from ========== Mac[Views]: Make native menus more responsive. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and events are only pumped in private run loop modes. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it whilst the menu becomes transparent. Using this animation is possible in MacViews, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance, so it looks weird. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. MenuController can peek at this and (optionally) post the action handling as a task. Then, observe the private run loop modes that AppKit uses during menu fade and "un" highlight of the menu bar. This allows the posted task to execute and compositing tasks to run in parallel with the menu fade. BUG=602914 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. ========== to ========== Mac[Views]: Make native menus more responsive. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and events are only pumped in private run loop modes. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it whilst the menu becomes transparent. Using this animation is possible in MacViews, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance, so it looks weird. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. MenuController can peek at this and (optionally) post the action handling as a task. Then, observe the private run loop modes that AppKit uses during menu fade and "un" highlight of the menu bar. This allows the posted task to execute and compositing tasks to run in parallel with the menu fade. BUG=602914 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. ==========
Hi Robert, please take a look. I think this is what you had in mind? Although, a current problem is that there are two failing interactive_ui_tests: BrowserActionButtonUiTest.ContextMenusOnMainAndOverflow cache BrowserActionButtonUiTest.MediaRouterActionContextMenuInOverflow cache When running the steps of those tests manually in Chromium with this patched in, everything seems to work. So I think it's just that the techniques that the test harness uses to spy on the menu interaction is failing to meet its expectations the same way. I've spend a while on this, but haven't managed to sort out a fix yet.. The tests are trying to spawn a new menu and they use a lot of PostTask. No luck so far trying to switch things to NSObject performSelector..afterDelay.. still hunting for another trick.
On 2017/05/08 10:22:57, tapted wrote: > Hi Robert, please take a look. I think this is what you had in mind? Yes, this is. > Although, a current problem is that there are two failing interactive_ui_tests: > BrowserActionButtonUiTest.ContextMenusOnMainAndOverflow cache > BrowserActionButtonUiTest.MediaRouterActionContextMenuInOverflow cache > > When running the steps of those tests manually in Chromium with this patched in, > everything seems to work. So I think it's just that the techniques that the test > harness uses to spy on the menu interaction is failing to meet its expectations > the same way. > > I've spend a while on this, but haven't managed to sort out a fix yet.. The > tests are trying to spawn a new menu and they use a lot of PostTask. No luck so > far trying to switch things to NSObject performSelector..afterDelay.. still > hunting for another trick. I'm wondering if the events and closure for SendMouseEventsNotifyWhenDone() are being pumped now while the menu is animating open. https://codereview.chromium.org/2852233002/diff/260001/ui/base/cocoa/menu_con... File ui/base/cocoa/menu_controller.mm (right): https://codereview.chromium.org/2852233002/diff/260001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller.mm:33: // Called by the posted task to selected an item during menu fade out. Make it clear that these are ui::EventFlags and not NSEventModifierFlags. https://codereview.chromium.org/2852233002/diff/260001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller.mm:257: [[sender target] respondsToSelector:@selector(itemSelected:eventFlags:)]) nit: braces https://codereview.chromium.org/2852233002/diff/260001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller.mm:342: base::MessageLoop::current()->task_runner()->PostTask( The new thing, which I can never remember, is: base::ThreadTaskRunnerHandle::Get()
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #8 (id:280001) has been deleted
Description was changed from ========== Mac[Views]: Make native menus more responsive. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and events are only pumped in private run loop modes. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it whilst the menu becomes transparent. Using this animation is possible in MacViews, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance, so it looks weird. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. MenuController can peek at this and (optionally) post the action handling as a task. Then, observe the private run loop modes that AppKit uses during menu fade and "un" highlight of the menu bar. This allows the posted task to execute and compositing tasks to run in parallel with the menu fade. BUG=602914 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. ========== to ========== Mac[Views]: Make native menus more responsive by pumping private runloop modes. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and events are only pumped in private run loop modes. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it whilst the menu becomes transparent. Using this animation is possible in MacViews, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance, so it looks weird. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. MenuController can peek at this and (optionally) post the action handling as a task. Then, observe the private run loop modes that AppKit uses during menu fade and "un" highlight of the menu bar. This allows the posted task to execute and compositing tasks to run in parallel with the menu fade. BUG=602914 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. ==========
PTAL - figured out the test. The problem was that the test harness invokes the browseraction context menu by opening it directly rather than via an event. Doing this via PostTask would get Cocoa into a situation where it would try to open a new menu before the prior menu closed. To fix, I've hooked menuDidClose for the context menu invoke. NSMenuWillSendActionNotification happens after that, so I think this is legit. https://codereview.chromium.org/2852233002/diff/260001/ui/base/cocoa/menu_con... File ui/base/cocoa/menu_controller.mm (right): https://codereview.chromium.org/2852233002/diff/260001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller.mm:33: // Called by the posted task to selected an item during menu fade out. On 2017/05/08 22:42:10, Robert Sesek wrote: > Make it clear that these are ui::EventFlags and not NSEventModifierFlags. Done. https://codereview.chromium.org/2852233002/diff/260001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller.mm:257: [[sender target] respondsToSelector:@selector(itemSelected:eventFlags:)]) On 2017/05/08 22:42:10, Robert Sesek wrote: > nit: braces Done. https://codereview.chromium.org/2852233002/diff/260001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller.mm:342: base::MessageLoop::current()->task_runner()->PostTask( On 2017/05/08 22:42:10, Robert Sesek wrote: > The new thing, which I can never remember, is: > > base::ThreadTaskRunnerHandle::Get() Done. (I wish there was just base::PostUITask(..) or something :/)
LGTM. Please also get Mark to review and add BUG=640466. https://codereview.chromium.org/2852233002/diff/300001/ui/base/cocoa/menu_con... File ui/base/cocoa/menu_controller.mm (right): https://codereview.chromium.org/2852233002/diff/300001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller.mm:42: class PostedItemSelectedTask I think you can replace this class with a base::CancelableCallback. It may be a little easier to follow, since the base::Bind logic could live in -itemWillBeSelected: (assuming that scoped_nsobject plays well with Bind). https://codereview.chromium.org/2852233002/diff/300001/ui/base/cocoa/menu_con... File ui/base/cocoa/menu_controller_unittest.mm (right): https://codereview.chromium.org/2852233002/diff/300001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller_unittest.mm:517: /* No pump. */ nit: /* -> //
Patchset #9 (id:320001) has been deleted
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Mac[Views]: Make native menus more responsive by pumping private runloop modes. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and events are only pumped in private run loop modes. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it whilst the menu becomes transparent. Using this animation is possible in MacViews, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance, so it looks weird. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. MenuController can peek at this and (optionally) post the action handling as a task. Then, observe the private run loop modes that AppKit uses during menu fade and "un" highlight of the menu bar. This allows the posted task to execute and compositing tasks to run in parallel with the menu fade. BUG=602914 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. ========== to ========== Mac[Views]: Make native menus more responsive by pumping private runloop modes. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and events are only pumped in private run loop modes. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it whilst the menu becomes transparent. Using this animation is possible in MacViews, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance, so it looks weird. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. MenuController can peek at this and (optionally) post the action handling as a task. Then, observe the private run loop modes that AppKit uses during menu fade and "un" highlight of the menu bar. This allows the posted task to execute and compositing tasks to run in parallel with the menu fade. BUG=602914, 640466 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. ==========
tapted@chromium.org changed reviewers: + mark@chromium.org
Mark: can you take a look too - thanks! On 2017/05/09 15:49:45, Robert Sesek wrote: > Please also get Mark to review and add BUG=640466. Done :) https://codereview.chromium.org/2852233002/diff/300001/ui/base/cocoa/menu_con... File ui/base/cocoa/menu_controller.mm (right): https://codereview.chromium.org/2852233002/diff/300001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller.mm:42: class PostedItemSelectedTask On 2017/05/09 15:49:45, Robert Sesek wrote: > I think you can replace this class with a base::CancelableCallback. It may be a > little easier to follow, since the base::Bind logic could live in > -itemWillBeSelected: (assuming that scoped_nsobject plays well with Bind). Done (along with base::BindBlock to make the ObjC method call) https://codereview.chromium.org/2852233002/diff/300001/ui/base/cocoa/menu_con... File ui/base/cocoa/menu_controller_unittest.mm (right): https://codereview.chromium.org/2852233002/diff/300001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller_unittest.mm:517: /* No pump. */ On 2017/05/09 15:49:45, Robert Sesek wrote: > nit: /* -> // Done. https://codereview.chromium.org/2852233002/diff/350001/ui/base/cocoa/menu_con... File ui/base/cocoa/menu_controller.h (right): https://codereview.chromium.org/2852233002/diff/350001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller.h:85: - (BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item; (This is called outside of testcode, so I merged the categories: app_menu_controller.mm:262:31: error: 'MenuController' may not respond to 'validateUserInterfaceItem:' [-Werror] const BOOL enabled = [super validateUserInterfaceItem:item]; )
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2852233002/diff/350001/base/message_loop/mess... File base/message_loop/message_pump_mac.mm (right): https://codereview.chromium.org/2852233002/diff/350001/base/message_loop/mess... base/message_loop/message_pump_mac.mm:44: void CFRunLoopAddSourceToAllModes(CFRunLoopRef rl, CFRunLoopSourceRef source) { One last thought: it would be nice if we could limit this to the MessagePumpNSApplication, but I don't see a clean way to do that since these are installed in the MessagePumpCFRunLoopBase and the members are private, not protected. Maybe Mark has an idea. https://codereview.chromium.org/2852233002/diff/350001/ui/base/cocoa/menu_con... File ui/base/cocoa/menu_controller.h (right): https://codereview.chromium.org/2852233002/diff/350001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller.h:88: // with the entry in the model identified by |modelIndex|. nit: no modelIndex in the args https://codereview.chromium.org/2852233002/diff/350001/ui/base/cocoa/menu_con... File ui/base/cocoa/menu_controller.mm (right): https://codereview.chromium.org/2852233002/diff/350001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller.mm:227: base::MakeUnique<base::CancelableClosure>(base::BindBlock(^() { nit: can omit the () and just do |^{|.
LGTM. Excited to try this out. https://codereview.chromium.org/2852233002/diff/350001/base/message_loop/mess... File base/message_loop/message_pump_mac.mm (right): https://codereview.chromium.org/2852233002/diff/350001/base/message_loop/mess... base/message_loop/message_pump_mac.mm:35: kCFRunLoopCommonModes, kMessageLoopExclusiveRunLoopMode, I guess I’m not supposed to complain if clang-format wrote this, but the second item on this line is kind of hiding.
https://codereview.chromium.org/2852233002/diff/350001/base/message_loop/mess... File base/message_loop/message_pump_mac.mm (right): https://codereview.chromium.org/2852233002/diff/350001/base/message_loop/mess... base/message_loop/message_pump_mac.mm:35: kCFRunLoopCommonModes, kMessageLoopExclusiveRunLoopMode, On 2017/05/10 14:00:18, Mark Mentovai wrote: > I guess I’m not supposed to complain if clang-format wrote this, but the second > item on this line is kind of hiding. I complain all the time about this because clang-format is wrong, and I'm not going to apologize for that :). I've started using "// clang-format off" around lists of things to compensate.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2852233002/diff/350001/base/message_loop/mess... File base/message_loop/message_pump_mac.mm (right): https://codereview.chromium.org/2852233002/diff/350001/base/message_loop/mess... base/message_loop/message_pump_mac.mm:35: kCFRunLoopCommonModes, kMessageLoopExclusiveRunLoopMode, On 2017/05/10 14:02:49, Robert Sesek wrote: > On 2017/05/10 14:00:18, Mark Mentovai wrote: > > I guess I’m not supposed to complain if clang-format wrote this, but the > second > > item on this line is kind of hiding. > > I complain all the time about this because clang-format is wrong, and I'm not > going to apologize for that :). > > I've started using "// clang-format off" around lists of things to compensate. I added a comment, // Mode that only sees Chrome work sources. kMessageLoopExclusiveRunLoopMode, (Incidentally... and probably orthogonal, do we still need kMessageLoopExclusiveRunLoopMode? `git grep` only shows it being passed to CFRunLoopAddObserver(), but then only in combination with kCFRunLoopCommonModes -- it looks like we only ever spin a run loop in the kMessageLoopExclusiveRunLoopMode in a couple of unit test files). https://codereview.chromium.org/2852233002/diff/350001/base/message_loop/mess... base/message_loop/message_pump_mac.mm:44: void CFRunLoopAddSourceToAllModes(CFRunLoopRef rl, CFRunLoopSourceRef source) { On 2017/05/10 13:37:27, Robert Sesek wrote: > One last thought: it would be nice if we could limit this to the > MessagePumpNSApplication, but I don't see a clean way to do that since these are > installed in the MessagePumpCFRunLoopBase and the members are private, not > protected. Maybe Mark has an idea. We could pass a std::vector of scoped_cftyperef<CFStringRef> to the MessagePumpCFRunLoopBase constructor? Or a vector<size_t> of kAllModes indices. I don't know enough about the message pump stuff to know how crucial this is.. Leaving as is, but LMK if I should explore it now, or in a follow-up. https://codereview.chromium.org/2852233002/diff/350001/ui/base/cocoa/menu_con... File ui/base/cocoa/menu_controller.h (right): https://codereview.chromium.org/2852233002/diff/350001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller.h:88: // with the entry in the model identified by |modelIndex|. On 2017/05/10 13:37:27, Robert Sesek wrote: > nit: no modelIndex in the args Updated: // Adds the item at |index| in |model| as an NSMenuItem at |index| of |menu|. // Associates a submenu if the MenuModel::ItemType is TYPE_SUBMENU. https://codereview.chromium.org/2852233002/diff/350001/ui/base/cocoa/menu_con... File ui/base/cocoa/menu_controller.mm (right): https://codereview.chromium.org/2852233002/diff/350001/ui/base/cocoa/menu_con... ui/base/cocoa/menu_controller.mm:227: base::MakeUnique<base::CancelableClosure>(base::BindBlock(^() { On 2017/05/10 13:37:27, Robert Sesek wrote: > nit: can omit the () and just do |^{|. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, mark@chromium.org Link to the patchset: https://codereview.chromium.org/2852233002/#ps370001 (title: "respond to comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 370001, "attempt_start_ts": 1494465011303390,
"parent_rev": "91d3ee3c63734399ce574d0ed71f4004fb7e8605", "commit_rev":
"335d78a0e0299bda3465cb7e303b79e043f680da"}
Message was sent while issue was closed.
Description was changed from ========== Mac[Views]: Make native menus more responsive by pumping private runloop modes. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and events are only pumped in private run loop modes. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it whilst the menu becomes transparent. Using this animation is possible in MacViews, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance, so it looks weird. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. MenuController can peek at this and (optionally) post the action handling as a task. Then, observe the private run loop modes that AppKit uses during menu fade and "un" highlight of the menu bar. This allows the posted task to execute and compositing tasks to run in parallel with the menu fade. BUG=602914, 640466 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. ========== to ========== Mac[Views]: Make native menus more responsive by pumping private runloop modes. Native NSMenus run a UI-thread-blocking animation to flash the selected item and fade out. This takes 300ms or more and events are only pumped in private run loop modes. This can make menus feel really laggy - <select> popups in particular. A menu run by a native NSPopUpButton feels better since it uses a different menu close animation which is opaque. Since it's opaque you don't see how laggy the interface is behind it whilst the menu becomes transparent. Using this animation is possible in MacViews, but its style is fixed with a blue bar on the right, to match the native NSPopUpButton appearance, so it looks weird. Changing any of the NSPopUpButtonCell's style will just revert to a fade-out menu animation. To get the menus feeling more responsive, "peek" at the processing on the NSMenuItem by hooking -[NSMenuItem _sendItemSelectedNote]. This is sent early in the event processing and before any animations start blocking the UI thread. MenuController can peek at this and (optionally) post the action handling as a task. Then, observe the private run loop modes that AppKit uses during menu fade and "un" highlight of the menu bar. This allows the posted task to execute and compositing tasks to run in parallel with the menu fade. BUG=602914, 640466 TEST=With chrome://flags#secondary-ui-md enabled, click the bookmark star. Changing the Folder: should be immediately visible while the menu is fading out. Review-Url: https://codereview.chromium.org/2852233002 Cr-Commit-Position: refs/heads/master@{#470769} Committed: https://chromium.googlesource.com/chromium/src/+/335d78a0e0299bda3465cb7e303b... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:370001) as https://chromium.googlesource.com/chromium/src/+/335d78a0e0299bda3465cb7e303b...
Message was sent while issue was closed.
https://codereview.chromium.org/2852233002/diff/350001/base/message_loop/mess... File base/message_loop/message_pump_mac.mm (right): https://codereview.chromium.org/2852233002/diff/350001/base/message_loop/mess... base/message_loop/message_pump_mac.mm:35: kCFRunLoopCommonModes, kMessageLoopExclusiveRunLoopMode, On 2017/05/11 00:24:06, tapted wrote: > On 2017/05/10 14:02:49, Robert Sesek wrote: > > On 2017/05/10 14:00:18, Mark Mentovai wrote: > > > I guess I’m not supposed to complain if clang-format wrote this, but the > > second > > > item on this line is kind of hiding. > > > > I complain all the time about this because clang-format is wrong, and I'm not > > going to apologize for that :). > > > > I've started using "// clang-format off" around lists of things to compensate. > > I added a comment, > > // Mode that only sees Chrome work sources. > kMessageLoopExclusiveRunLoopMode, > > > (Incidentally... and probably orthogonal, do we still need > kMessageLoopExclusiveRunLoopMode? > > `git grep` only shows it being passed to CFRunLoopAddObserver(), but then only > in combination with kCFRunLoopCommonModes -- it looks like we only ever spin a > run loop in the kMessageLoopExclusiveRunLoopMode in a couple of unit test > files). I was wondering the same thing. The Exclusive mode was added a while ago, but I don't think it was ultimately used as intended. I can't tell if the few cases where it's currently used are actually required, or if CommonModes are sufficient. https://codereview.chromium.org/2852233002/diff/350001/base/message_loop/mess... base/message_loop/message_pump_mac.mm:44: void CFRunLoopAddSourceToAllModes(CFRunLoopRef rl, CFRunLoopSourceRef source) { On 2017/05/11 00:24:05, tapted wrote: > On 2017/05/10 13:37:27, Robert Sesek wrote: > > One last thought: it would be nice if we could limit this to the > > MessagePumpNSApplication, but I don't see a clean way to do that since these > are > > installed in the MessagePumpCFRunLoopBase and the members are private, not > > protected. Maybe Mark has an idea. > > We could pass a std::vector of scoped_cftyperef<CFStringRef> to the > MessagePumpCFRunLoopBase constructor? Or a vector<size_t> of kAllModes indices. > > I don't know enough about the message pump stuff to know how crucial this is.. > Leaving as is, but LMK if I should explore it now, or in a follow-up. I don't think it's critical since the mode should only be used on the AppKit main thread. Passing a vector of additional modes (in addition to CommonModes) to the MessagePumpCFRunLoopBase constructor seems reasonable. If this CL sticks, I think it's worth considering. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
