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..0a0f91ef5fd42beff2c3669009894698096266bf 100644
--- a/chrome/browser/global_keyboard_shortcuts_mac.mm
+++ b/chrome/browser/global_keyboard_shortcuts_mac.mm
@@ -33,6 +33,16 @@ NSMenuItem* FindMenuItem(NSEvent* key, NSMenu* menu) {
   return result;
 }
 
+#if DCHECK_IS_ON()
+// The three main shortcut tables have been mostly migrated to
+// accelerator_table.cc, and should be not used to remove redundant use.
+bool IsMacViews() {
+  // FIXME(mblsha): What's the best way to check for that, that won't trigger on

[tapted, 2016/10/26 08:40:47]

We should just move the tables into separate files. E.g.

chrome/browser/accelerator_table_cocoa_mac.mm/h
chrome/browser/accelerator_table_views_mac.mm/h

and then filter them out in the .gn files

[themblsha, 2016/10/26 17:10:03]

The normal tables are also used by the 

-[ChromeCommandDispatcherDelegate handleExtraKeyboardShortcut:]
  * -[CommandDispatcher performKeyEquivalent:]
    * -[NativeWidgetMacNSWindow performKeyEquivalent:]

So we probably need to create a Views-specific ChromeCommandDispatcherDelegate?

[tapted, 2016/10/28 04:59:48]

I think we can still use ChromeCommandDispatcherDelegate as-is. The trick is to
link in the appropriate set of
CommandFor{[Delayed]Window,Browser}KeyboardShortcut methods that make sense for
mac_views_browser.

So your current global_keyboard_shortcuts_mac.mm needs to split into 3 files

global_keyboard_shortcuts_cocoa_mac.mm
global_keyboard_shortcuts_views_mac.mm
global_keyboard_shortcuts_mac.mm


The sum of global_keyboard_shortcuts_cocoa_mac.mm and
global_keyboard_shortcuts_mac.mm should be identical to what Chrome currently
has.

global_keyboard_shortcuts_views_mac.mm defines the versions of
CommandFor{[Delayed]Window,Browser}KeyboardShortcut that mac_views_browser_wants

global_keyboard_shortcuts_mac.mm holds the shared bits.

[themblsha, 2016/10/28 17:32:22]

That's a great idea and is much simpler than what I tried to do privately before
your comment. Thanks!

+  // normal Cocoa builds?
+  return NSClassFromString(@"NativeWidgetMacNSWindow") != nil;
+}
+#endif  // DCHECK_IS_ON()
+
 }  // namespace
 
 // Basically, there are two kinds of keyboard shortcuts: Ones that should work
