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}

chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc

@@ -978,6 +978,14 @@ IN_PROC_BROWSER_TEST_F(WebViewInteractiveTest, EditCommandsNoMenu) {
 
   ExtensionTestMessageListener start_of_line_listener("StartOfLine", false);
   SendStartOfLineKeyPressToPlatformApp();
+#if defined(OS_MACOSX)
+  // On macOS, sending an accelerator [key-down] will also cause the subsequent
+  // key-up to be swallowed. The implementation of guest.html is waiting for a
+  // key-up to send the caret-position message. So we send a key-down/key-up of
+  // a character that otherwise has no effect.
+  ASSERT_TRUE(ui_test_utils::SendKeyPressToWindowSync(
+      GetPlatformAppWindow(), ui::VKEY_UP, false, false, false, false));
+#endif
   // Wait for the guest to receive a 'copy' edit command.
   ASSERT_TRUE(start_of_line_listener.WaitUntilSatisfied());
 }

chrome/browser/global_keyboard_shortcuts_mac.h

@@ -5,12 +5,19 @@
 #ifndef CHROME_BROWSER_GLOBAL_KEYBOARD_SHORTCUTS_MAC_H_
 #define CHROME_BROWSER_GLOBAL_KEYBOARD_SHORTCUTS_MAC_H_
 
-#include <Carbon/Carbon.h>  // For unichar.
 #include <stddef.h>
 
 #include <vector>
 
+#if defined(__OBJC__)
 @class NSEvent;
