MacViews: Views accelerators table should match the Cocoa one.

chrome/browser/global_keyboard_shortcuts_mac.mm

Index: chrome/browser/global_keyboard_shortcuts_mac.mm
diff --git a/chrome/browser/global_keyboard_shortcuts_mac.mm b/chrome/browser/global_keyboard_shortcuts_mac.mm
index e197c07c39cfc23ce95fb2536f0edefb821ce2c2..8a2fbb29ea145d7b440181b062f43bdbd2cc43d3 100644
--- a/chrome/browser/global_keyboard_shortcuts_mac.mm
+++ b/chrome/browser/global_keyboard_shortcuts_mac.mm
@@ -33,91 +33,13 @@ NSMenuItem* FindMenuItem(NSEvent* key, NSMenu* menu) {
   return result;
 }
 
-}  // namespace
-
-// Basically, there are two kinds of keyboard shortcuts: Ones that should work
-// only if the tab contents is focused (BrowserKeyboardShortcut), and ones that
-// should work in all other cases (WindowKeyboardShortcut). In the latter case,
-// we differentiate between shortcuts that are checked before any other view
-// gets the chance to handle them (WindowKeyboardShortcut) or after all views
-// had a chance but did not handle the keypress event
-// (DelayedWindowKeyboardShortcut).
-
-const KeyboardShortcutData* GetWindowKeyboardShortcutTable(
-    size_t* num_entries) {
-  static const KeyboardShortcutData keyboard_shortcuts[] = {
-      // cmd   shift  cntrl  option
-      // ---   -----  -----  ------
-      // '{' / '}' characters should be matched earlier than virtual key code
-      // (therefore we can match alt-8 as '{' on german keyboards).
-      {true, false, false, false, 0, '}', IDC_SELECT_NEXT_TAB},
-      {true, false, false, false, 0, '{', IDC_SELECT_PREVIOUS_TAB},
-      {false, false, true, false, kVK_PageDown, 0, IDC_SELECT_NEXT_TAB},
-      {false, false, true, false, kVK_Tab, 0, IDC_SELECT_NEXT_TAB},
-      {false, false, true, false, kVK_PageUp, 0, IDC_SELECT_PREVIOUS_TAB},
-      {false, true, true, false, kVK_Tab, 0, IDC_SELECT_PREVIOUS_TAB},
-      // Cmd-0..8 select the Nth tab, with cmd-9 being "last tab".
-      {true, false, false, false, kVK_ANSI_1, 0, IDC_SELECT_TAB_0},
-      {true, false, false, false, kVK_ANSI_Keypad1, 0, IDC_SELECT_TAB_0},
-      {true, false, false, false, kVK_ANSI_2, 0, IDC_SELECT_TAB_1},
-      {true, false, false, false, kVK_ANSI_Keypad2, 0, IDC_SELECT_TAB_1},
-      {true, false, false, false, kVK_ANSI_3, 0, IDC_SELECT_TAB_2},
-      {true, false, false, false, kVK_ANSI_Keypad3, 0, IDC_SELECT_TAB_2},
-      {true, false, false, false, kVK_ANSI_4, 0, IDC_SELECT_TAB_3},
-      {true, false, false, false, kVK_ANSI_Keypad4, 0, IDC_SELECT_TAB_3},
-      {true, false, false, false, kVK_ANSI_5, 0, IDC_SELECT_TAB_4},
-      {true, false, false, false, kVK_ANSI_Keypad5, 0, IDC_SELECT_TAB_4},
-      {true, false, false, false, kVK_ANSI_6, 0, IDC_SELECT_TAB_5},
-      {true, false, false, false, kVK_ANSI_Keypad6, 0, IDC_SELECT_TAB_5},
-      {true, false, false, false, kVK_ANSI_7, 0, IDC_SELECT_TAB_6},
-      {true, false, false, false, kVK_ANSI_Keypad7, 0, IDC_SELECT_TAB_6},
-      {true, false, false, false, kVK_ANSI_8, 0, IDC_SELECT_TAB_7},
-      {true, false, false, false, kVK_ANSI_Keypad8, 0, IDC_SELECT_TAB_7},
-      {true, false, false, false, kVK_ANSI_9, 0, IDC_SELECT_LAST_TAB},
-      {true, false, false, false, kVK_ANSI_Keypad9, 0, IDC_SELECT_LAST_TAB},
-      {true, true, false, false, kVK_ANSI_M, 0, IDC_SHOW_AVATAR_MENU},
-      {true, false, false, true, kVK_ANSI_L, 0, IDC_SHOW_DOWNLOADS},
-  };
-
-  *num_entries = arraysize(keyboard_shortcuts);
-
-  return keyboard_shortcuts;
-}
-
-const KeyboardShortcutData* GetDelayedWindowKeyboardShortcutTable(
-    size_t* num_entries) {
-  static const KeyboardShortcutData keyboard_shortcuts[] = {
-    //cmd   shift  cntrl  option
-    //---   -----  -----  ------
-    {false, false, false, false, kVK_Escape,        0, IDC_STOP},
-  };
-
-  *num_entries = arraysize(keyboard_shortcuts);
-
-  return keyboard_shortcuts;
-}
-
-const KeyboardShortcutData* GetBrowserKeyboardShortcutTable(
-    size_t* num_entries) {
-  static const KeyboardShortcutData keyboard_shortcuts[] = {
-    //cmd   shift  cntrl  option
-    //---   -----  -----  ------
-    {true,  false, false, false, kVK_LeftArrow,    0,   IDC_BACK},
-    {true,  false, false, false, kVK_RightArrow,   0,   IDC_FORWARD},
-    {false, false, false, false, kVK_Delete,       0,   IDC_BACKSPACE_BACK},
-    {false, true,  false, false, kVK_Delete,       0,   IDC_BACKSPACE_FORWARD},
-    {true,  true,  false, false, 0,                'c', IDC_DEV_TOOLS_INSPECT},
-  };
-
-  *num_entries = arraysize(keyboard_shortcuts);
-
-  return keyboard_shortcuts;
-}
-
-static bool MatchesEventForKeyboardShortcut(
-    const KeyboardShortcutData& shortcut,
-    bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
-    int vkey_code, unichar key_char) {
+bool MatchesEventForKeyboardShortcut(const KeyboardShortcutData& shortcut,
+                                     bool command_key,
+                                     bool shift_key,
+                                     bool cntrl_key,
+                                     bool opt_key,
+                                     int vkey_code,
+                                     unichar key_char) {
   // Expects that one of |key_char| or |vkey_code| is 0.
   DCHECK((shortcut.key_char == 0) ^ (shortcut.vkey_code == 0));
   if (shortcut.key_char) {
@@ -145,11 +67,13 @@ static bool MatchesEventForKeyboardShortcut(
   return false;
 }
 
-static int CommandForKeyboardShortcut(
-    const KeyboardShortcutData* (*get_keyboard_shortcut_table)(size_t*),
-    bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
-    int vkey_code, unichar key_char) {
-
+int CommandForKeyboardShortcut(const std::vector<KeyboardShortcutData>& table,
+                               bool command_key,
+                               bool shift_key,
+                               bool cntrl_key,
+                               bool opt_key,
+                               int vkey_code,
+                               unichar key_char) {
   // Scan through keycodes and see if it corresponds to one of the global
   // shortcuts on file.
   //
@@ -157,21 +81,64 @@ static int CommandForKeyboardShortcut(
   // entries in the array to make a difference.
   // (When turning this into a hash table, note that the current behavior
   // relies on the order of the table (see the comment for '{' / '}' above).
-  size_t num_shortcuts = 0;
-  const KeyboardShortcutData *it = get_keyboard_shortcut_table(&num_shortcuts);
-  for (size_t i = 0; i < num_shortcuts; ++i, ++it) {
-    if (MatchesEventForKeyboardShortcut(*it, command_key, shift_key, cntrl_key,
-                                        opt_key, vkey_code, key_char))
-      return it->chrome_command;
+  for (const auto& shortcut : table) {
+    if (MatchesEventForKeyboardShortcut(shortcut, command_key, shift_key,
+                                        cntrl_key, opt_key, vkey_code,
+                                        key_char))
+      return shortcut.chrome_command;
   }
 
   return -1;
 }
 
+}  // namespace
+
+// MacViews: Lists shortcuts that are impossible to migrate to
+// accelerator_table.cc (crbug.com/25946) as well as those that should work when
+// Omnibox is in focus.

[tapted, 2016/11/14 22:44:49]

This doesn't sound right.

BrowserView::LoadAccelerators() should add everything from accelerator_table.cc
into the views::FocusManager, which is associated with the entire Widget. So
those accelerators should be consulted when the omnibox has focus. We shouldn't
need to duplicate the mapping that hooks into the native NSEvent event dispatch.

Accelerators are processed in Widget::OnKeyEvent, but only if the event wasn't
marked as processed. 

Is the problem that Textfield::OnKeyPressed(..) is saying the event has been
handled, when it should not?

[themblsha, 2016/11/15 11:43:33]

No. The Views code doesn't see the event at all.

This is what happens when you press Cmd+1 while Omnibox is in focus:

1. -[NativeWidgetMacNSWindow performKeyEquivalent:]
  This is where the Cocoa and current MacViews implementation handle commands
  using global_keyboard_shortcuts_mac.mm.

2. -[BridgedContentView keyDown:]
  If the -performKeyEquivalent: returned NO the -keyDown: is called and it
  attempts to translate the key event to the Cocoa Text System command using
  -interpretKeyEvents:.

  * -[NSView interpretKeyEvents:]
    * -[NSTextInputContext handleEvent:]
      * -[NSTextInputContext
_handleEvent:options:allowingSyntheticEvent:completionHandler:]
        Internally consults NSKeyBindingManager and does some TSM stuff. Quite a
lot of code there.

-[BridgedContentView keyUp:] is not called in this case.

You could test by yourself: make sure the GetWindowKeyboardShortcutTable,
GetDelayedWindowKeyboardShortcutTable and GetBrowserKeyboardShortcutTable tables
are all empty, focus the Omnibox and try pressing Cmd+1/2/3 and see that nothing
happens.

How about trying to commit this CL as is and try to improve accelerator handling
separately?

+//
+// Cocoa: see comment in global_keyboard_shortcuts_cocoa_mac.mm.
+const std::vector<KeyboardShortcutData>& GetWindowKeyboardShortcutTable() {
+  CR_DEFINE_STATIC_LOCAL(std::vector<KeyboardShortcutData>, result, ({
+      // cmd   shift  cntrl  option
+      // ---   -----  -----  ------
+      // '{' / '}' characters should be matched earlier than virtual key code
+      // (therefore we can match alt-8 as '{' on german keyboards).
+      {true, false, false, false, 0, '}', IDC_SELECT_NEXT_TAB},
+      {true, false, false, false, 0, '{', IDC_SELECT_PREVIOUS_TAB},
+      {false, false, true, false, kVK_PageDown, 0, IDC_SELECT_NEXT_TAB},
+      {false, false, true, false, kVK_Tab, 0, IDC_SELECT_NEXT_TAB},
+      {false, false, true, false, kVK_PageUp, 0, IDC_SELECT_PREVIOUS_TAB},
+      {false, true, true, false, kVK_Tab, 0, IDC_SELECT_PREVIOUS_TAB},
+      // Cmd-0..8 select the Nth tab, with cmd-9 being "last tab".
+      {true, false, false, false, kVK_ANSI_1, 0, IDC_SELECT_TAB_0},
+      {true, false, false, false, kVK_ANSI_Keypad1, 0, IDC_SELECT_TAB_0},
+      {true, false, false, false, kVK_ANSI_2, 0, IDC_SELECT_TAB_1},
+      {true, false, false, false, kVK_ANSI_Keypad2, 0, IDC_SELECT_TAB_1},
+      {true, false, false, false, kVK_ANSI_3, 0, IDC_SELECT_TAB_2},
+      {true, false, false, false, kVK_ANSI_Keypad3, 0, IDC_SELECT_TAB_2},
+      {true, false, false, false, kVK_ANSI_4, 0, IDC_SELECT_TAB_3},
+      {true, false, false, false, kVK_ANSI_Keypad4, 0, IDC_SELECT_TAB_3},
+      {true, false, false, false, kVK_ANSI_5, 0, IDC_SELECT_TAB_4},
+      {true, false, false, false, kVK_ANSI_Keypad5, 0, IDC_SELECT_TAB_4},
+      {true, false, false, false, kVK_ANSI_6, 0, IDC_SELECT_TAB_5},
+      {true, false, false, false, kVK_ANSI_Keypad6, 0, IDC_SELECT_TAB_5},
+      {true, false, false, false, kVK_ANSI_7, 0, IDC_SELECT_TAB_6},
+      {true, false, false, false, kVK_ANSI_Keypad7, 0, IDC_SELECT_TAB_6},
+      {true, false, false, false, kVK_ANSI_8, 0, IDC_SELECT_TAB_7},
+      {true, false, false, false, kVK_ANSI_Keypad8, 0, IDC_SELECT_TAB_7},
+      {true, false, false, false, kVK_ANSI_9, 0, IDC_SELECT_LAST_TAB},
+      {true, false, false, false, kVK_ANSI_Keypad9, 0, IDC_SELECT_LAST_TAB},
+      {true, true, false, false, kVK_ANSI_M, 0, IDC_SHOW_AVATAR_MENU},
+      {true, false, false, true, kVK_ANSI_L, 0, IDC_SHOW_DOWNLOADS},
+  }));
+  return result;
+}
+
 int CommandForWindowKeyboardShortcut(
     bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
     int vkey_code, unichar key_char) {
-  return CommandForKeyboardShortcut(GetWindowKeyboardShortcutTable,
+  return CommandForKeyboardShortcut(GetWindowKeyboardShortcutTable(),
                                     command_key, shift_key,
                                     cntrl_key, opt_key, vkey_code,
                                     key_char);
@@ -180,7 +147,7 @@ int CommandForWindowKeyboardShortcut(
 int CommandForDelayedWindowKeyboardShortcut(
     bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
     int vkey_code, unichar key_char) {
-  return CommandForKeyboardShortcut(GetDelayedWindowKeyboardShortcutTable,
+  return CommandForKeyboardShortcut(GetDelayedWindowKeyboardShortcutTable(),
                                     command_key, shift_key,
                                     cntrl_key, opt_key, vkey_code,
                                     key_char);
@@ -189,7 +156,7 @@ int CommandForDelayedWindowKeyboardShortcut(
 int CommandForBrowserKeyboardShortcut(
     bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
     int vkey_code, unichar key_char) {
-  return CommandForKeyboardShortcut(GetBrowserKeyboardShortcutTable,
+  return CommandForKeyboardShortcut(GetBrowserKeyboardShortcutTable(),
                                     command_key, shift_key,
                                     cntrl_key, opt_key, vkey_code,
                                     key_char);