@@ -45,6 +55,7 @@ NSMenuItem* FindMenuItem(NSEvent* key, NSMenu* menu) {
 
 const KeyboardShortcutData* GetWindowKeyboardShortcutTable(
     size_t* num_entries) {
+  DCHECK(!IsMacViews());
   static const KeyboardShortcutData keyboard_shortcuts[] = {
       // cmd   shift  cntrl  option
       // ---   -----  -----  ------
@@ -86,6 +97,7 @@ const KeyboardShortcutData* GetWindowKeyboardShortcutTable(
 
 const KeyboardShortcutData* GetDelayedWindowKeyboardShortcutTable(
     size_t* num_entries) {
+  DCHECK(!IsMacViews());
   static const KeyboardShortcutData keyboard_shortcuts[] = {
     //cmd   shift  cntrl  option
     //---   -----  -----  ------
@@ -99,6 +111,7 @@ const KeyboardShortcutData* GetDelayedWindowKeyboardShortcutTable(
 
 const KeyboardShortcutData* GetBrowserKeyboardShortcutTable(
     size_t* num_entries) {
+  DCHECK(!IsMacViews());
   static const KeyboardShortcutData keyboard_shortcuts[] = {
     //cmd   shift  cntrl  option
     //---   -----  -----  ------
@@ -114,6 +127,24 @@ const KeyboardShortcutData* GetBrowserKeyboardShortcutTable(
   return keyboard_shortcuts;
 }
 
+// Lists shortcuts that are impossible to migrate to accelerator_table.cc
+// (crbug.com/25946).
+const KeyboardShortcutData* GetMacViewsKeyboardShortcutTable(
+    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},
+  };
+
+  *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,
@@ -195,7 +226,22 @@ int CommandForBrowserKeyboardShortcut(
                                     key_char);
 }
 
-int CommandForKeyEvent(NSEvent* event) {
+int CommandForMacViewsKeyboardShortcut(
+    bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
+    int vkey_code, unichar key_char) {
+  return CommandForKeyboardShortcut(GetMacViewsKeyboardShortcutTable,
+                                    command_key, shift_key,
+                                    cntrl_key, opt_key, vkey_code,
+                                    key_char);
+}
+
+int CommandForKeyEvent(NSEvent* event,
+                       int (^get_secondary_shortcuts_block)(bool cmdKey,
+                                                            bool shiftKey,
+                                                            bool cntrlKey,
+                                                            bool optKey,
+                                                            int keyCode,
+                                                            unichar keyChar)) {
   if ([event type] != NSKeyDown)
     return -1;
 
@@ -224,17 +270,32 @@ int CommandForKeyEvent(NSEvent* event) {
   const int keyCode = [event keyCode];
   const unichar keyChar = KeyCharacterForEvent(event);
 
-  int cmdNum = CommandForWindowKeyboardShortcut(
-      cmdKey, shiftKey, cntrlKey, optKey, keyCode, keyChar);
-  if (cmdNum != -1)
-    return cmdNum;
+  return get_secondary_shortcuts_block(cmdKey, shiftKey, cntrlKey, optKey,
+                                       keyCode, keyChar);
+}
 
-  cmdNum = CommandForBrowserKeyboardShortcut(
-      cmdKey, shiftKey, cntrlKey, optKey, keyCode, keyChar);
-  if (cmdNum != -1)
-    return cmdNum;
+int CommandForKeyEvent(NSEvent* event) {
+  return CommandForKeyEvent(
+      event, ^int(bool cmdKey, bool shiftKey, bool cntrlKey, bool optKey,

[tapted, 2016/10/26 08:40:47]

I don't think this needs to be a block -- there's nothing captured. You could
declare a helper method in an anonymous namespace and just pass a function
pointer.  I think even neater would probably just be something like a `bool
check_windows_table`. Then move:

CommandForBrowserKeyboardShortcut to accelerator_table_cocoa_mac.mm
CommandForMacViewsKeyboardShortcut to accelerator_table_cocoa_mac.mm (but call
it CommandForBrowserKeyboardShortcut)

But also this might not work - it's very complex, and we need to simplify and
add comments to make it less complex. (and ObjC blocks add particular complexity
that only a few developers understand well, so are best avoided).

[themblsha, 2016/10/28 17:32:22]

Ah, yes, the implicit capturing of blocks. Probably should've just used a func
pointer and a C++ lambda. Anyway, I simplified the code and it should be clearer
now.

+                  int keyCode, unichar keyChar) {
+        int cmdNum = CommandForWindowKeyboardShortcut(
+            cmdKey, shiftKey, cntrlKey, optKey, keyCode, keyChar);
+        if (cmdNum != -1)
+          return cmdNum;
+
+        cmdNum = CommandForBrowserKeyboardShortcut(cmdKey, shiftKey, cntrlKey,
+                                                   optKey, keyCode, keyChar);
+        return cmdNum;
+      });
+}
 
-  return -1;
+int CommandForKeyEventOnMacViews(NSEvent* event) {
+  return CommandForKeyEvent(
+      event, ^int(bool cmdKey, bool shiftKey, bool cntrlKey, bool optKey,
+                  int keyCode, unichar keyChar) {
+        return CommandForMacViewsKeyboardShortcut(cmdKey, shiftKey, cntrlKey,
+                                                  optKey, keyCode, keyChar);
+      });
 }
 
 unichar KeyCharacterForEvent(NSEvent* event) {