Any progress here?

For previous discussions and context, see:
https://bugs.chromium.org/p/chromium/issues/detail?id=651203
https://bugs.chromium.org/p/chromium/issues/detail?id=383558
https://sites.google.com/a/chromium.org/dev/developers/design-documents/command-dispatch-mac

Here's what happens on pressing a hotkey on macOS. Each level of indentation refers to a caller/callee relationship. Comments are prefixed with +++ and refer to the previous item.

* -[NSApplication(NSEvent) sendEvent:]                                            (1)
  * routeKeyEquivalent
    * ChromeEventProcessingWindow performKeyEquivalent:                           (2)
      +++ This can early out by returning "true"
    * [NSMenu performKeyEquivalent:]
      * NSMenuDelegate menuHasKeyEquivalent:
      * -[NSCarbonMenuImpl performActionWithHighlightingForItemAtIndex:]
         *  Highlight menu item
         * -[NSCarbonMenuImpl performActionWithHighlightingForItemAtIndex:]
           * -[BrowserCrApplication sendAction:to:from:]
             * -[ChromeEventProcessingWindow commandDispatch:]                    (3)
         * Unhighlight menu item
           +++ Spins a nested CFRunLoop in NSUnhighlightMenuRunLoopMode for 100ms.

There are two approaches to solve the problem:
* Call (3) directly from (2) and return "true", short circuiting the logic that would trigger menu highlighting.
* Instantiate ScopedPumpMessagesInPrivateModes in (3), reset it in (1).

Why does it work in the cocoa version but not in views?

The second approach doesn't actually work. There's a second throttle in [NSMenu performKeyEquivalent]: Instead of spending all the time spinning in NSUnhighlightMenuRunLoopMode, instead the application spends all its time sleeping in:

* -[NSMenu performKeyEquivalent:]
  * -[NSCarbonMenuImpl performActionWithHighlightingForItemAtIndex:]
    * _NSHighlightCarbonMenu
      * NSMENU_IS_THROTTLING_REPEATED_MENU_ITEM_INVOCATIONS
        * usleep

re c#7:

Whoops, all of the analysis in c#6 and c#8 *is* for the Cocoa version. Looking into the Views version now.

Turns out the Views flow is basically the same as the Cocoa flow, just with some different classes stubbed into the hierarchy. I've expanded the control flow to include the CommandDispatcherTarget [RenderWidgetHostView] flow, since that's relevant.

If we want to short-circuit cmd+W commands, we'll need to do so in two locations: BrowserFrameMac::PreHandleKeyboardEvent and NativeWidgetMacNSWindow performKeyEquivalent:.

* -[NSApplication(NSEvent) sendEvent:]
  * routeKeyEquivalent
    * NativeWidgetMacNSWindow performKeyEquivalent:
       * CommandDispatcher performKeyEquivalent:
          * CommandDispatcherTarget performKeyEquivalent:
            * ...
              * BrowserFrameMac::PreHandleKeyboardEvent
                * [NSMenu performKeyEquivalent:]
          * ChromeCommandDispatcherDelegate prePerformKeyEquivalent:     
          * NativeWidgetMacNSWindow defaultPerformKeyEquivalent:
            * [NSWindow performKeyEquivalent:]
          ...
    * [NSMenu performKeyEquivalent:]
      * NSMenuDelegate menuHasKeyEquivalent:
      * -[NSCarbonMenuImpl performActionWithHighlightingForItemAtIndex:]
         *  Highlight menu item
           +++ Sleeps if called too frequently.
         * -[NSCarbonMenuImpl performActionWithHighlightingForItemAtIndex:]
           * -[BrowserCrApplication sendAction:to:from:]
             * NativeWidgetMacNSWindow commandDispatch:
         * Unhighlight menu item
           +++ Spins a nested CFRunLoop in NSUnhighlightMenuRunLoopMode for 100ms.

Note this bug isn't necessarily about shortcutting, just about keeping cmd-w pressed and expecting multiple tabs to close over time.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f2f9568808ee117698e0a9008a0b52902d2363e

commit 8f2f9568808ee117698e0a9008a0b52902d2363e
Author: Erik Chen <erikchen@chromium.org>
Date: Fri Jun 08 00:44:54 2018

Unification of macOS keyEquivalent (hotkey) handling.

This CL makes the CommandDispatcher class responsible for all NSMenu-related
keyEquivalent handling. The logic is now shared by both Views and Cocoa. This
fixes many Views bugs, and some Cocoa bugs.

