MacViews: Views accelerators table should match the Cocoa one.

chrome/browser/global_keyboard_shortcuts_mac.mm

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/global_keyboard_shortcuts_mac.h"
 
 #import <AppKit/AppKit.h>
 
 #include "base/logging.h"
 #include "base/macros.h"
@@ skipping 15 matching lines @@
       result = item;
     }
 
     if (result)
       break;
   }
 
   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
 // 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) {
+  DCHECK(!IsMacViews());
   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},
@@ skipping 21 matching lines @@
       {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) {
+  DCHECK(!IsMacViews());
   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) {
+  DCHECK(!IsMacViews());
   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;
 }
 
+// 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,
     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) {
     // Shortcuts that have a |key_char| and have |opt_key| set are mistakes,
     // since |opt_key| is not checked when there is a |key_char|.
     DCHECK(!shortcut.opt_key);
@@ skipping 61 matching lines @@
 
 int CommandForBrowserKeyboardShortcut(
     bool command_key, bool shift_key, bool cntrl_key, bool opt_key,
     int vkey_code, unichar key_char) {
   return CommandForKeyboardShortcut(GetBrowserKeyboardShortcutTable,
                                     command_key, shift_key,
                                     cntrl_key, opt_key, vkey_code,
                                     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;
 
   // Look in menu.
   NSMenuItem* item = FindMenuItem(event, [NSApp mainMenu]);
   if (item && [item action] == @selector(commandDispatch:) && [item tag] > 0)
     return [item tag];
 
   // "Close window" doesn't use the |commandDispatch:| mechanism. Menu items
   // that do not correspond to IDC_ constants need no special treatment however,
   // as they can't be blacklisted in
   // |BrowserCommandController::IsReservedCommandOrKey()| anyhow.
   if (item && [item action] == @selector(performClose:))
     return IDC_CLOSE_WINDOW;
 
   // "Exit" doesn't use the |commandDispatch:| mechanism either.
   if (item && [item action] == @selector(terminate:))
     return IDC_EXIT;
 
   // Look in secondary keyboard shortcuts.
   NSUInteger modifiers = [event modifierFlags];
   const bool cmdKey = (modifiers & NSCommandKeyMask) != 0;
   const bool shiftKey = (modifiers & NSShiftKeyMask) != 0;
   const bool cntrlKey = (modifiers & NSControlKeyMask) != 0;
   const bool optKey = (modifiers & NSAlternateKeyMask) != 0;
   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;
 
-  return -1;
+        cmdNum = CommandForBrowserKeyboardShortcut(cmdKey, shiftKey, cntrlKey,
+                                                   optKey, keyCode, keyChar);
+        return cmdNum;
+      });
+}
+
+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) {
   NSString* eventString = [event charactersIgnoringModifiers];
   NSString* characters = [event characters];
 
   if ([eventString length] != 1)
     return 0;
 
   if ([characters length] != 1)
@@ skipping 46 matching lines @@
       }
     }
 
     // opt/alt modifier is set (e.g. on german layout we want '{' for opt-8).
     if ([event modifierFlags] & NSAlternateKeyMask)
       return rawChar;
   }
 
   return noModifiersChar;
 }