+#else   // __OBJC__
+class NSEvent;
+#endif  // __OBJC__
+
+namespace ui {
+class Accelerator;
+}
 
 struct KeyboardShortcutData {
   bool command_key;
@@ -22,70 +29,50 @@ struct KeyboardShortcutData {
   // should be specified instead.
   // Set 0 for the one you do not want to specify.
   int vkey_code;  // Virtual Key code for the command.
-  unichar key_char;  // Key event characters for the command as reported by
-                     // [NSEvent charactersIgnoringModifiers].
+
+  // Key event characters for the command as reported by
+  // [NSEvent charactersIgnoringModifiers].
+  // This should be a unichar, but the type is defined in
+  // Foundation.framework/.../NSString.h, which is an ObjC header. This header
+  // is included in non-ObjC translation units, so we cannot rely on that
+  // include.
+  unsigned short key_char;
+
   int chrome_command;  // The chrome command # to execute for this shortcut.
 };
 
-// Check if a given keycode + modifiers (or keychar + modifiers if the
-// |key_char| is specified) correspond to a given Chrome command.
-// returns: Command number (as passed to
-// BrowserCommandController::ExecuteCommand) or -1 if there was no match.
+// macOS applications are supposed to put all keyEquivalents [hotkeys] in the
+// menu bar. For legacy reasons, Chrome does not. There are around 30 hotkeys
+// that are explicitly coded to virtual keycodes. This has the following
+// downsides:
+//  * There is no way for the user to configure or disable these keyEquivalents.
+//  * This can cause keyEquivalent conflicts for non-US keyboard layouts with
+//    different default keyEquivalents, see https://crbug.com/841299.
 //
-// |performKeyEquivalent:| bubbles events up from the window to the views.  If
-// we let it bubble up to the Omnibox, then the Omnibox handles cmd-left/right
-// just fine, but it swallows cmd-1 and doesn't give us a chance to intercept
-// this. Hence, we need three types of keyboard shortcuts: shortcuts that are
-// intercepted before the Omnibox handles events, shortcuts that are
-// intercepted after the Omnibox had a chance but did not handle them, and
-// shortcuts that are only handled when tab contents is focused.
+// This function first searches the menu bar for a matching keyEquivalent. If
+// nothing is found, then it searches through the explicitly coded virtual
+// keycodes not present in the NSMenu.
 //
-// This means cmd-left doesn't work if you hit cmd-l tab, which focusses
-// something that's neither omnibox nor tab contents. This behavior is
-// consistent with safari and camino, and I think it's the best we can do
-// without rewriting event dispatching ( http://crbug.com/251069 ).
-
-// This returns shortcuts that should work no matter what component of the
-// browser is focused. They are executed by the window, before any view has the
-// opportunity to override the shortcut (with the exception of the tab contents,
-// which first checks if the current web page wants to handle the shortcut).
-int CommandForWindowKeyboardShortcut(
-    bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
-    int vkey_code, unichar key_char);
-
-// This returns shortcuts that should work no matter what component of the
-// browser is focused. They are executed by the window, after any view has the
-// opportunity to override the shortcut
-int CommandForDelayedWindowKeyboardShortcut(
-    bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
-    int vkey_code, unichar key_char);
-
-// This returns shortcuts that should work only if the tab contents have focus
-// (e.g. cmd-left, which shouldn't do history navigation if e.g. the omnibox has
-// focus).
-int CommandForBrowserKeyboardShortcut(
-    bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
-    int vkey_code, unichar key_char);
-
-// Returns the Chrome command associated with |event|, or -1 if not found.
+// Note: AppKit exposes symbolic hotkeys [e.g. cmd + `] not present in the
+// NSMenu as well. The user can remap these to conflict with Chrome hotkeys.
+// This function will return the Chrome hotkey, regardless of whether there's a
+// conflicting symbolic hotkey.
 int CommandForKeyEvent(NSEvent* event);
 
-// Returns the menu command associated with |event|, or -1 if not found.
-int MenuCommandForKeyEvent(NSEvent* event);
+// Whether the event goes through the performKeyEquivalent: path and is handled
+// by CommandDispatcher.
+bool EventUsesPerformKeyEquivalent(NSEvent* event);
 
-// Returns a keyboard event character for the given |event|.  In most cases
-// this returns the first character of [NSEvent charactersIgnoringModifiers],
-// but when [NSEvent character] has different printable ascii character
-// we may return the first character of [NSEvent characters] instead.
-// (E.g. for dvorak-qwerty layout we want [NSEvent characters] rather than
-// [charactersIgnoringModifiers] for command keys.  Similarly, on german
-// layout we want '{' character rather than '8' for opt-8.)
-unichar KeyCharacterForEvent(NSEvent* event);
+// On macOS, most accelerators are defined in MainMenu.xib and are user
+// configurable. Furthermore, their values and enabled state depends on the key
+// window. Views code relies on a static mapping that is not dependent on the
+// key window. Thus, we provide the default Mac accelerator for each CommandId,
+// which is static. This may be inaccurate, but is at least sufficiently well
+// defined for Views to use.
+bool GetDefaultMacAcceleratorForCommandId(int command_id,
+                                          ui::Accelerator* accelerator);
 
 // For testing purposes.
-const std::vector<KeyboardShortcutData>& GetWindowKeyboardShortcutTable();
-const std::vector<KeyboardShortcutData>&
-GetDelayedWindowKeyboardShortcutTable();
-const std::vector<KeyboardShortcutData>& GetBrowserKeyboardShortcutTable();
+const std::vector<KeyboardShortcutData>& GetShortcutsNotPresentInMainMenu();
 
 #endif  // #ifndef CHROME_BROWSER_GLOBAL_KEYBOARD_SHORTCUTS_MAC_H_

chrome/browser/global_keyboard_shortcuts_mac_browsertest.mm

@@ -14,6 +14,7 @@
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/views_mode_controller.h"
+#include "chrome/test/base/interactive_test_utils.h"
 #import "ui/events/test/cocoa_test_event_utils.h"
 
 using cocoa_test_event_utils::SynthesizeKeyEvent;
@@ -63,13 +64,13 @@ IN_PROC_BROWSER_TEST_F(GlobalKeyboardShortcutsTest, SwitchTabsMac) {
 }
 
 IN_PROC_BROWSER_TEST_F(GlobalKeyboardShortcutsTest, MenuCommandPriority) {
-  // This test doesn't work in Views mode at the moment:
-  // https://crbug.com/845503. Disabled pending a bit of design work to fix it.
-  if (!views_mode_controller::IsViewsBrowserCocoa())
-    return;
   NSWindow* ns_window = browser()->window()->GetNativeWindow();
   TabStripModel* tab_strip = browser()->tab_strip_model();
 
+  // The IDC_SELECT_NEXT_TAB menu item is only enabled when the browser window
+  // is key.
+  ASSERT_TRUE(ui_test_utils::ShowAndFocusNativeWindow(ns_window));
+
   // Set up window with 4 tabs.
   chrome::NewTab(browser());
   chrome::NewTab(browser());
@@ -95,6 +96,7 @@ IN_PROC_BROWSER_TEST_F(GlobalKeyboardShortcutsTest, MenuCommandPriority) {
   ASSERT_NE(nil, next_item);
   [next_item setKeyEquivalent:@"2"];
   [next_item setKeyEquivalentModifierMask:NSCommandKeyMask];
+  ASSERT_TRUE([next_item isEnabled]);
 
   // Send cmd-2 again, and ensure the tab switches.
   ActivateAccelerator(ns_window, SynthesizeKeyEvent(ns_window, true, ui::VKEY_2,

chrome/browser/ui/cocoa/accelerators_cocoa.h

@@ -50,6 +50,14 @@ class AcceleratorsCocoa {
   // Returns the singleton instance.
   static AcceleratorsCocoa* GetInstance();
 
+  // TODO(erikchen): This shouldn't be necessary because ui::Accelerator has
+  // almost the same constructor and contains the same information. Remove this.
+  // https://crbug.com/846893.
+  // Create a cross platform accelerator given a cross platform |key_code| and
+  // the |cocoa_modifiers|.
+  static ui::Accelerator AcceleratorFromKeyCode(ui::KeyboardCode key_code,
+                                                NSUInteger cocoa_modifiers);
+
  private:
   friend struct base::DefaultSingletonTraits<AcceleratorsCocoa>;
   FRIEND_TEST_ALL_PREFIXES(AcceleratorsCocoaBrowserTest,

chrome/browser/ui/cocoa/accelerators_cocoa.mm

@@ -12,6 +12,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/singleton.h"
+#include "base/stl_util.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "printing/buildflags/buildflags.h"
 #import "ui/base/accelerators/platform_accelerator_cocoa.h"
@@ -120,10 +121,12 @@ std::unique_ptr<ui::PlatformAccelerator> PlatformAcceleratorFromKeyCode(
       new ui::PlatformAcceleratorCocoa(key_equivalent, cocoa_modifiers));
 }
 
-// Create a cross platform accelerator given a cross platform |key_code| and
-// the |cocoa_modifiers|.
-ui::Accelerator AcceleratorFromKeyCode(ui::KeyboardCode key_code,
-                                       NSUInteger cocoa_modifiers) {
+}  // namespace
+
+// static
+ui::Accelerator AcceleratorsCocoa::AcceleratorFromKeyCode(
+    ui::KeyboardCode key_code,
+    NSUInteger cocoa_modifiers) {
   int cross_platform_modifiers = ui::EventFlagsFromModifiers(cocoa_modifiers);
   ui::Accelerator accelerator(key_code, cross_platform_modifiers);
 
@@ -133,17 +136,15 @@ ui::Accelerator AcceleratorFromKeyCode(ui::KeyboardCode key_code,
   return accelerator;
 }
 
-}  // namespace
-
 AcceleratorsCocoa::AcceleratorsCocoa() {
-  for (size_t i = 0; i < arraysize(kAcceleratorMap); ++i) {
+  for (size_t i = 0; i < base::size(kAcceleratorMap); ++i) {
     const AcceleratorMapping& entry = kAcceleratorMap[i];
     ui::Accelerator accelerator =
         AcceleratorFromKeyCode(entry.key_code, entry.modifiers);
     accelerators_.insert(std::make_pair(entry.command_id, accelerator));
   }
 
-  for (size_t i = 0; i < arraysize(kAcceleratorList); ++i) {
+  for (size_t i = 0; i < base::size(kAcceleratorList); ++i) {
     const AcceleratorListing& entry = kAcceleratorList[i];
     ui::Accelerator accelerator =
         AcceleratorFromKeyCode(entry.key_code, entry.modifiers);

chrome/browser/ui/cocoa/apps/app_shim_menu_controller_mac_interactive_uitest.mm

@@ -14,6 +14,7 @@
 #include "chrome/browser/apps/app_browsertest_util.h"
 #include "chrome/browser/apps/app_shim/extension_app_shim_handler_mac.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/interactive_test_utils.h"
@@ -48,7 +49,9 @@ class AppShimMenuControllerUITest : public extensions::PlatformAppBrowserTest {
     app1_ = GetFirstAppWindow();
     app2_ = CreateAppWindow(browser()->profile(), extension);
     browser1_ = browser()->window();
-    browser2_ = (new Browser(Browser::CreateParams(profile(), true)))->window();
+    Browser* browser_new = new Browser(Browser::CreateParams(profile(), true));
+    chrome::AddTabAt(browser_new, GURL(), -1, true);
+    browser2_ = browser_new->window();
     browser2_->Show();
 
     // Since a pending key status change on any window could cause the test to

chrome/browser/ui/cocoa/browser_window_cocoa.mm

@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
+#include "base/mac/foundation_util.h"
 #import "base/mac/sdk_forward_declarations.h"
 #include "base/strings/string_util.h"
 #include "base/strings/sys_string_conversions.h"
@@ -14,6 +15,7 @@
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/download/download_shelf.h"
 #include "chrome/browser/extensions/tab_helper.h"
+#include "chrome/browser/global_keyboard_shortcuts_mac.h"
 #include "chrome/browser/metrics/browser_window_histogram_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_window.h"
@@ -24,6 +26,7 @@
 #include "chrome/browser/ui/browser_command_controller.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_dialogs.h"
+#include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window_state.h"
 #include "chrome/browser/ui/cocoa/autofill/save_card_bubble_view_views.h"
@@ -571,48 +574,45 @@ void BrowserWindowCocoa::ShowAppMenu() {
 content::KeyboardEventProcessingResult
 BrowserWindowCocoa::PreHandleKeyboardEvent(
     const NativeWebKeyboardEvent& event) {
+  using Result = content::KeyboardEventProcessingResult;
   // Handle ESC to dismiss permission bubbles, but still forward it
   // to the window afterwards.
   if (event.windows_key_code == ui::VKEY_ESCAPE)
     [controller_ dismissPermissionBubble];
 
   if (![BrowserWindowUtils shouldHandleKeyboardEvent:event])
-    return content::KeyboardEventProcessingResult::NOT_HANDLED;
-
-  if (event.GetType() == blink::WebInputEvent::kRawKeyDown &&
-      [controller_
-          handledByExtensionCommand:event.os_event
-                           priority:ui::AcceleratorManager::kHighPriority])
-    return content::KeyboardEventProcessingResult::HANDLED;
-
-  int id = [BrowserWindowUtils getCommandId:event];
-  if (id == -1)
-    return content::KeyboardEventProcessingResult::NOT_HANDLED;
-
-  if (browser_->command_controller()->IsReservedCommandOrKey(id, event)) {
-    using Result = content::KeyboardEventProcessingResult;
-    return [BrowserWindowUtils handleKeyboardEvent:event.os_event
-                                          inWindow:window()]
-               ? Result::HANDLED
-               : Result::NOT_HANDLED;
-  }
+    return Result::NOT_HANDLED;
 
-  return content::KeyboardEventProcessingResult::NOT_HANDLED_IS_SHORTCUT;
+  int command = CommandForKeyEvent(event.os_event);
+  return command == -1 ? Result::NOT_HANDLED : Result::NOT_HANDLED_IS_SHORTCUT;
 }
 
 void BrowserWindowCocoa::HandleKeyboardEvent(
     const NativeWebKeyboardEvent& event) {
-  if ([BrowserWindowUtils shouldHandleKeyboardEvent:event]) {
-    if (![BrowserWindowUtils handleKeyboardEvent:event.os_event
-                                        inWindow:window()]) {
-
-      // TODO(spqchan): This is a temporary fix for exit extension fullscreen.
-      // A priority system for exiting extension fullscreen when there is a
-      // conflict is being experimented. See Issue 536047.
-      if (event.windows_key_code == ui::VKEY_ESCAPE)
-        [controller_ exitExtensionFullscreenIfPossible];
+  if (![BrowserWindowUtils shouldHandleKeyboardEvent:event])
+    return;
+
+  // TODO(spqchan): This is a temporary fix for exit extension fullscreen.
+  // A priority system for exiting extension fullscreen when there is a
+  // conflict is being experimented. See Issue 536047.
+  if (event.windows_key_code == ui::VKEY_ESCAPE) {
+    [controller_ exitExtensionFullscreenIfPossible];
+
+    // This is a press of an escape key with no modifiers except potentially
+    // shift. This will not be handled by the performKeyEquivalent: path, so
+    // handle it directly here.
+    if (!EventUsesPerformKeyEquivalent(event.os_event)) {
+      int command = IDC_STOP;
+      Browser* browser = chrome::FindBrowserWithWindow(window());
+      if (browser)
+        chrome::ExecuteCommand(browser, command);
+      return;
     }
   }
+
+  ChromeEventProcessingWindow* event_window =
+      base::mac::ObjCCastStrict<ChromeEventProcessingWindow>(window());
+  [event_window redispatchKeyEvent:event.os_event];
 }
 
 void BrowserWindowCocoa::CutCopyPaste(int command_id) {

chrome/browser/ui/cocoa/browser_window_utils.mm

@@ -53,21 +53,6 @@ CGFloat GetPatternVerticalOffsetWithTabStrip(bool tabStripVisible) {
       static_cast<ChromeEventProcessingWindow*>(window);
   DCHECK([event_window isKindOfClass:[ChromeEventProcessingWindow class]]);
 
-  // Do not fire shortcuts on key up.
-  if ([event type] == NSKeyDown) {
-    // Send the event to the menu before sending it to the browser/window
-    // shortcut handling, so that if a user configures cmd-left to mean
-    // "previous tab", it takes precedence over the built-in "history back"
-    // binding. Other than that, the |-redispatchKeyEvent:| call would take care
-    // of invoking the original menu item shortcut as well.
-
-    if ([[NSApp mainMenu] performKeyEquivalent:event])
-      return true;
-
-    if ([event_window handleExtraKeyboardShortcut:event])
-      return true;
-  }
-
   return [event_window redispatchKeyEvent:event];
 }
 

chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.mm

@@ -5,52 +5,20 @@
 #import "chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h"
 
 #include "base/logging.h"
+#include "chrome/browser/extensions/global_shortcut_listener.h"
 #include "chrome/browser/global_keyboard_shortcuts_mac.h"
+#include "chrome/browser/ui/browser_command_controller.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_finder.h"
 #import "chrome/browser/ui/cocoa/browser_window_controller_private.h"
 #import "chrome/browser/ui/cocoa/browser_window_views_mac.h"
 #import "chrome/browser/ui/cocoa/tabs/tab_strip_controller.h"
+#include "content/public/browser/native_web_keyboard_event.h"
 
-namespace {
-
-// Type of functions listed in global_keyboard_shortcuts_mac.h.
-typedef int (*KeyToCommandMapper)(bool, bool, bool, bool, int, unichar);
-
-// Returns the command that would be executed if |window| received |event|
-// according to |command_for_keyboard_shortcut|, or -1 if no command would be
-// executed.
-int CommandForExtraKeyboardShortcut(
-    NSEvent* event,
-    NSWindow* window,
-    KeyToCommandMapper command_for_keyboard_shortcut) {
-  // Extract info from |event|.
-  NSUInteger modifers = [event modifierFlags];
-  const bool command = modifers & NSCommandKeyMask;
-  const bool shift = modifers & NSShiftKeyMask;
-  const bool control = modifers & NSControlKeyMask;
-  const bool option = modifers & NSAlternateKeyMask;
-  const int key_code = [event keyCode];
-  const unichar key_char = KeyCharacterForEvent(event);
-
-  int cmd = command_for_keyboard_shortcut(command, shift, control, option,
-                                          key_code, key_char);
-
-  // Non-browser windows don't execute any commands.
-  if (!chrome::FindBrowserWithWindow(window))
-    return -1;
-
-  return cmd;
-}
+@implementation ChromeCommandDispatcherDelegate
 
-// If the event is for a Browser window, and the key combination has an
-// associated command, execute the command.
-bool HandleExtraKeyboardShortcut(
-    NSEvent* event,
-    NSWindow* window,
-    KeyToCommandMapper command_for_keyboard_shortcut) {
-  int cmd = CommandForExtraKeyboardShortcut(event, window,
-                                            command_for_keyboard_shortcut);
+- (BOOL)handleExtraKeyboardShortcut:(NSEvent*)event window:(NSWindow*)window {
+  int cmd = CommandForKeyEvent(event);
   if (cmd == -1)
     return false;
 
@@ -58,37 +26,9 @@ bool HandleExtraKeyboardShortcut(
   return true;
 }
 
-bool HandleExtraWindowKeyboardShortcut(NSEvent* event, NSWindow* window) {
-  return HandleExtraKeyboardShortcut(event, window,
-                                     CommandForWindowKeyboardShortcut);
-}
-
-bool HandleDelayedWindowKeyboardShortcut(NSEvent* event, NSWindow* window) {
-  return HandleExtraKeyboardShortcut(event, window,
-                                     CommandForDelayedWindowKeyboardShortcut);
-}
-
-bool HandleExtraBrowserKeyboardShortcut(NSEvent* event, NSWindow* window) {
-  return HandleExtraKeyboardShortcut(event, window,
-                                     CommandForBrowserKeyboardShortcut);
-}
-
-}  // namespace
-
-@implementation ChromeCommandDispatcherDelegate
-
-- (BOOL)handleExtraKeyboardShortcut:(NSEvent*)event window:(NSWindow*)window {
-  return HandleExtraBrowserKeyboardShortcut(event, window) ||
-         HandleExtraWindowKeyboardShortcut(event, window) ||
-         HandleDelayedWindowKeyboardShortcut(event, window);
-}
-
 - (BOOL)eventHandledByExtensionCommand:(NSEvent*)event
-                          isRedispatch:(BOOL)isRedispatch {
-  // Some extension commands have higher priority than web content, and some
-  // have lower priority. Regardless of whether the event is being redispatched,
-  // let the extension system try to handle the event. In case this is a
-  // redispatched event, [event window] gives the correct window.
+                              priority:(ui::AcceleratorManager::HandlerPriority)
+                                           priority {
   if ([event window]) {
     BrowserWindowController* controller =
         BrowserWindowControllerForWindow([event window]);
@@ -96,9 +36,6 @@ bool HandleExtraBrowserKeyboardShortcut(NSEvent* event, NSWindow* window) {
     // are handled by BrowserView.
     if ([controller respondsToSelector:@selector(handledByExtensionCommand:
                                                                   priority:)]) {
-      ui::AcceleratorManager::HandlerPriority priority =
-          isRedispatch ? ui::AcceleratorManager::kNormalPriority
-                       : ui::AcceleratorManager::kHighPriority;
       if ([controller handledByExtensionCommand:event priority:priority])
         return YES;
     }
@@ -107,34 +44,79 @@ bool HandleExtraBrowserKeyboardShortcut(NSEvent* event, NSWindow* window) {
 }
 
 - (BOOL)prePerformKeyEquivalent:(NSEvent*)event window:(NSWindow*)window {
-  // If a command has a menu key equivalent that *replaces* one of the window
-  // keyboard shortcuts, the menu key equivalent needs to be executed, because
-  // these are user-addded keyboard shortcuts that replace builtin shortcuts.
+  // TODO(erikchen): Detect symbolic hot keys, and force control to be passed
+  // back to AppKit so that it can handle it correctly.
+  // https://crbug.com/846893.
+
+  NSResponder* responder = [window firstResponder];
+  if ([responder conformsToProtocol:@protocol(CommandDispatcherTarget)]) {
+    NSObject<CommandDispatcherTarget>* target =
+        static_cast<NSObject<CommandDispatcherTarget>*>(responder);
+    if ([target isKeyLocked:event])
+      return NO;
+  }
+
+  if ([self eventHandledByExtensionCommand:event
+                                  priority:ui::AcceleratorManager::
+                                               kHighPriority]) {
+    return YES;
+  }
+
+  // The specification for this private extensions API is incredibly vague. For
+  // now, we avoid triggering chrome commands prior to giving the firstResponder
+  // a chance to handle the event.
+  if (extensions::GlobalShortcutListener::GetInstance()
+          ->IsShortcutHandlingSuspended()) {
+    return NO;
+  }
+
+  // If this keyEquivalent corresponds to a Chrome command, trigger it directly
+  // via chrome::ExecuteCommand. We avoid going through the NSMenu for two
+  // reasons:
+  //  * consistency - some commands are not present in the NSMenu. Furthermore,
+  //  the NSMenu's contents can be dynamically updated, so there's no guarantee
+  //  that passing the event to NSMenu will even do what we think it will do.
+  //  * Avoiding sleeps. By default, the implementation of NSMenu
+  //  performKeyEquivalent: has a nested run loop that spins for 100ms. If we
+  //  avoid that by spinning our task runner in their private mode, there's a
+  //  built in nanosleep. See https://crbug.com/836947#c8.
+  //
+  // By not passing the event to AppKit, we do lose out on the brief
+  // highlighting of the NSMenu.
   //
-  // If a command has a menu key equivalent that does *not* replace a window
-  // keyboard shortcut, it will be handled later; only window shortcuts need
-  // special handling here since they happen before normal command dispatch.
-  int cmd = MenuCommandForKeyEvent(event);
+  // TODO(erikchen): Add a throttle. Otherwise, it's possible for a user holding
+  // down a hotkey [e.g. cmd + w] to accidentally close too many tabs!
+  // https://crbug.com/846893.
+  int cmd = CommandForKeyEvent(event);
   if (cmd != -1) {
-    int keyCmd = CommandForExtraKeyboardShortcut(
-        event, window, CommandForWindowKeyboardShortcut);
     Browser* browser = chrome::FindBrowserWithWindow(window);
-    if (keyCmd != -1 && browser) {
+    if (browser && browser->command_controller()->IsReservedCommandOrKey(
+                       cmd, content::NativeWebKeyboardEvent(event))) {
       chrome::ExecuteCommand(browser, cmd);
       return YES;
     }
   }
 
-  // Handle per-window shortcuts like cmd-1, but do not handle browser-level
-  // shortcuts like cmd-left (else, cmd-left would do history navigation even
-  // if e.g. the Omnibox has focus).
-  return HandleExtraWindowKeyboardShortcut(event, window);
+  return NO;
 }
 
 - (BOOL)postPerformKeyEquivalent:(NSEvent*)event window:(NSWindow*)window {
-  // Handle per-window shortcuts like Esc after giving everybody else a chance
-  // to handle them
-  return HandleDelayedWindowKeyboardShortcut(event, window);
+  if ([self eventHandledByExtensionCommand:event
+                                  priority:ui::AcceleratorManager::
+                                               kNormalPriority]) {
+    return true;
+  }
+
+  int cmd = CommandForKeyEvent(event);
+  if (cmd != -1) {
+    Browser* browser = chrome::FindBrowserWithWindow(window);
+    if (browser) {
+      chrome::ExecuteCommand(browser, cmd);
+      return true;
+    }
+  }
+
+  return false;
 }
 
 @end  // ChromeCommandDispatchDelegate

chrome/browser/ui/views/accelerator_table.cc

@@ -20,10 +20,16 @@ namespace {
 // ../../app/chrome_dll.rc.
 // Do not use Ctrl-Alt as a shortcut modifier, as it is used by i18n keyboards:
 // http://blogs.msdn.com/b/oldnewthing/archive/2004/03/29/101121.aspx
-// For many commands, the Mac equivalent uses Cmd instead of Ctrl. We only need
-// to list the ones that do not have a key equivalent in the main menu, i.e.
-// only the ones in global_keyboard_shortcuts_mac.mm.
 const AcceleratorMapping kAcceleratorMap[] = {
+    // To add an accelerator to macOS that uses modifier keys, either:
+    //   1) Update MainMenu.xib to include a new menu item with the appropriate
+    //      modifier.
+    //   2) Update GetShortcutsNotPresentInMainMenu() in
+    //      global_keyboard_shortcuts_mac.mm.
+    {ui::VKEY_F12, ui::EF_NONE, IDC_DEV_TOOLS_TOGGLE},
+    {ui::VKEY_ESCAPE, ui::EF_NONE, IDC_STOP},
+
+#if !defined(OS_MACOSX)
     {ui::VKEY_D, ui::EF_PLATFORM_ACCELERATOR, IDC_BOOKMARK_PAGE},
     {ui::VKEY_D, ui::EF_SHIFT_DOWN | ui::EF_PLATFORM_ACCELERATOR,
      IDC_BOOKMARK_ALL_TABS},
@@ -35,7 +41,6 @@ const AcceleratorMapping kAcceleratorMap[] = {
     {ui::VKEY_G, ui::EF_SHIFT_DOWN | ui::EF_PLATFORM_ACCELERATOR,
      IDC_FIND_PREVIOUS},
     {ui::VKEY_L, ui::EF_PLATFORM_ACCELERATOR, IDC_FOCUS_LOCATION},
-    {ui::VKEY_F12, ui::EF_NONE, IDC_DEV_TOOLS_TOGGLE},
     {ui::VKEY_O, ui::EF_PLATFORM_ACCELERATOR, IDC_OPEN_FILE},
     {ui::VKEY_P, ui::EF_PLATFORM_ACCELERATOR, IDC_PRINT},
     {ui::VKEY_R, ui::EF_PLATFORM_ACCELERATOR, IDC_RELOAD},
@@ -95,7 +100,6 @@ const AcceleratorMapping kAcceleratorMap[] = {
 #endif  // OS_LINUX && !OS_CHROMEOS
     {ui::VKEY_B, ui::EF_SHIFT_DOWN | ui::EF_PLATFORM_ACCELERATOR,
      IDC_SHOW_BOOKMARK_BAR},
-    {ui::VKEY_ESCAPE, ui::EF_NONE, IDC_STOP},
     {ui::VKEY_OEM_MINUS, ui::EF_PLATFORM_ACCELERATOR, IDC_ZOOM_MINUS},
     {ui::VKEY_SUBTRACT, ui::EF_PLATFORM_ACCELERATOR, IDC_ZOOM_MINUS},
     {ui::VKEY_0, ui::EF_PLATFORM_ACCELERATOR, IDC_ZOOM_NORMAL},
@@ -103,7 +107,6 @@ const AcceleratorMapping kAcceleratorMap[] = {
     {ui::VKEY_OEM_PLUS, ui::EF_PLATFORM_ACCELERATOR, IDC_ZOOM_PLUS},
     {ui::VKEY_ADD, ui::EF_PLATFORM_ACCELERATOR, IDC_ZOOM_PLUS},
 
-#if !defined(OS_MACOSX)  // Function keys aren't mapped on Mac.
     {ui::VKEY_F1, ui::EF_NONE, IDC_HELP_PAGE_VIA_KEYBOARD},
     {ui::VKEY_F3, ui::EF_NONE, IDC_FIND_NEXT},
     {ui::VKEY_F3, ui::EF_SHIFT_DOWN, IDC_FIND_PREVIOUS},
@@ -116,7 +119,6 @@ const AcceleratorMapping kAcceleratorMap[] = {
     {ui::VKEY_F6, ui::EF_SHIFT_DOWN, IDC_FOCUS_PREVIOUS_PANE},
     {ui::VKEY_F10, ui::EF_NONE, IDC_FOCUS_MENU_BAR},
     {ui::VKEY_F11, ui::EF_NONE, IDC_FULLSCREEN},
-#endif  // !OS_MACOSX
 
   // Platform-specific key maps.
 #if defined(OS_LINUX)
@@ -149,9 +151,7 @@ const AcceleratorMapping kAcceleratorMap[] = {
     {ui::VKEY_BROWSER_SEARCH, ui::EF_NONE, IDC_FOCUS_SEARCH},
     {ui::VKEY_M, ui::EF_SHIFT_DOWN | ui::EF_PLATFORM_ACCELERATOR,
      IDC_SHOW_AVATAR_MENU},
-#if !defined(OS_MACOSX)
     {ui::VKEY_Q, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_EXIT},
-#endif  // !OS_MACOSX
 #endif  // !OS_CHROMEOS
 
 #if defined(GOOGLE_CHROME_BUILD) && !defined(OS_MACOSX)
@@ -164,51 +164,6 @@ const AcceleratorMapping kAcceleratorMap[] = {
     {ui::VKEY_T, ui::EF_SHIFT_DOWN | ui::EF_PLATFORM_ACCELERATOR,
      IDC_RESTORE_TAB},
 
-#if defined(OS_MACOSX)
-    // VKEY_OEM_4 is Left Brace '[{' key.
-    {ui::VKEY_OEM_4, ui::EF_COMMAND_DOWN, IDC_BACK},
-    {ui::VKEY_LEFT, ui::EF_COMMAND_DOWN, IDC_BACK},
-#if BUILDFLAG(ENABLE_PRINTING)
-    {ui::VKEY_P, ui::EF_COMMAND_DOWN | ui::EF_ALT_DOWN, IDC_BASIC_PRINT},
-#endif  // ENABLE_PRINTING
-    {ui::VKEY_BACK, ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN,
-     IDC_CLEAR_BROWSING_DATA},
-    {ui::VKEY_V, ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN,
-     IDC_CONTENT_CONTEXT_PASTE_AND_MATCH_STYLE},
-    {ui::VKEY_Z, ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN,
-     IDC_CONTENT_CONTEXT_REDO},
-    {ui::VKEY_A, ui::EF_COMMAND_DOWN, IDC_CONTENT_CONTEXT_SELECTALL},
-    {ui::VKEY_Z, ui::EF_COMMAND_DOWN, IDC_CONTENT_CONTEXT_UNDO},
-    {ui::VKEY_I, ui::EF_COMMAND_DOWN | ui::EF_ALT_DOWN, IDC_DEV_TOOLS},
-    {ui::VKEY_J, ui::EF_COMMAND_DOWN | ui::EF_ALT_DOWN, IDC_DEV_TOOLS_CONSOLE},
-    {ui::VKEY_C, ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN,
-     IDC_DEV_TOOLS_INSPECT},
-    {ui::VKEY_I, ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN,
-     IDC_EMAIL_PAGE_LOCATION},
-    {ui::VKEY_Q, ui::EF_COMMAND_DOWN, IDC_EXIT},
-    {ui::VKEY_F, ui::EF_COMMAND_DOWN | ui::EF_ALT_DOWN, IDC_FOCUS_SEARCH},
-    // VKEY_OEM_6 is Right Brace ']}' key.
-    {ui::VKEY_OEM_6, ui::EF_COMMAND_DOWN, IDC_FORWARD},
-    {ui::VKEY_RIGHT, ui::EF_COMMAND_DOWN, IDC_FORWARD},
-    {ui::VKEY_F, ui::EF_COMMAND_DOWN | ui::EF_CONTROL_DOWN, IDC_FULLSCREEN},
-    // VKEY_OEM_2 is Slash '/?' key.
-    {ui::VKEY_OEM_2, ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN,
-     IDC_HELP_PAGE_VIA_MENU},
-    {ui::VKEY_H, ui::EF_COMMAND_DOWN, IDC_HIDE_APP},
-    {ui::VKEY_H, ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN, IDC_HOME},
-    {ui::VKEY_M, ui::EF_COMMAND_DOWN, IDC_MINIMIZE_WINDOW},
-    {ui::VKEY_RIGHT, ui::EF_COMMAND_DOWN | ui::EF_ALT_DOWN,
-     IDC_SELECT_NEXT_TAB},
-    {ui::VKEY_LEFT, ui::EF_COMMAND_DOWN | ui::EF_ALT_DOWN,
-     IDC_SELECT_PREVIOUS_TAB},
-    {ui::VKEY_B, ui::EF_COMMAND_DOWN | ui::EF_ALT_DOWN,
-     IDC_SHOW_BOOKMARK_MANAGER},
-    {ui::VKEY_J, ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN, IDC_SHOW_DOWNLOADS},
-    {ui::VKEY_L, ui::EF_COMMAND_DOWN | ui::EF_ALT_DOWN, IDC_SHOW_DOWNLOADS},
-    {ui::VKEY_Y, ui::EF_COMMAND_DOWN, IDC_SHOW_HISTORY},
-    {ui::VKEY_OEM_PERIOD, ui::EF_COMMAND_DOWN, IDC_STOP},
-    {ui::VKEY_U, ui::EF_COMMAND_DOWN | ui::EF_ALT_DOWN, IDC_VIEW_SOURCE},
-#else  // !OS_MACOSX
     // Alt by itself (or with just shift) is never used on Mac since it's used
     // to generate non-ASCII characters. Such commands are given Mac-specific
     // bindings as well. Mapping with just Alt appear here, and should have an
@@ -274,6 +229,13 @@ std::vector<AcceleratorMapping> GetAcceleratorList() {
 
 bool GetStandardAcceleratorForCommandId(int command_id,
                                         ui::Accelerator* accelerator) {
+#if defined(OS_MACOSX)
+  // On macOS, the cut/copy/paste accelerators are defined in MainMenu.xib and
+  // the accelerator is user configurable. All of this is handled by
+  // CommandDispatcher.
+  NOTREACHED();
+  return false;
+#else
   // The standard Ctrl-X, Ctrl-V and Ctrl-C are not defined as accelerators
   // anywhere else.
   switch (command_id) {
@@ -288,6 +250,7 @@ bool GetStandardAcceleratorForCommandId(int command_id,
       return true;
   }
   return false;
+#endif
 }
 
 bool IsCommandRepeatable(int command_id) {

chrome/browser/ui/views/accelerator_table.h

@@ -23,6 +23,15 @@ struct AcceleratorMapping {
 
 // Returns a list of accelerator mapping information for accelerators
 // handled by Chrome but excluding accelerators handled by Ash.
+//
+// On macOS, most accelerators are present in the main menu. The mapping from
+// vkey -> command is modifiable by the user at any point in time. As such, it
+// doesn't make sense to have a static mapping. Furthermore, on macOS, all
+// accelerators that use a modifier key are handled much earlier during event
+// processing.
+//
+// On macOS (only), the result will only contain accelerators that do not use a
+// modifier key (e.g. escape).
 CHROME_VIEWS_EXPORT std::vector<AcceleratorMapping> GetAcceleratorList();
 
 // Returns true on Ash and if the command id has an associated accelerator which

chrome/browser/ui/views/accelerator_table_unittest_mac.mm

@@ -54,63 +54,21 @@ void VerifyTableDoesntHaveDuplicates(
 
 }  // namespace
 
-// Vefifies that only the whitelisted accelerators could have Control key
-// modifier, while running on macOS.
-TEST(AcceleratorTableTest, CheckMacOSControlAccelerators) {
-  // Only the accelerators that also work in Cocoa browser are allowed to appear
-  // on this whitelist.
-  const std::set<int> whitelisted_control_shortcuts = {
-      IDC_SELECT_NEXT_TAB,
-      IDC_SELECT_PREVIOUS_TAB,
-      IDC_FULLSCREEN,
-  };
-
-  const std::vector<AcceleratorMapping> accelerators(GetAcceleratorList());
-
-  // Control modifier is rarely used on Mac, and all valid uses must be
-  // whitelisted.
-  for (const auto& entry : accelerators) {
-    if (base::ContainsKey(whitelisted_control_shortcuts, entry.command_id))
-      continue;
-    EXPECT_FALSE(entry.modifiers & ui::EF_CONTROL_DOWN)
-        << "Found non-whitelisted accelerator that contains Control "
-           "modifier: " << entry.command_id;
-  }
-
-  // Test that whitelist is not outdated.
-  for (const auto& whitelist_entry : whitelisted_control_shortcuts) {
-    const auto entry =
-        std::find_if(accelerators.begin(), accelerators.end(),
-                     [whitelist_entry](const AcceleratorMapping& a) {
-          return a.command_id == whitelist_entry &&
-                 (a.modifiers & ui::EF_CONTROL_DOWN) != 0;
-        });
-    EXPECT_NE(entry, accelerators.end())
-        << "Whitelisted accelerator not found in the actual list: "
-        << whitelist_entry;
-  }
-}
-
-// Verifies that Alt-only (or with just Shift) accelerators are not present in
-// the list.
-TEST(AcceleratorTableTest, CheckMacOSAltAccelerators) {
-  const int kNonShiftMask =
-      ui::EF_COMMAND_DOWN | ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN;
-  for (const auto& entry : GetAcceleratorList()) {
-    EXPECT_FALSE((entry.modifiers & kNonShiftMask) == ui::EF_ALT_DOWN)
-        << "Found accelerator that uses solely Alt modifier: "
-        << entry.command_id;
-  }
+// On macOS, accelerator handling is done by CommandDispatcher. The only
+// accelerators allowed to appear in AcceleratorTable are those that don't
+// have any modifiers, and thus cannot be interpreted as macOS
+// keyEquivalents.
+TEST(AcceleratorTableTest, CheckMacOSAccelerators) {
+  for (const auto& entry : GetAcceleratorList())
+    EXPECT_EQ(0, entry.modifiers);
 }
 
 // Verifies that we're not processing any duplicate accelerators in
-// global_keyboard_shortcuts_mac.mm functions.
+// global_keyboard_shortcuts_mac.mm functions. Note that the bulk of
+// accelerators are defined in MainMenu.xib. We do not check that there is no
+// overlap with that.
 TEST(AcceleratorTableTest, CheckNoDuplicatesGlobalKeyboardShortcutsMac) {
   test::ScopedMacViewsBrowserMode views_mode_{true};
-  VerifyTableDoesntHaveDuplicates(GetWindowKeyboardShortcutTable(),
-                                  "WindowKeyboardShortcutTable");
-  VerifyTableDoesntHaveDuplicates(GetDelayedWindowKeyboardShortcutTable(),
-                                  "DelayedWindowKeyboardShortcutTable");
-  VerifyTableDoesntHaveDuplicates(GetBrowserKeyboardShortcutTable(),
-                                  "BrowserKeyboardShortcutTable");
+  VerifyTableDoesntHaveDuplicates(GetShortcutsNotPresentInMainMenu(),
+                                  "ShortcutsNotPresentInMainMenu");
 }

chrome/browser/ui/views/frame/browser_frame.cc

@@ -134,7 +134,7 @@ void BrowserFrame::GetWindowPlacement(gfx::Rect* bounds,
   return native_browser_frame_->GetWindowPlacement(bounds, show_state);
 }
 
-bool BrowserFrame::PreHandleKeyboardEvent(
+content::KeyboardEventProcessingResult BrowserFrame::PreHandleKeyboardEvent(
     const content::NativeWebKeyboardEvent& event) {
   return native_browser_frame_->PreHandleKeyboardEvent(event);
 }

chrome/browser/ui/views/frame/browser_frame.h

@@ -10,6 +10,7 @@
 #include "base/macros.h"
 #include "build/build_config.h"
 #include "chrome/browser/ui/views/frame/browser_non_client_frame_view.h"
+#include "content/public/browser/keyboard_event_processing_result.h"
 #include "ui/views/context_menu_controller.h"
 #include "ui/views/widget/widget.h"
 
@@ -84,10 +85,13 @@ class BrowserFrame
   void GetWindowPlacement(gfx::Rect* bounds,
                           ui::WindowShowState* show_state) const;
 
-  // Returns true if the |event| was handled by the platform implementation
+  // Returns HANDLED if the |event| was handled by the platform implementation
   // before sending it to the renderer. E.g., it may be swallowed by a native
-  // menu bar.
-  bool PreHandleKeyboardEvent(const content::NativeWebKeyboardEvent& event);
+  // menu bar. Returns NOT_HANDLED_IS_SHORTCUT if the event was not handled, but
+  // would be handled as a shortcut if the renderer chooses not to handle it.
+  // Otherwise returns NOT_HANDLED.
+  content::KeyboardEventProcessingResult PreHandleKeyboardEvent(
+      const content::NativeWebKeyboardEvent& event);
 
   // Returns true if the |event| was handled by the platform implementation,
   // if the renderer did not process it.

chrome/browser/ui/views/frame/browser_frame_ash.cc

@@ -128,9 +128,9 @@ void BrowserFrameAsh::GetWindowPlacement(
   }
 }
 
-bool BrowserFrameAsh::PreHandleKeyboardEvent(
+content::KeyboardEventProcessingResult BrowserFrameAsh::PreHandleKeyboardEvent(
     const content::NativeWebKeyboardEvent& event) {
-  return false;
+  return content::KeyboardEventProcessingResult::NOT_HANDLED;
 }
 
 bool BrowserFrameAsh::HandleKeyboardEvent(

chrome/browser/ui/views/frame/browser_frame_ash.h

@@ -38,7 +38,7 @@ class BrowserFrameAsh : public views::NativeWidgetAura,
   bool ShouldSaveWindowPlacement() const override;
   void GetWindowPlacement(gfx::Rect* bounds,
                           ui::WindowShowState* show_state) const override;
-  bool PreHandleKeyboardEvent(
+  content::KeyboardEventProcessingResult PreHandleKeyboardEvent(
       const content::NativeWebKeyboardEvent& event) override;
   bool HandleKeyboardEvent(
       const content::NativeWebKeyboardEvent& event) override;

chrome/browser/ui/views/frame/browser_frame_mac.h

@@ -35,7 +35,7 @@ class BrowserFrameMac : public views::NativeWidgetMac,
   bool ShouldSaveWindowPlacement() const override;
   void GetWindowPlacement(gfx::Rect* bounds,
                           ui::WindowShowState* show_state) const override;
-  bool PreHandleKeyboardEvent(
+  content::KeyboardEventProcessingResult PreHandleKeyboardEvent(
       const content::NativeWebKeyboardEvent& event) override;
   bool HandleKeyboardEvent(
       const content::NativeWebKeyboardEvent& event) override;

chrome/browser/ui/views/frame/browser_frame_mac.mm

@@ -36,24 +36,6 @@ bool ShouldHandleKeyboardEvent(const content::NativeWebKeyboardEvent& event) {
   return [event.os_event type] == NSKeyDown;
 }
 
-// Returns true if |event| was handled.
-bool HandleExtraKeyboardShortcut(NSEvent* event,
-                                 Browser* browser,
-                                 ChromeCommandDispatcherDelegate* delegate) {
-  // Send the event to the menu before sending it to the browser/window
-  // shortcut handling, so that if a user configures cmd-left to mean
-  // "previous tab", it takes precedence over the built-in "history back"
-  // binding.
-  if ([[NSApp mainMenu] performKeyEquivalent:event])
-    return true;
-
-  // Invoke ChromeCommandDispatcherDelegate for Mac-specific shortcuts that
-  // can't be handled by accelerator_table.cc.
-  return [delegate
-      handleExtraKeyboardShortcut:event
-                           window:browser->window()->GetNativeWindow()];
-}
-
 }  // namespace
 
 BrowserFrameMac::BrowserFrameMac(BrowserFrame* browser_frame,
@@ -158,30 +140,19 @@ void BrowserFrameMac::GetWindowPlacement(
   return NativeWidgetMac::GetWindowPlacement(bounds, show_state);
 }
 
-// Mac is special because the user could override the menu shortcuts (see
-// comment in HandleExtraKeyboardShortcut), and there's a set of additional
-// accelerator tables (handled by ChromeCommandDispatcherDelegate) that couldn't
-// be ported to accelerator_table.cc: see global_keyboard_shortcuts_views_mac.mm
-bool BrowserFrameMac::PreHandleKeyboardEvent(
+content::KeyboardEventProcessingResult BrowserFrameMac::PreHandleKeyboardEvent(
     const content::NativeWebKeyboardEvent& event) {
-  if (!ShouldHandleKeyboardEvent(event))
-    return false;
-
-  // CommandForKeyEvent consults the [NSApp mainMenu] and Mac-specific
-  // accelerator tables internally.
-  int command_id = CommandForKeyEvent(event.os_event);
-  if (command_id == -1)
-    return false;
-
-  // Only handle a small list of reserved commands that we don't want to be
-  // handled by the renderer.
-  Browser* browser = browser_view_->browser();
-  if (!browser->command_controller()->IsReservedCommandOrKey(
-          command_id, event))
-    return false;
+  // On macOS, all keyEquivalents that use modifier keys are handled by
+  // -[CommandDispatcher performKeyEquivalent:]. If this logic is being hit,
+  // it means that the event was not handled, so we must return either
+  // NOT_HANDLED or NOT_HANDLED_IS_SHORTCUT.
+  if (EventUsesPerformKeyEquivalent(event.os_event)) {
+    int command_id = CommandForKeyEvent(event.os_event);
+    if (command_id != -1)
+      return content::KeyboardEventProcessingResult::NOT_HANDLED_IS_SHORTCUT;
+  }
 
-  return HandleExtraKeyboardShortcut(event.os_event, browser,
-                                     command_dispatcher_delegate_);
+  return content::KeyboardEventProcessingResult::NOT_HANDLED;
 }
 
 bool BrowserFrameMac::HandleKeyboardEvent(
@@ -189,6 +160,11 @@ bool BrowserFrameMac::HandleKeyboardEvent(
   if (!ShouldHandleKeyboardEvent(event))
     return false;
 
-  return HandleExtraKeyboardShortcut(event.os_event, browser_view_->browser(),
-                                     command_dispatcher_delegate_);
+  // Redispatch the event. If it's a keyEquivalent:, this gives
+  // CommandDispatcher the opportunity to finish passing the event to consumers.
+  NSWindow* window = GetNativeWindow();
+  DCHECK([window.class conformsToProtocol:@protocol(CommandDispatchingWindow)]);
+  NSObject<CommandDispatchingWindow>* command_dispatching_window =
+      base::mac::ObjCCastStrict<NSObject<CommandDispatchingWindow>>(window);
+  return [command_dispatching_window redispatchKeyEvent:event.os_event];
 }

chrome/browser/ui/views/frame/browser_frame_mash.cc

@@ -134,9 +134,9 @@ void BrowserFrameMash::GetWindowPlacement(
   }
 }
 
-bool BrowserFrameMash::PreHandleKeyboardEvent(
+content::KeyboardEventProcessingResult BrowserFrameMash::PreHandleKeyboardEvent(
     const content::NativeWebKeyboardEvent& event) {
-  return false;
+  return content::KeyboardEventProcessingResult::NOT_HANDLED;
 }
 
 bool BrowserFrameMash::HandleKeyboardEvent(

chrome/browser/ui/views/frame/browser_frame_mash.h

@@ -27,7 +27,7 @@ class BrowserFrameMash : public views::DesktopNativeWidgetAura,
   bool ShouldSaveWindowPlacement() const override;
   void GetWindowPlacement(gfx::Rect* bounds,
                           ui::WindowShowState* show_state) const override;
-  bool PreHandleKeyboardEvent(
+  content::KeyboardEventProcessingResult PreHandleKeyboardEvent(
       const content::NativeWebKeyboardEvent& event) override;
   bool HandleKeyboardEvent(
       const content::NativeWebKeyboardEvent& event) override;

chrome/browser/ui/views/frame/browser_view.cc

@@ -159,6 +159,7 @@
 #endif  // !defined(OS_CHROMEOS)
 
 #if defined(OS_MACOSX)
+#include "chrome/browser/global_keyboard_shortcuts_mac.h"
 #include "chrome/browser/ui/views/frame/browser_view_commands_mac.h"
 #endif
 
@@ -557,10 +558,21 @@ bool BrowserView::HasClientEdge() const {
 
 bool BrowserView::GetAccelerator(int cmd_id,
                                  ui::Accelerator* accelerator) const {
+#if defined(OS_MACOSX)
+  // On macOS, most accelerators are defined in MainMenu.xib and are user
+  // configurable. Furthermore, their values and enabled state depends on the
+  // key window. Views code relies on a static mapping that is not dependent on
+  // the key window. Thus, we provide the default Mac accelerator for each
+  // CommandId, which is static. This may be inaccurate, but is at least
+  // sufficiently well defined for Views to use.
+  if (GetDefaultMacAcceleratorForCommandId(cmd_id, accelerator))
+    return true;
+#else
   // We retrieve the accelerator information for standard accelerators
   // for cut, copy and paste.
   if (GetStandardAcceleratorForCommandId(cmd_id, accelerator))
     return true;
+#endif
   // Else, we retrieve the accelerator information from the accelerator table.
   for (std::map<ui::Accelerator, int>::const_iterator it =
            accelerator_table_.begin(); it != accelerator_table_.end(); ++it) {
@@ -1356,8 +1368,10 @@ content::KeyboardEventProcessingResult BrowserView::PreHandleKeyboardEvent(
   }
 #endif  // defined(OS_CHROMEOS)
 
-  if (frame_->PreHandleKeyboardEvent(event))
-    return content::KeyboardEventProcessingResult::HANDLED;
+  content::KeyboardEventProcessingResult result =
+      frame_->PreHandleKeyboardEvent(event);
+  if (result != content::KeyboardEventProcessingResult::NOT_HANDLED)
+    return result;
 
 #if defined(OS_CHROMEOS)
   if (event.os_event && event.os_event->IsKeyEvent() &&

chrome/browser/ui/views/frame/browser_view_unittest.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/views/frame/browser_view.h"
 
 #include "base/macros.h"
+#include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/layout_constants.h"
@@ -178,6 +179,8 @@ TEST_F(BrowserViewTest, BrowserViewLayout) {
   BookmarkBarView::DisableAnimationsForTesting(false);
 }
 
+// On macOS, most accelerators are handled by CommandDispatcher.
+#if !defined(OS_MACOSX)
 // Test that repeated accelerators are processed or ignored depending on the
 // commands that they refer to. The behavior for different commands is dictated
 // by IsCommandRepeatable() in chrome/browser/ui/views/accelerator_table.h.
@@ -196,6 +199,7 @@ TEST_F(BrowserViewTest, RepeatedAccelerators) {
       ui::VKEY_TAB, ui::EF_CONTROL_DOWN | ui::EF_IS_REPEAT);
   EXPECT_TRUE(browser_view()->AcceleratorPressed(kNextTabRepeatAccel));
 }
+#endif  // !defined(OS_MACOSX)
 
 // Test that bookmark bar view becomes invisible when closing the browser.
 TEST_F(BrowserViewTest, BookmarkBarInvisibleOnShutdown) {

chrome/browser/ui/views/frame/desktop_browser_frame_aura.cc

@@ -106,9 +106,10 @@ void DesktopBrowserFrameAura::GetWindowPlacement(
     *show_state = ui::SHOW_STATE_NORMAL;
 }
 
-bool DesktopBrowserFrameAura::PreHandleKeyboardEvent(
+content::KeyboardEventProcessingResult
+DesktopBrowserFrameAura::PreHandleKeyboardEvent(
     const content::NativeWebKeyboardEvent& event) {
-  return false;
+  return content::KeyboardEventProcessingResult::NOT_HANDLED;
 }
 
 bool DesktopBrowserFrameAura::HandleKeyboardEvent(

chrome/browser/ui/views/frame/desktop_browser_frame_aura.h

@@ -50,7 +50,7 @@ class DesktopBrowserFrameAura : public views::DesktopNativeWidgetAura,
   bool ShouldSaveWindowPlacement() const override;
   void GetWindowPlacement(gfx::Rect* bounds,
                           ui::WindowShowState* show_state) const override;
-  bool PreHandleKeyboardEvent(
+  content::KeyboardEventProcessingResult PreHandleKeyboardEvent(
       const content::NativeWebKeyboardEvent& event) override;
   bool HandleKeyboardEvent(
       const content::NativeWebKeyboardEvent& event) override;

chrome/browser/ui/views/frame/native_browser_frame.h

@@ -5,6 +5,7 @@
 #ifndef CHROME_BROWSER_UI_VIEWS_FRAME_NATIVE_BROWSER_FRAME_H_
 #define CHROME_BROWSER_UI_VIEWS_FRAME_NATIVE_BROWSER_FRAME_H_
 
+#include "content/public/browser/keyboard_event_processing_result.h"
 #include "ui/base/ui_base_types.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/views/widget/widget.h"
@@ -36,8 +37,12 @@ class NativeBrowserFrame {
   virtual void GetWindowPlacement(gfx::Rect* bounds,
                                   ui::WindowShowState* show_state) const = 0;
 
-  // Returns true if the |event| was handled by the platform implementation.
-  virtual bool PreHandleKeyboardEvent(
+  // Returns HANDLED if the |event| was handled by the platform implementation
+  // before sending it to the renderer. E.g., it may be swallowed by a native
+  // menu bar. Returns NOT_HANDLED_IS_SHORTCUT if the event was not handled, but
+  // would be handled as a shortcut if the renderer chooses not to handle it.
+  // Otherwise returns NOT_HANDLED.
+  virtual content::KeyboardEventProcessingResult PreHandleKeyboardEvent(
       const content::NativeWebKeyboardEvent& event) = 0;
 
   // Returns true if the |event| was handled by the platform implementation.

chrome/browser/ui/views/media_router/presentation_receiver_window_view.cc

@@ -36,6 +36,10 @@
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/widget/widget.h"
 
+#if defined(OS_MACOSX)
+#include "chrome/browser/global_keyboard_shortcuts_mac.h"
+#endif
+
 #if defined(OS_CHROMEOS)
 #include "ash/public/cpp/window_properties.h"
 #include "ash/public/interfaces/window_state_type.mojom.h"
@@ -142,9 +146,13 @@ PresentationReceiverWindowView::PresentationReceiverWindowView(
 PresentationReceiverWindowView::~PresentationReceiverWindowView() = default;
 
 void PresentationReceiverWindowView::Init() {
-  auto* const focus_manager = GetFocusManager();
-  DCHECK(focus_manager);
-
+#if defined(OS_MACOSX)
+  // On macOS, the mapping between accelerators and commands is dynamic and user
+  // configurable. We fetch and use the default mapping.
+  bool result = GetDefaultMacAcceleratorForCommandId(IDC_FULLSCREEN,
+                                                     &fullscreen_accelerator_);
+  DCHECK(result);
+#else
   const auto accelerators = GetAcceleratorList();
   const auto fullscreen_accelerator =
       std::find_if(accelerators.begin(), accelerators.end(),
@@ -154,6 +162,10 @@ void PresentationReceiverWindowView::Init() {
   DCHECK(fullscreen_accelerator != accelerators.end());
   fullscreen_accelerator_ = ui::Accelerator(fullscreen_accelerator->keycode,
                                             fullscreen_accelerator->modifiers);
+#endif
+
+  auto* const focus_manager = GetFocusManager();
+  DCHECK(focus_manager);
   focus_manager->RegisterAccelerator(
       fullscreen_accelerator_, ui::AcceleratorManager::kNormalPriority, this);
 

chrome/test/BUILD.gn

@@ -537,7 +537,6 @@ test("browser_tests") {
       "../browser/favicon/content_favicon_driver_browsertest.cc",
       "../browser/first_run/first_run_browsertest.cc",
       "../browser/geolocation/geolocation_browsertest.cc",
-      "../browser/global_keyboard_shortcuts_mac_browsertest.mm",
       "../browser/history/history_browsertest.cc",
       "../browser/history/redirect_browsertest.cc",
       "../browser/iframe_browsertest.cc",
@@ -4645,6 +4644,7 @@ if (!is_android) {
       "../browser/extensions/updater/extension_cache_fake.cc",
       "../browser/extensions/updater/extension_cache_fake.h",
       "../browser/extensions/window_open_interactive_apitest.cc",
+      "../browser/global_keyboard_shortcuts_mac_browsertest.mm",
       "../browser/mouse_events_interactive_uitest.cc",
       "../browser/notifications/platform_notification_service_interactive_uitest.cc",
       "../browser/password_manager/password_generation_interactive_uitest.cc",

content/browser/renderer_host/render_widget_host_view_cocoa.mm

@@ -203,7 +203,6 @@ void ExtractUnderlines(NSAttributedString* string,
 - (void)sendViewBoundsInWindowToClient;
 - (void)sendWindowFrameInScreenToClient;
 - (bool)clientIsDisconnected;
-- (bool)isKeyLocked:(int)keyCode;
 @end
 
 @implementation RenderWidgetHostViewCocoa
@@ -556,7 +555,9 @@ void ExtractUnderlines(NSAttributedString* string,
   lockedKeys_.reset();
 }
 
-- (bool)isKeyLocked:(int)keyCode {
+// CommandDispatcherTarget implementation:
+- (BOOL)isKeyLocked:(NSEvent*)event {
+  int keyCode = [event keyCode];
   // Note: We do not want to treat the ESC key as locked as that key is used
   // to exit fullscreen and we don't want to prevent them from exiting.
   ui::DomCode domCode = ui::KeycodeConverter::NativeKeycodeToDomCode(keyCode);
@@ -668,7 +669,7 @@ void ExtractUnderlines(NSAttributedString* string,
 
   // If KeyboardLock has been requested for this keyCode, then mark the event
   // so it skips the pre-handler and is delivered straight to the website.
-  if ([self isKeyLocked:keyCode])
+  if ([self isKeyLocked:theEvent])
     event.skip_in_browser = true;
 
   // Do not forward key up events unless preceded by a matching key down,

ui/base/cocoa/command_dispatcher.h

@@ -21,7 +21,8 @@
 // NSWindow can use CommandDispatcher by implementing CommandDispatchingWindow
 // and overriding -[NSWindow performKeyEquivalent:] and -[NSWindow sendEvent:]
 // to call the respective CommandDispatcher methods.
-UI_BASE_EXPORT @interface CommandDispatcher : NSObject
+UI_BASE_EXPORT
+@interface CommandDispatcher : NSObject
 
 @property(assign, nonatomic) id<CommandDispatcherDelegate> delegate;
 
@@ -59,33 +60,34 @@ UI_BASE_EXPORT @interface CommandDispatcher : NSObject
 
 @end
 
-// If the NSWindow's firstResponder implements CommandDispatcherTarget, it is
-// given the first opportunity to process a command.
+// If the NSWindow's firstResponder implements CommandDispatcherTarget, then
+// it is allowed to grant itself exclusive access to certain keyEquivalents,
+// preempting the usual consumer order.
 @protocol CommandDispatcherTarget
 
-// To handle an event asynchronously, return YES. If the event is ultimately not
-// handled, return the event to the CommandDispatchingWindow via -[[event
-// window] redispatchKeyEvent:event].
-- (BOOL)performKeyEquivalent:(NSEvent*)event;
+// The System Keyboard Lock API <https://w3c.github.io/keyboard-lock/>
+// allows web contents to override keyEquivalents normally reserved by the
+// browser. If the firstResponder returns |YES| from this method, then
+// keyEquivalents shortcuts should be skipped.
+- (BOOL)isKeyLocked:(NSEvent*)event;
 
 @end
 
 // Provides CommandDispatcher with the means to redirect key equivalents at
 // different stages of event handling.
-@protocol CommandDispatcherDelegate<NSObject>
-
-// Called before any other event handling, and possibly again if an unhandled
-// event comes back from CommandDispatcherTarget.
-- (BOOL)eventHandledByExtensionCommand:(NSEvent*)event
-                          isRedispatch:(BOOL)isRedispatch;
+@protocol CommandDispatcherDelegate
 
-// Called before the default -performKeyEquivalent:, but after the
-// CommandDispatcherTarget has had a chance to intercept it. |window| is the
-// CommandDispatchingWindow that owns CommandDispatcher.
+// Gives the delegate a chance to process the keyEquivalent before the first
+// responder. See https://crbug.com/846893#c5 for more details on
+// keyEquivalent consumer ordering. |window| is the CommandDispatchingWindow
+// that owns CommandDispatcher, not the window of the event.
 - (BOOL)prePerformKeyEquivalent:(NSEvent*)event window:(NSWindow*)window;
 
-// Called after the default -performKeyEquivalent:. |window| is the
-// CommandDispatchingWindow that owns CommandDispatcher.
+// Gives the delegate a chance to process the keyEquivalent after the first
+// responder has declined to process the event. See https://crbug.com/846893#c5
+// for more details on keyEquivalent consumer ordering.
+// |window| is the CommandDispatchingWindow that owns CommandDispatcher, not the
+// window of the event.
 - (BOOL)postPerformKeyEquivalent:(NSEvent*)event window:(NSWindow*)window;
 
 @end

ui/base/cocoa/command_dispatcher.mm

@@ -63,6 +63,11 @@ NSEvent* KeyEventForWindow(NSWindow* window, NSEvent* event) {
  @private
   BOOL redispatchingEvent_;
   BOOL eventHandled_;
+
+  // If CommandDispatcher handles a keyEquivalent: [e.g. cmd + w], then it
+  // should suppress future key-up events, e.g. [cmd + w (key up)].
+  BOOL suppressEventsUntilKeyDown_;
+
   NSWindow<CommandDispatchingWindow>* owner_;  // Weak, owns us.
 }
 
@@ -75,31 +80,38 @@ NSEvent* KeyEventForWindow(NSWindow* window, NSEvent* event) {
   return self;
 }
 
-- (BOOL)performKeyEquivalent:(NSEvent*)event {
-  if ([delegate_ eventHandledByExtensionCommand:event
-                                   isRedispatch:redispatchingEvent_]) {
-    return YES;
-  }
-
-  if (redispatchingEvent_)
-    return NO;
-
-  // Give a CommandDispatcherTarget (e.g. a web site) a chance to handle the
-  // event. If it doesn't want to handle it, it will call us back with
-  // -redispatchKeyEvent:. Only allow this behavior when dispatching key events
-  // on the key window.
-  if ([owner_ isKeyWindow]) {
-    NSResponder* r = [owner_ firstResponder];
-    if ([r conformsToProtocol:@protocol(CommandDispatcherTarget)])
-      return [r performKeyEquivalent:event];
+- (BOOL)doPerformKeyEquivalent:(NSEvent*)event {
+  // If |redispatchingEvent_| is true, then this is the second time
+  // performKeyEquivalent: is being called on the event. The first time, a
+  // WebContents was firstResponder and claimed to have handled the event [but
+  // instead sent the event asynchronously to the renderer process]. The
+  // renderer process chose not to handle the event, and the consumer
+  // redispatched the event by calling -[CommandDispatchingWindow
+  // redispatchKeyEvent:].
+  //
+  // We skip all steps before postPerformKeyEquivalent, since those were already
+  // triggered on the first pass of the event.
+  if (redispatchingEvent_) {
+    if ([delegate_ postPerformKeyEquivalent:event window:owner_])
+      return YES;
+    return [[self bubbleParent] performKeyEquivalent:event];
   }
 
+  // First, give the delegate an opportunity to consume this event.
   if ([delegate_ prePerformKeyEquivalent:event window:owner_])
     return YES;
 
+  // Next, pass the event down the NSView hierarchy. Surprisingly, this doesn't
+  // use the responder chain. See implementation of -[NSWindow
+  // performKeyEquivalent:]. If the view hierarchy contains a
+  // RenderWidgetHostViewCocoa, it may choose to return true, and to
+  // asynchronously pass the event to the renderer. See
+  // -[RenderWidgetHostViewCocoa performKeyEquivalent:].
   if ([owner_ defaultPerformKeyEquivalent:event])
     return YES;
 
+  // If the firstResponder [e.g. omnibox] chose not to handle the keyEquivalent,
+  // then give the delegate another chance to consume it.
   if ([delegate_ postPerformKeyEquivalent:event window:owner_])
     return YES;
 
@@ -108,6 +120,17 @@ NSEvent* KeyEventForWindow(NSWindow* window, NSEvent* event) {
   return [[self bubbleParent] performKeyEquivalent:event];
 }
 
+- (BOOL)performKeyEquivalent:(NSEvent*)event {
+  DCHECK_EQ(NSKeyDown, [event type]);
+  suppressEventsUntilKeyDown_ = NO;
+
+  BOOL consumed = [self doPerformKeyEquivalent:event];
+  if (consumed)
+    suppressEventsUntilKeyDown_ = YES;
+
+  return consumed;
+}
+
 - (BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item
                        forHandler:(id<UserInterfaceItemCommandHandler>)handler {
   // Since this class implements these selectors, |super| will always say they
@@ -172,6 +195,16 @@ NSEvent* KeyEventForWindow(NSWindow* window, NSEvent* event) {
 }
 
 - (BOOL)preSendEvent:(NSEvent*)event {
+  // AppKit does not call performKeyEquivalent: if the event only has the
+  // NSEventModifierFlagOption modifier. However, Chrome wants to treat these
+  // events just like keyEquivalents, since they can be consumed by extensions.
+  if ([event type] == NSKeyDown &&
+      ([event modifierFlags] & NSEventModifierFlagOption)) {
+    BOOL handled = [self performKeyEquivalent:event];
+    if (handled)
+      return YES;
+  }
+
   if (redispatchingEvent_) {
     // If we get here, then the event was not handled by NSApplication.
     eventHandled_ = NO;
@@ -179,6 +212,18 @@ NSEvent* KeyEventForWindow(NSWindow* window, NSEvent* event) {
     return YES;
   }
 
+  // This occurs after redispatchingEvent_, since we want a physical
+  // key-press from the user to reset this.
+  if ([event type] == NSKeyDown)
+    suppressEventsUntilKeyDown_ = NO;
+
+  // If CommandDispatcher handled a keyEquivalent: [e.g. cmd + w], then it
+  // should suppress future key-up events, e.g. [cmd + w (key up)].
+  if (suppressEventsUntilKeyDown_ &&
+      ([event type] == NSKeyUp || [event type] == NSFlagsChanged)) {
+    return YES;
+  }
+
   return NO;
 }
 

ui/events/keycodes/keyboard_code_conversion_mac.h

@@ -36,6 +36,9 @@ EVENTS_BASE_EXPORT int MacKeyCodeForWindowsKeyCode(
     unichar* us_keyboard_shifted_character,
     unichar* keyboard_character);
 
+// Returns the WindowsKeyCode from the Mac key code.
+EVENTS_BASE_EXPORT KeyboardCode KeyboardCodeFromKeyCode(unsigned short keyCode);
+
 // This implementation cribbed from:
 //   content/browser/render_host/input/web_input_event_builder_mac.mm
 // Converts |event| into a |KeyboardCode|.  The mapping is not direct as the Mac

ui/events/keycodes/keyboard_code_conversion_mac.mm

@@ -322,144 +322,6 @@ KeyboardCode KeyboardCodeFromCharCode(unichar charCode) {
   return VKEY_UNKNOWN;
 }
 
-KeyboardCode KeyboardCodeFromKeyCode(unsigned short keyCode) {
-  static const KeyboardCode kKeyboardCodes[] = {
-    /* 0 */ VKEY_A,
-    /* 1 */ VKEY_S,
-    /* 2 */ VKEY_D,
-    /* 3 */ VKEY_F,
-    /* 4 */ VKEY_H,
-    /* 5 */ VKEY_G,
-    /* 6 */ VKEY_Z,
-    /* 7 */ VKEY_X,
-    /* 8 */ VKEY_C,
-    /* 9 */ VKEY_V,
-    /* 0x0A */ VKEY_OEM_3, // Section key.
-    /* 0x0B */ VKEY_B,
-    /* 0x0C */ VKEY_Q,
-    /* 0x0D */ VKEY_W,
-    /* 0x0E */ VKEY_E,
-    /* 0x0F */ VKEY_R,
-    /* 0x10 */ VKEY_Y,
-    /* 0x11 */ VKEY_T,
-    /* 0x12 */ VKEY_1,
-    /* 0x13 */ VKEY_2,
-    /* 0x14 */ VKEY_3,
-    /* 0x15 */ VKEY_4,
-    /* 0x16 */ VKEY_6,
-    /* 0x17 */ VKEY_5,
-    /* 0x18 */ VKEY_OEM_PLUS, // =+
-    /* 0x19 */ VKEY_9,
-    /* 0x1A */ VKEY_7,
-    /* 0x1B */ VKEY_OEM_MINUS, // -_
-    /* 0x1C */ VKEY_8,
-    /* 0x1D */ VKEY_0,
-    /* 0x1E */ VKEY_OEM_6, // ]}
-    /* 0x1F */ VKEY_O,
-    /* 0x20 */ VKEY_U,
-    /* 0x21 */ VKEY_OEM_4, // {[
-    /* 0x22 */ VKEY_I,
-    /* 0x23 */ VKEY_P,
-    /* 0x24 */ VKEY_RETURN, // Return
-    /* 0x25 */ VKEY_L,
-    /* 0x26 */ VKEY_J,
-    /* 0x27 */ VKEY_OEM_7, // '"
-    /* 0x28 */ VKEY_K,
-    /* 0x29 */ VKEY_OEM_1, // ;:
-    /* 0x2A */ VKEY_OEM_5, // \|
-    /* 0x2B */ VKEY_OEM_COMMA, // ,<
-    /* 0x2C */ VKEY_OEM_2, // /?
-    /* 0x2D */ VKEY_N,
-    /* 0x2E */ VKEY_M,
-    /* 0x2F */ VKEY_OEM_PERIOD, // .>
-    /* 0x30 */ VKEY_TAB,
-    /* 0x31 */ VKEY_SPACE,
-    /* 0x32 */ VKEY_OEM_3, // `~
-    /* 0x33 */ VKEY_BACK, // Backspace
-    /* 0x34 */ VKEY_UNKNOWN, // n/a
-    /* 0x35 */ VKEY_ESCAPE,
-    /* 0x36 */ VKEY_APPS, // Right Command
-    /* 0x37 */ VKEY_LWIN, // Left Command
-    /* 0x38 */ VKEY_SHIFT, // Left Shift
-    /* 0x39 */ VKEY_CAPITAL, // Caps Lock
-    /* 0x3A */ VKEY_MENU, // Left Option
-    /* 0x3B */ VKEY_CONTROL, // Left Ctrl
-    /* 0x3C */ VKEY_SHIFT, // Right Shift
-    /* 0x3D */ VKEY_MENU, // Right Option
-    /* 0x3E */ VKEY_CONTROL, // Right Ctrl
-    /* 0x3F */ VKEY_UNKNOWN, // fn
-    /* 0x40 */ VKEY_F17,
-    /* 0x41 */ VKEY_DECIMAL, // Num Pad .
-    /* 0x42 */ VKEY_UNKNOWN, // n/a
-    /* 0x43 */ VKEY_MULTIPLY, // Num Pad *
-    /* 0x44 */ VKEY_UNKNOWN, // n/a
-    /* 0x45 */ VKEY_ADD, // Num Pad +
-    /* 0x46 */ VKEY_UNKNOWN, // n/a
-    /* 0x47 */ VKEY_CLEAR, // Num Pad Clear
-    /* 0x48 */ VKEY_VOLUME_UP,
-    /* 0x49 */ VKEY_VOLUME_DOWN,
-    /* 0x4A */ VKEY_VOLUME_MUTE,
-    /* 0x4B */ VKEY_DIVIDE, // Num Pad /
-    /* 0x4C */ VKEY_RETURN, // Num Pad Enter
-    /* 0x4D */ VKEY_UNKNOWN, // n/a
-    /* 0x4E */ VKEY_SUBTRACT, // Num Pad -
-    /* 0x4F */ VKEY_F18,
-    /* 0x50 */ VKEY_F19,
-    /* 0x51 */ VKEY_OEM_PLUS, // Num Pad =.
-    /* 0x52 */ VKEY_NUMPAD0,
-    /* 0x53 */ VKEY_NUMPAD1,
-    /* 0x54 */ VKEY_NUMPAD2,
-    /* 0x55 */ VKEY_NUMPAD3,
-    /* 0x56 */ VKEY_NUMPAD4,
-    /* 0x57 */ VKEY_NUMPAD5,
-    /* 0x58 */ VKEY_NUMPAD6,
-    /* 0x59 */ VKEY_NUMPAD7,
-    /* 0x5A */ VKEY_F20,
-    /* 0x5B */ VKEY_NUMPAD8,
-    /* 0x5C */ VKEY_NUMPAD9,
-    /* 0x5D */ VKEY_UNKNOWN, // Yen (JIS Keyboard Only)
-    /* 0x5E */ VKEY_UNKNOWN, // Underscore (JIS Keyboard Only)
-    /* 0x5F */ VKEY_UNKNOWN, // KeypadComma (JIS Keyboard Only)
-    /* 0x60 */ VKEY_F5,
-    /* 0x61 */ VKEY_F6,
-    /* 0x62 */ VKEY_F7,
-    /* 0x63 */ VKEY_F3,
-    /* 0x64 */ VKEY_F8,
-    /* 0x65 */ VKEY_F9,
-    /* 0x66 */ VKEY_UNKNOWN, // Eisu (JIS Keyboard Only)
-    /* 0x67 */ VKEY_F11,
-    /* 0x68 */ VKEY_UNKNOWN, // Kana (JIS Keyboard Only)
-    /* 0x69 */ VKEY_F13,
-    /* 0x6A */ VKEY_F16,
-    /* 0x6B */ VKEY_F14,
-    /* 0x6C */ VKEY_UNKNOWN, // n/a
-    /* 0x6D */ VKEY_F10,
-    /* 0x6E */ VKEY_APPS, // Context Menu key
-    /* 0x6F */ VKEY_F12,
-    /* 0x70 */ VKEY_UNKNOWN, // n/a
-    /* 0x71 */ VKEY_F15,
-    /* 0x72 */ VKEY_INSERT, // Help
-    /* 0x73 */ VKEY_HOME, // Home
-    /* 0x74 */ VKEY_PRIOR, // Page Up
-    /* 0x75 */ VKEY_DELETE, // Forward Delete
-    /* 0x76 */ VKEY_F4,
-    /* 0x77 */ VKEY_END, // End
-    /* 0x78 */ VKEY_F2,
-    /* 0x79 */ VKEY_NEXT, // Page Down
-    /* 0x7A */ VKEY_F1,
-    /* 0x7B */ VKEY_LEFT, // Left Arrow
-    /* 0x7C */ VKEY_RIGHT, // Right Arrow
-    /* 0x7D */ VKEY_DOWN, // Down Arrow
-    /* 0x7E */ VKEY_UP, // Up Arrow
-    /* 0x7F */ VKEY_UNKNOWN // n/a
-  };
-
-  if (keyCode >= 0x80)
-    return VKEY_UNKNOWN;
-
-  return kKeyboardCodes[keyCode];
-}
-
 DomKey DomKeyFromKeyCode(unsigned short keyCode) {
   switch (keyCode) {
     case kVK_ANSI_KeypadEnter:
@@ -788,6 +650,144 @@ int MacKeyCodeForWindowsKeyCode(KeyboardCode keycode,
   return macKeycode;
 }
 
+KeyboardCode KeyboardCodeFromKeyCode(unsigned short keyCode) {
+  static const KeyboardCode kKeyboardCodes[] = {
+      /* 0 */ VKEY_A,
+      /* 1 */ VKEY_S,
+      /* 2 */ VKEY_D,
+      /* 3 */ VKEY_F,
+      /* 4 */ VKEY_H,
+      /* 5 */ VKEY_G,
+      /* 6 */ VKEY_Z,
+      /* 7 */ VKEY_X,
+      /* 8 */ VKEY_C,
+      /* 9 */ VKEY_V,
+      /* 0x0A */ VKEY_OEM_3,  // Section key.
+      /* 0x0B */ VKEY_B,
+      /* 0x0C */ VKEY_Q,
+      /* 0x0D */ VKEY_W,
+      /* 0x0E */ VKEY_E,
+      /* 0x0F */ VKEY_R,
+      /* 0x10 */ VKEY_Y,
+      /* 0x11 */ VKEY_T,
+      /* 0x12 */ VKEY_1,
+      /* 0x13 */ VKEY_2,
+      /* 0x14 */ VKEY_3,
+      /* 0x15 */ VKEY_4,
+      /* 0x16 */ VKEY_6,
+      /* 0x17 */ VKEY_5,
+      /* 0x18 */ VKEY_OEM_PLUS,  // =+
+      /* 0x19 */ VKEY_9,
+      /* 0x1A */ VKEY_7,
+      /* 0x1B */ VKEY_OEM_MINUS,  // -_
+      /* 0x1C */ VKEY_8,
+      /* 0x1D */ VKEY_0,
+      /* 0x1E */ VKEY_OEM_6,  // ]}
+      /* 0x1F */ VKEY_O,
+      /* 0x20 */ VKEY_U,
+      /* 0x21 */ VKEY_OEM_4,  // {[
+      /* 0x22 */ VKEY_I,
+      /* 0x23 */ VKEY_P,
+      /* 0x24 */ VKEY_RETURN,  // Return
+      /* 0x25 */ VKEY_L,
+      /* 0x26 */ VKEY_J,
+      /* 0x27 */ VKEY_OEM_7,  // '"
+      /* 0x28 */ VKEY_K,
+      /* 0x29 */ VKEY_OEM_1,      // ;:
+      /* 0x2A */ VKEY_OEM_5,      // \|
+      /* 0x2B */ VKEY_OEM_COMMA,  // ,<
+      /* 0x2C */ VKEY_OEM_2,      // /?
+      /* 0x2D */ VKEY_N,
+      /* 0x2E */ VKEY_M,
+      /* 0x2F */ VKEY_OEM_PERIOD,  // .>
+      /* 0x30 */ VKEY_TAB,
+      /* 0x31 */ VKEY_SPACE,
+      /* 0x32 */ VKEY_OEM_3,    // `~
+      /* 0x33 */ VKEY_BACK,     // Backspace
+      /* 0x34 */ VKEY_UNKNOWN,  // n/a
+      /* 0x35 */ VKEY_ESCAPE,
+      /* 0x36 */ VKEY_APPS,     // Right Command
+      /* 0x37 */ VKEY_LWIN,     // Left Command
+      /* 0x38 */ VKEY_SHIFT,    // Left Shift
+      /* 0x39 */ VKEY_CAPITAL,  // Caps Lock
+      /* 0x3A */ VKEY_MENU,     // Left Option
+      /* 0x3B */ VKEY_CONTROL,  // Left Ctrl
+      /* 0x3C */ VKEY_SHIFT,    // Right Shift
+      /* 0x3D */ VKEY_MENU,     // Right Option
+      /* 0x3E */ VKEY_CONTROL,  // Right Ctrl
+      /* 0x3F */ VKEY_UNKNOWN,  // fn
+      /* 0x40 */ VKEY_F17,
+      /* 0x41 */ VKEY_DECIMAL,   // Num Pad .
+      /* 0x42 */ VKEY_UNKNOWN,   // n/a
+      /* 0x43 */ VKEY_MULTIPLY,  // Num Pad *
+      /* 0x44 */ VKEY_UNKNOWN,   // n/a
+      /* 0x45 */ VKEY_ADD,       // Num Pad +
+      /* 0x46 */ VKEY_UNKNOWN,   // n/a
+      /* 0x47 */ VKEY_CLEAR,     // Num Pad Clear
+      /* 0x48 */ VKEY_VOLUME_UP,
+      /* 0x49 */ VKEY_VOLUME_DOWN,
+      /* 0x4A */ VKEY_VOLUME_MUTE,
+      /* 0x4B */ VKEY_DIVIDE,    // Num Pad /
+      /* 0x4C */ VKEY_RETURN,    // Num Pad Enter
+      /* 0x4D */ VKEY_UNKNOWN,   // n/a
+      /* 0x4E */ VKEY_SUBTRACT,  // Num Pad -
+      /* 0x4F */ VKEY_F18,
+      /* 0x50 */ VKEY_F19,
+      /* 0x51 */ VKEY_OEM_PLUS,  // Num Pad =.
+      /* 0x52 */ VKEY_NUMPAD0,
+      /* 0x53 */ VKEY_NUMPAD1,
+      /* 0x54 */ VKEY_NUMPAD2,
+      /* 0x55 */ VKEY_NUMPAD3,
+      /* 0x56 */ VKEY_NUMPAD4,
+      /* 0x57 */ VKEY_NUMPAD5,
+      /* 0x58 */ VKEY_NUMPAD6,
+      /* 0x59 */ VKEY_NUMPAD7,
+      /* 0x5A */ VKEY_F20,
+      /* 0x5B */ VKEY_NUMPAD8,
+      /* 0x5C */ VKEY_NUMPAD9,
+      /* 0x5D */ VKEY_UNKNOWN,  // Yen (JIS Keyboard Only)
+      /* 0x5E */ VKEY_UNKNOWN,  // Underscore (JIS Keyboard Only)
+      /* 0x5F */ VKEY_UNKNOWN,  // KeypadComma (JIS Keyboard Only)
+      /* 0x60 */ VKEY_F5,
+      /* 0x61 */ VKEY_F6,
+      /* 0x62 */ VKEY_F7,
+      /* 0x63 */ VKEY_F3,
+      /* 0x64 */ VKEY_F8,
+      /* 0x65 */ VKEY_F9,
+      /* 0x66 */ VKEY_UNKNOWN,  // Eisu (JIS Keyboard Only)
+      /* 0x67 */ VKEY_F11,
+      /* 0x68 */ VKEY_UNKNOWN,  // Kana (JIS Keyboard Only)
+      /* 0x69 */ VKEY_F13,
+      /* 0x6A */ VKEY_F16,
+      /* 0x6B */ VKEY_F14,
+      /* 0x6C */ VKEY_UNKNOWN,  // n/a
+      /* 0x6D */ VKEY_F10,
+      /* 0x6E */ VKEY_APPS,  // Context Menu key
+      /* 0x6F */ VKEY_F12,
+      /* 0x70 */ VKEY_UNKNOWN,  // n/a
+      /* 0x71 */ VKEY_F15,
+      /* 0x72 */ VKEY_INSERT,  // Help
+      /* 0x73 */ VKEY_HOME,    // Home
+      /* 0x74 */ VKEY_PRIOR,   // Page Up
+      /* 0x75 */ VKEY_DELETE,  // Forward Delete
+      /* 0x76 */ VKEY_F4,
+      /* 0x77 */ VKEY_END,  // End
+      /* 0x78 */ VKEY_F2,
+      /* 0x79 */ VKEY_NEXT,  // Page Down
+      /* 0x7A */ VKEY_F1,
+      /* 0x7B */ VKEY_LEFT,    // Left Arrow
+      /* 0x7C */ VKEY_RIGHT,   // Right Arrow
+      /* 0x7D */ VKEY_DOWN,    // Down Arrow
+      /* 0x7E */ VKEY_UP,      // Up Arrow
+      /* 0x7F */ VKEY_UNKNOWN  // n/a
+  };
+
+  if (keyCode >= 0x80)
+    return VKEY_UNKNOWN;
+
+  return kKeyboardCodes[keyCode];
+}
+
 KeyboardCode KeyboardCodeFromNSEvent(NSEvent* event) {
   KeyboardCode code = VKEY_UNKNOWN;