Previously, there several different pieces of logic for handling NSMenu-related
keyEquivalents.

* If a RenderWidgetHostViewCocoa was the firstResponder, then all the logic in
  CommandDispatcher was skipped.
  * In both Cocoa and Views, there was logic in content/ via RenderWidget*
    classes that would short-circuit but also fail to handle the keyEquivalent.
    [e.g. if renderer process was killed].
    * In Cocoa, content/ would call into
      BrowserWindowCocoa::PreHandleKeyboardEvent(). This had mostly the right
      behavior, except it missed out on edge-case logic in CommandDispatcher.
    * In Views, content/ would call into BrowserView::PreHandleKeyboardEvent.
      This relied on AcceleratorTable to lookup the command for a keyEquivalent.
      This entire class is unusable for keyEquivalents because it holds a static
      mapping, whereas on macOS, the mapping is user configurable and dynamic.
  * If the hotkey was not immediately consumed, it would be passed to the
    renderer process. If the renderer process chose not to consume the hotkey,
    the browser process would get it again.
    * In Cocoa, the event would be redispatched to CommandDispatcher. This had
      the right behavior.
    * In Views, the event would be sent to AcceleratorTable again, once again
      having the wrong behavior.
* If any other class was firstResponder, then the logic in CommandDispatcher
  would trigger.

This CL makes CommandDispatcher the only place to handle NSMenu-related
keyEquivalents. This allows us to centralize the logic, including workarounds.

New logic:

* The CommandDispatcherDelegate is given an opportunity to process the
  keyEquivalent via prePerformKeyEquivalent:. If the keyEquivalent is reserved
  by the browser, it is immediately handled.
* Next, the firstResponder is given a chance to process the keyEquivalent. If
  the firstResponder is a RenderWidgetHostViewCocoa, this goes through the same
  code paths as above, except BrowserWindowCocoa::PreHandleKeyboardEvent() is
  mostly a no-op.
* Then, the CommandDispatcherDelegate gets another chance to process the
  keyEquivalent via postPerformKeyEquivalent:.

If the firstResponder was a RenderWidgetHostViewCocoa which asynchronously chose
to not process the event, then both Cocoa and Views will take the returning
event and call redispatchEvent:, which will also call into
-[CommandDispatcherDelegate postPerformKeyEquivalent:].

Change-Id: If724e125c6acf64ddd9f1d9cc296e761ffb2a886
Bug:  846893 ,  836947 ,  845465 ,  47134 
Reviewed-on: https://chromium-review.googlesource.com/1082818
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565490}
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/global_keyboard_shortcuts_mac.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/global_keyboard_shortcuts_mac.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/global_keyboard_shortcuts_mac_unittest.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/accelerators_cocoa.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/accelerators_cocoa.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_interactive_uitest.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/browser_window_cocoa.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/browser_window_utils.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/accelerator_table.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/accelerator_table.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/accelerator_table_unittest_mac.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_ash.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_ash.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_mac.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_mac.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_mash.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_frame_mash.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/browser_view_unittest.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/desktop_browser_frame_aura.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/desktop_browser_frame_aura.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/frame/native_browser_frame.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/browser/ui/views/media_router/presentation_receiver_window_view.cc
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/chrome/test/BUILD.gn
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/content/browser/renderer_host/render_widget_host_view_cocoa.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/ui/base/cocoa/command_dispatcher.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/ui/base/cocoa/command_dispatcher.mm
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/ui/events/keycodes/keyboard_code_conversion_mac.h
[modify] https://crrev.com/8f2f9568808ee117698e0a9008a0b52902d2363e/ui/events/keycodes/keyboard_code_conversion_mac.mm

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3be9df1c51294f066b0f3b6ecc389421b76d287d

commit 3be9df1c51294f066b0f3b6ecc389421b76d287d
Author: erikchen <erikchen@chromium.org>
Date: Thu Jun 21 16:14:13 2018

macOS: Let reserved keyEquivalents bypass the main menu.

The main menu has a built-in 100ms delay. Combined with higher priority for
incoming NSEvents compared to chrome tasks, holding down key equivalents [e.g.
cmd + w] starves the chrome task queue. The implementation of tab-closing
requires the main thread to process the on unload ACK from the renderer before
closing the tab [which is a chrome task], so this causes a live-lock.

This CL lets reserved keyEquivalents bypass the main menu. This avoids the
live-lock indicated above. This also loses the built-in 100ms throttle, so this
CL adds a custom 50ms throttle [disabled in tests].

