MacViews: Views accelerators table should match the Cocoa one.

chrome/browser/ui/views/accelerator_table.cc

Index: chrome/browser/ui/views/accelerator_table.cc
diff --git a/chrome/browser/ui/views/accelerator_table.cc b/chrome/browser/ui/views/accelerator_table.cc
index f674d5a8f92abd8abc18a0c07fd5333fe0b799c3..5fcc9fa614c927cc0094456647c34cfedd8abb9f 100644
--- a/chrome/browser/ui/views/accelerator_table.cc
+++ b/chrome/browser/ui/views/accelerator_table.cc
@@ -6,6 +6,12 @@
 
 #include <stddef.h>
 
+#include <algorithm>
+#include <initializer_list>
+#include <set>
+#include <tuple>
+
+#include "base/logging.h"
 #include "base/macros.h"
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
@@ -22,7 +28,6 @@ namespace {
 // 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.
-// TODO(jackhou): If-def out the accelerators that should not be on Mac.
 #if defined(OS_MACOSX)
 const ui::EventFlags kPlatformModifier = ui::EF_COMMAND_DOWN;
 #else
@@ -36,16 +41,16 @@ const ui::EventFlags kPlatformModifier = ui::EF_CONTROL_DOWN;
 const AcceleratorMapping kAcceleratorMap[] = {
   { ui::VKEY_LEFT, ui::EF_ALT_DOWN, IDC_BACK },
   { ui::VKEY_BACK, ui::EF_NONE, IDC_BACKSPACE_BACK },
-  { ui::VKEY_D, ui::EF_CONTROL_DOWN, IDC_BOOKMARK_PAGE },
-  { ui::VKEY_D, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
+  { ui::VKEY_D, kPlatformModifier, IDC_BOOKMARK_PAGE },
+  { ui::VKEY_D, ui::EF_SHIFT_DOWN | kPlatformModifier,
     IDC_BOOKMARK_ALL_TABS },
-  { ui::VKEY_W, ui::EF_CONTROL_DOWN, IDC_CLOSE_TAB },
-  { ui::VKEY_W, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_CLOSE_WINDOW },
-  { ui::VKEY_F, ui::EF_CONTROL_DOWN, IDC_FIND },
-  { ui::VKEY_G, ui::EF_CONTROL_DOWN, IDC_FIND_NEXT },
-  { ui::VKEY_G, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_FIND_PREVIOUS },
+  { ui::VKEY_W, kPlatformModifier, IDC_CLOSE_TAB },
+  { ui::VKEY_W, ui::EF_SHIFT_DOWN | kPlatformModifier, IDC_CLOSE_WINDOW },
+  { ui::VKEY_F, kPlatformModifier, IDC_FIND },
+  { ui::VKEY_G, kPlatformModifier, IDC_FIND_NEXT },
+  { ui::VKEY_G, ui::EF_SHIFT_DOWN | kPlatformModifier, IDC_FIND_PREVIOUS },
   { ui::VKEY_D, ui::EF_ALT_DOWN, IDC_FOCUS_LOCATION },
-  { ui::VKEY_L, ui::EF_CONTROL_DOWN, IDC_FOCUS_LOCATION },
+  { ui::VKEY_L, kPlatformModifier, IDC_FOCUS_LOCATION },
   { ui::VKEY_K, ui::EF_CONTROL_DOWN, IDC_FOCUS_SEARCH },
   { ui::VKEY_E, ui::EF_CONTROL_DOWN, IDC_FOCUS_SEARCH },
   { ui::VKEY_T, ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN, IDC_FOCUS_TOOLBAR },
@@ -59,16 +64,16 @@ const AcceleratorMapping kAcceleratorMap[] = {
     IDC_DEV_TOOLS_CONSOLE },
   { ui::VKEY_C, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
     IDC_DEV_TOOLS_INSPECT },
-  { ui::VKEY_O, ui::EF_CONTROL_DOWN, IDC_OPEN_FILE },
-  { ui::VKEY_P, ui::EF_CONTROL_DOWN, IDC_PRINT},
+  { ui::VKEY_O, kPlatformModifier, IDC_OPEN_FILE },
+  { ui::VKEY_P, kPlatformModifier, IDC_PRINT },
 #if defined(ENABLE_BASIC_PRINTING)
   { ui::VKEY_P, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_BASIC_PRINT},
 #endif  // ENABLE_BASIC_PRINTING
-  { ui::VKEY_R, ui::EF_CONTROL_DOWN, IDC_RELOAD },
-  { ui::VKEY_R, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
+  { ui::VKEY_R, kPlatformModifier, IDC_RELOAD },
+  { ui::VKEY_R, ui::EF_SHIFT_DOWN | kPlatformModifier,
     IDC_RELOAD_BYPASSING_CACHE },
   { ui::VKEY_HOME, ui::EF_ALT_DOWN, IDC_HOME },
-  { ui::VKEY_S, ui::EF_CONTROL_DOWN, IDC_SAVE_PAGE },
+  { ui::VKEY_S, kPlatformModifier, IDC_SAVE_PAGE },
   { ui::VKEY_9, kPlatformModifier, IDC_SELECT_LAST_TAB },
   { ui::VKEY_NUMPAD9, kPlatformModifier, IDC_SELECT_LAST_TAB },
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
@@ -118,7 +123,7 @@ const AcceleratorMapping kAcceleratorMap[] = {
   { ui::VKEY_NUMPAD8, ui::EF_ALT_DOWN, IDC_SELECT_TAB_7 },
   { ui::VKEY_BROWSER_FAVORITES, ui::EF_NONE, IDC_SHOW_BOOKMARK_BAR },
 #endif
-  { ui::VKEY_B, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
+  { ui::VKEY_B, ui::EF_SHIFT_DOWN | kPlatformModifier,
     IDC_SHOW_BOOKMARK_BAR },
   { ui::VKEY_O, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
     IDC_SHOW_BOOKMARK_MANAGER },
@@ -128,12 +133,14 @@ const AcceleratorMapping kAcceleratorMap[] = {
   { ui::VKEY_E, ui::EF_ALT_DOWN, IDC_SHOW_APP_MENU},
   { ui::VKEY_ESCAPE, ui::EF_NONE, IDC_STOP },
   { ui::VKEY_U, ui::EF_CONTROL_DOWN, IDC_VIEW_SOURCE },
-  { ui::VKEY_OEM_MINUS, ui::EF_CONTROL_DOWN, IDC_ZOOM_MINUS },
-  { ui::VKEY_SUBTRACT, ui::EF_CONTROL_DOWN, IDC_ZOOM_MINUS },
-  { ui::VKEY_0, ui::EF_CONTROL_DOWN, IDC_ZOOM_NORMAL },
-  { ui::VKEY_NUMPAD0, ui::EF_CONTROL_DOWN, IDC_ZOOM_NORMAL },
-  { ui::VKEY_OEM_PLUS, ui::EF_CONTROL_DOWN, IDC_ZOOM_PLUS },
-  { ui::VKEY_ADD, ui::EF_CONTROL_DOWN, IDC_ZOOM_PLUS },
+  { ui::VKEY_OEM_MINUS, kPlatformModifier, IDC_ZOOM_MINUS },
+  { ui::VKEY_SUBTRACT, kPlatformModifier, IDC_ZOOM_MINUS },
+  { ui::VKEY_0, kPlatformModifier, IDC_ZOOM_NORMAL },
+  { ui::VKEY_NUMPAD0, kPlatformModifier, IDC_ZOOM_NORMAL },
+  { ui::VKEY_OEM_PLUS, kPlatformModifier, IDC_ZOOM_PLUS },
+  { ui::VKEY_ADD, kPlatformModifier, 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 },
@@ -146,6 +153,7 @@ 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)
@@ -168,7 +176,7 @@ const AcceleratorMapping kAcceleratorMap[] = {
   { ui::VKEY_BROWSER_STOP, ui::EF_NONE, IDC_STOP },
   { ui::VKEY_P, ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN,
     IDC_TOUCH_HUD_PROJECTION_TOGGLE },
-#else  // OS_CHROMEOS
+#else
   { ui::VKEY_ESCAPE, ui::EF_SHIFT_DOWN, IDC_TASK_MANAGER },
   { ui::VKEY_DELETE, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
     IDC_CLEAR_BROWSING_DATA },
@@ -178,7 +186,7 @@ const AcceleratorMapping kAcceleratorMap[] = {
   // On Windows, all VKEY_BROWSER_* keys except VKEY_BROWSER_SEARCH are handled
   // via WM_APPCOMMAND.
   { ui::VKEY_BROWSER_SEARCH, ui::EF_NONE, IDC_FOCUS_SEARCH },
-  { ui::VKEY_M, ui::EF_SHIFT_DOWN | kPlatformModifier, IDC_SHOW_AVATAR_MENU},
+  { ui::VKEY_M, ui::EF_SHIFT_DOWN | kPlatformModifier, IDC_SHOW_AVATAR_MENU },
   // On Chrome OS, these keys are assigned to change UI scale.
   { ui::VKEY_OEM_PLUS, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_ZOOM_PLUS },
   { ui::VKEY_OEM_MINUS, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
@@ -189,14 +197,58 @@ const AcceleratorMapping kAcceleratorMap[] = {
   { ui::VKEY_I, ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN, IDC_FEEDBACK },
 #endif
   { ui::VKEY_Q, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_EXIT },
-  { ui::VKEY_N, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
+  { ui::VKEY_N, ui::EF_SHIFT_DOWN | kPlatformModifier,
     IDC_NEW_INCOGNITO_WINDOW },
-  { ui::VKEY_T, ui::EF_CONTROL_DOWN, IDC_NEW_TAB },
-  { ui::VKEY_N, ui::EF_CONTROL_DOWN, IDC_NEW_WINDOW },
-  { ui::VKEY_T, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_RESTORE_TAB },
-#endif
+  { ui::VKEY_T, kPlatformModifier, IDC_NEW_TAB },
+  { ui::VKEY_N, kPlatformModifier, IDC_NEW_WINDOW },
+  { ui::VKEY_T, ui::EF_SHIFT_DOWN | kPlatformModifier, IDC_RESTORE_TAB },
+#endif  // !OS_CHROMEOS
+
+#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 defined(ENABLE_BASIC_PRINTING)
+  { ui::VKEY_P, ui::EF_COMMAND_DOWN | ui::EF_ALT_DOWN, IDC_BASIC_PRINT },
+#endif  // ENABLE_BASIC_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

[tapted, 2016/11/01 00:22:50]

nit: fullstop at end.

[themblsha, 2016/11/01 14:36:24]

Done.

+  { 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 },
+#endif  // OS_MACOSX
 };
-const size_t kAcceleratorMapLength = arraysize(kAcceleratorMap);
 
 const int kRepeatableCommandIds[] = {
   IDC_FIND_NEXT,
@@ -238,9 +290,45 @@ const size_t kChromeCmdId2AshActionIdLength =
 
 } // namespace
 
+std::vector<AcceleratorMapping> GetUnfilteredAcceleratorListForTesting() {
+  return std::vector<AcceleratorMapping>(std::begin(kAcceleratorMap),
+                                         std::end(kAcceleratorMap));

[Peter Kasting, 2016/10/31 18:07:32]

Nit: Or just inline kAcceleratorMap here:

  return {
      { ui::VKEY_LEFT, ui::EF_ALT_DOWN, IDC_BACK },
      { ui::VKEY_BACK, ui::EF_NONE, IDC_BACKSPACE_BACK },
      ....
  };

Then use this function when building |accelerators| in GetAcceleratorList()
below.

[themblsha, 2016/11/01 14:36:24]

I tried to preserve the original formatting of the kAcceleratorMap table in
order not to break the git blame. Should I reformat the table as well?

[Peter Kasting, 2016/11/01 18:20:58]

Yes, you should reformat/move it (don't insert your global accessor in the
middle of the anonymous namespace).  Don't worry about git blame.

If you want to make sure someone can really understand what's going on, do the
change to inline this as its own CL, with all the functional changes from this
current CL separate.

[themblsha, 2016/11/02 17:31:37]

Ok, I'd better do it separately alongside the de-chrome-ing of
AcceleratorMapping. I should create a crbug for that or use the one I'm using
for this CL? Or use https://bugs.chromium.org/p/chromium/issues/detail?id=289619
?

[Peter Kasting, 2016/11/02 17:51:28]

For de-chrome-ing, use bug 289619.  For inlining kAcceleratorMap, I'd probably
do it with no bug at all, but you're welcome to do whatever you want there.

+}
+
 std::vector<AcceleratorMapping> GetAcceleratorList() {
-  return std::vector<AcceleratorMapping>(
-      kAcceleratorMap, kAcceleratorMap + kAcceleratorMapLength);
+  CR_DEFINE_STATIC_LOCAL(std::vector<AcceleratorMapping>, accelerators, ());
+  if (!accelerators.empty())
+    return accelerators;
+
+  accelerators.insert(accelerators.begin(), std::begin(kAcceleratorMap),
+                      std::end(kAcceleratorMap));
+#if defined(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, so remove the mappings with Alt, but not those with
+  // Command or Control.

[Peter Kasting, 2016/10/31 18:07:32]

Nit: I suggest combining these two remove calls:

  auto RemoveAccelerators = [](const AcceleratorMapping& m) {
    // Alt by itself (or with just shift) is never used on Mac since it's used
    // ...
    ...

    // Control modifier is rarely used on Mac, so we allow it only in several
    // ...
    ...
  };
  accelerators.erase(std::remove_if(accelerators.begin(), accelerators.end(),
                                    RemoveAccelerators),
                     accelerators.end());

This is not only more efficient, it reduces the amount of boilerplate (yay
readability) and makes it simpler to safely add more such tweaks in the future.

[themblsha, 2016/11/01 14:36:24]

Yep, looks much better now, thanks!

+  const int kNonShiftMask =

[Peter Kasting, 2016/10/31 18:07:32]

Nit: constexpr (2 places)

[themblsha, 2016/11/01 14:36:24]

Tried to apply it to the std::set as well, but got this error:
../../chrome/browser/ui/views/accelerator_table.cc:318:29: error: constexpr
variable cannot have non-literal type 'const std::set<int>'
    constexpr std::set<int> control_whitelist = {

Did you mean some other const expression?

[Peter Kasting, 2016/11/01 18:20:58]

That means those sets can't be constexpr because their constructors are not
declared constexpr.  Don't worry about it, they can just be const.

+      ui::EF_COMMAND_DOWN | ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN;
+  auto remove_start = std::remove_if(accelerators.begin(), accelerators.end(),
+                                     [](const AcceleratorMapping& m) {
+    return (m.modifiers & kNonShiftMask) == ui::EF_ALT_DOWN;
+  });
+  accelerators.erase(remove_start, accelerators.end());
+
+  // Control modifier is rarely used on Mac, so we allow it only in several
+  // whiltelisted cases.

[Peter Kasting, 2016/10/31 18:07:32]

Nit: whitelisted

[themblsha, 2016/11/01 14:36:24]

Done.

+  const std::set<int> control_whitelist = {

[Peter Kasting, 2016/10/31 18:07:32]

Nit: kControlWhitelist

[themblsha, 2016/11/01 14:36:24]

Done.

+      IDC_SELECT_NEXT_TAB, IDC_SELECT_PREVIOUS_TAB, IDC_FULLSCREEN,
+  };
+  remove_start =
+      std::remove_if(accelerators.begin(), accelerators.end(),
+                     [&control_whitelist](const AcceleratorMapping& m) {
+        return (m.modifiers & ui::EF_CONTROL_DOWN) != 0 &&
+               control_whitelist.count(m.command_id) == 0;
+      });
+  accelerators.erase(remove_start, accelerators.end());
+#endif  // OS_MACOSX
+  return accelerators;
 }
 
 bool GetAshAcceleratorForCommandId(int command_id,
@@ -268,13 +356,13 @@ bool GetStandardAcceleratorForCommandId(int command_id,
   // anywhere else.
   switch (command_id) {
     case IDC_CUT:
-      *accelerator = ui::Accelerator(ui::VKEY_X, ui::EF_CONTROL_DOWN);
+      *accelerator = ui::Accelerator(ui::VKEY_X, kPlatformModifier);
       return true;
     case IDC_COPY:
-      *accelerator = ui::Accelerator(ui::VKEY_C, ui::EF_CONTROL_DOWN);
+      *accelerator = ui::Accelerator(ui::VKEY_C, kPlatformModifier);
       return true;
     case IDC_PASTE:
-      *accelerator = ui::Accelerator(ui::VKEY_V, ui::EF_CONTROL_DOWN);
+      *accelerator = ui::Accelerator(ui::VKEY_V, kPlatformModifier);
       return true;
   }
   return false;