Bug:  836947 
Change-Id: Idb88ee84d7aa82da4104893ed85358c75e385ea3
Reviewed-on: https://chromium-review.googlesource.com/1108095
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569285}
[modify] https://crrev.com/3be9df1c51294f066b0f3b6ecc389421b76d287d/chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h
[modify] https://crrev.com/3be9df1c51294f066b0f3b6ecc389421b76d287d/chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm
[modify] https://crrev.com/3be9df1c51294f066b0f3b6ecc389421b76d287d/chrome/test/BUILD.gn
[modify] https://crrev.com/3be9df1c51294f066b0f3b6ecc389421b76d287d/chrome/test/base/in_process_browser_test.cc
[modify] https://crrev.com/3be9df1c51294f066b0f3b6ecc389421b76d287d/chrome/test/base/in_process_browser_test.h
[rename] https://crrev.com/3be9df1c51294f066b0f3b6ecc389421b76d287d/chrome/test/base/in_process_browser_test_mac.mm

If today's canary has the delay, then the delay feels maybe just a tad too long for me. Is it possible to query the key repeat rate in sysprefs? I have that as low as possible, and if I keep a key pressed for 1 s, I get 33 letters (in apps that disable that weird "keep a pressed to get the opportunity to enter àáâ etc feature like terminal or macvim at least) -- so it's understandable that the 50ms feel just a bit too slow to me. (I noticed it feeling to slow before i did the key repeat experiment.)

> I have that as low as possible,

I hear you, same for me. My main concern is that the tab close rate is not constant, which really skew how long I want to hold down cmd + W.

So for example, if I have 100 NTPs, and each takes the same amount of time to close, then 50ms feels too long. But when I have real tabs, what I notice is that a single tab will stall for a long time [seconds], waiting for the unload handler, and then when that returns suddenly a bunch of tabs will close. In that case, 50ms actually feels too short, and I usually close more than I had intended.

50ms seemed like an okay middle-of-the-ground number.

Going through tabs by keeping cmd-shift-[ pressed is maybe a more common use case than cmd-w. I opened 20 tabs and kept cmd-shift-[ down for a second, and that went through all tabs in that time, which felt a bit slow.

To be clear, it doesn't feel super off, just a bit. Going through 20 tabs with cmd-shift-w in safari definitely feels worse, due to the weird tab-shifting-around thing they do. If getting the key repeat rate isn't super hard, trying to use that would be nice and chances are it'll feel subtly better, but as-is isn't bad.

err, make that "with cmd-shift-[" in comment 19.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ccb5d3f8c79cd97dd76c2a4515363c8cbb7295a3

commit ccb5d3f8c79cd97dd76c2a4515363c8cbb7295a3
Author: erikchen <erikchen@chromium.org>
Date: Fri Jun 22 21:53:02 2018

Remove throttling interval for quickly repeated key equivalents.

The system preferences for key repeat rate already limits the rate that
events are generated, so there's no need for another, arbitrary throttle.

Bug:  836947 
Change-Id: I35927d4b475a45d7ff43d3d8f7e746e86f11f746
Reviewed-on: https://chromium-review.googlesource.com/1112561
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569796}
[modify] https://crrev.com/ccb5d3f8c79cd97dd76c2a4515363c8cbb7295a3/chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h
[modify] https://crrev.com/ccb5d3f8c79cd97dd76c2a4515363c8cbb7295a3/chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm
[modify] https://crrev.com/ccb5d3f8c79cd97dd76c2a4515363c8cbb7295a3/chrome/test/base/in_process_browser_test.cc
[modify] https://crrev.com/ccb5d3f8c79cd97dd76c2a4515363c8cbb7295a3/chrome/test/base/in_process_browser_test.h
[modify] https://crrev.com/ccb5d3f8c79cd97dd76c2a4515363c8cbb7295a3/chrome/test/base/in_process_browser_test_mac.mm

Unable to reproduce the issue the issue on chrome version# 68.0.3406.0 using Mac 10.12.6 with steps mentioned below:
1) Launched chrome version and enabled Mac views browser
2) Pressed cmd-t for a bit, menu bar triggers continuously and few tabs got opened
3) Pressed cmd-w for a bit, menu bar triggers continuously and observed tabs got closed continuously

@Reporter: Please find the attached screencast for your reference and let us know if we missed anything in reproducing the issue and help us in verifying the fix.

Thanks!

Deleted: 836947.mp4 1.4 MB

	836947.mp4 1.4 MB View Download