MacViews: Views accelerators table should match the Cocoa one.

chrome/browser/ui/views/accelerator_table.cc

 // Copyright (c) 2012 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/ui/views/accelerator_table.h"
 
 #include <stddef.h>
 
 #include "base/macros.h"
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "ui/base/accelerators/accelerator.h"
 #include "ui/events/event_constants.h"
 
 #if defined(USE_ASH)
 #include "ash/accelerators/accelerator_table.h"
 #endif
 
 namespace chrome {
 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
 const ui::EventFlags kPlatformModifier = ui::EF_CONTROL_DOWN;
 #endif
 
 // NOTE: Keep this list in the same (mostly-alphabetical) order as
 // the Windows accelerators in ../../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
 const AcceleratorMapping kAcceleratorMap[] = {
   { ui::VKEY_LEFT, ui::EF_ALT_DOWN, IDC_BACK },
-  { ui::VKEY_BACK, ui::EF_NONE, IDC_BACKSPACE_BACK },

[tapted, 2016/06/22 03:06:09]

why this? It looks like you're removing backspace to navigate backwards 
(which.. is happening, but it should be orthogonal to this)

[themblsha, 2016/06/24 18:22:15]

No good reason, probably clumsiness on my part: this was done in the original
patchset.

-  { 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 },
   { ui::VKEY_B, ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN, IDC_FOCUS_BOOKMARKS },
   { ui::VKEY_A, ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN, IDC_FOCUS_INFOBARS },
   { ui::VKEY_RIGHT, ui::EF_ALT_DOWN, IDC_FORWARD },
-  { ui::VKEY_BACK, ui::EF_SHIFT_DOWN, IDC_BACKSPACE_FORWARD },

[tapted, 2016/06/22 03:06:09]

this too - I don't think this should be removed

[themblsha, 2016/06/24 18:22:15]

Thanks!

   { ui::VKEY_I, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_DEV_TOOLS },
   { ui::VKEY_F12, ui::EF_NONE, IDC_DEV_TOOLS_TOGGLE },
   { ui::VKEY_J, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
     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_9, kPlatformModifier, IDC_SELECT_LAST_TAB },
-  { ui::VKEY_NUMPAD9, kPlatformModifier, IDC_SELECT_LAST_TAB },
+  { ui::VKEY_S, kPlatformModifier, IDC_SAVE_PAGE },
+  { ui::VKEY_9, ui::EF_CONTROL_DOWN, IDC_SELECT_LAST_TAB },

[tapted, 2016/06/22 03:06:10]

why this change? Cmd+9 to select the last tab works on a Cocoa browser. Ctrl+9
isn't anything.

[themblsha, 2016/06/24 18:22:15]

Because it isn't present in the accelerators_cocoa.mm. Updated to use
kPlatformModifier as well.

[tapted, 2016/06/28 05:19:54]

This isn't a good rationale. How does Cmd+9 work on Cocoa without the mapping?
(is it mapped somewhere else?). Is Cmd+9 actually broken on mac_views_browser?
Does adding this fix it?

I guess this rationale needs to apply to the CL in general, and the bug you've
filed. It's not enough to say "this doesn't match accelerators_cocoa.mm". We
also need to identify that having them not match is causing a problem, and that
mapping them all fixes all the problems.

You need to test all the accelerators you're adding; confirm that they currently
don't work, and that they function correctly after mapping them.

It's possible there's other hidden stuff. Like "this key is intercepted
elsewhere somehow so mapping here doesn't work". We need to verify that the
changes being added are functional and useful, and not just say "make it look
the same as accelerators_cocoa.mm"  (i.e. we don't know whether mimicing
accelerators_cocoa.mm in this way is the correct way to fix the underlying
issues, because we haven't even properly identified what those underlying issues
are).

+  { ui::VKEY_NUMPAD9, ui::EF_CONTROL_DOWN, IDC_SELECT_LAST_TAB },
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   { ui::VKEY_9, ui::EF_ALT_DOWN, IDC_SELECT_LAST_TAB },
   { ui::VKEY_NUMPAD9, ui::EF_ALT_DOWN, IDC_SELECT_LAST_TAB },
   { ui::VKEY_NEXT, ui::EF_CONTROL_DOWN | ui::EF_SHIFT_DOWN, IDC_MOVE_TAB_NEXT },
   { ui::VKEY_PRIOR, ui::EF_CONTROL_DOWN | ui::EF_SHIFT_DOWN,
     IDC_MOVE_TAB_PREVIOUS },
 #endif
   { ui::VKEY_TAB, ui::EF_CONTROL_DOWN, IDC_SELECT_NEXT_TAB },
   { ui::VKEY_NEXT, ui::EF_CONTROL_DOWN, IDC_SELECT_NEXT_TAB },
   { ui::VKEY_TAB, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
@@ skipping 27 matching lines @@
   { ui::VKEY_5, ui::EF_ALT_DOWN, IDC_SELECT_TAB_4 },
   { ui::VKEY_NUMPAD5, ui::EF_ALT_DOWN, IDC_SELECT_TAB_4 },
   { ui::VKEY_6, ui::EF_ALT_DOWN, IDC_SELECT_TAB_5 },
   { ui::VKEY_NUMPAD6, ui::EF_ALT_DOWN, IDC_SELECT_TAB_5 },
   { ui::VKEY_7, ui::EF_ALT_DOWN, IDC_SELECT_TAB_6 },
   { ui::VKEY_NUMPAD7, ui::EF_ALT_DOWN, IDC_SELECT_TAB_6 },
   { ui::VKEY_8, ui::EF_ALT_DOWN, IDC_SELECT_TAB_7 },
   { 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 },
   { ui::VKEY_J, ui::EF_CONTROL_DOWN, IDC_SHOW_DOWNLOADS },
   { ui::VKEY_H, ui::EF_CONTROL_DOWN, IDC_SHOW_HISTORY },
   { ui::VKEY_F, ui::EF_ALT_DOWN, IDC_SHOW_APP_MENU},
   { 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_U, kPlatformModifier | ui::EF_ALT_DOWN, IDC_VIEW_SOURCE },

[tapted, 2016/06/22 03:06:09]

you can't just add alt here - it will change other platforms. This probably
needs to be #ifdef'd out on Mac so we can add a version with alt in the mac
section

[themblsha, 2016/06/24 18:22:15]

It was that same original commit at fault again. Should've pay it more attention
:-/

+  { 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)

[tapted, 2016/06/22 03:06:09]

nit: blank line above

[themblsha, 2016/06/24 18:22:15]

Done.

   { ui::VKEY_F1, ui::EF_NONE, IDC_HELP_PAGE_VIA_KEYBOARD },

[tapted, 2016/06/22 03:06:10]

Comment here like
  // Function keys aren't mapped on Mac.

[themblsha, 2016/06/24 18:22:15]

Done.

   { ui::VKEY_F3, ui::EF_NONE, IDC_FIND_NEXT },
   { ui::VKEY_F3, ui::EF_SHIFT_DOWN, IDC_FIND_PREVIOUS },
   { ui::VKEY_F4, ui::EF_CONTROL_DOWN, IDC_CLOSE_TAB },
   { ui::VKEY_F4, ui::EF_ALT_DOWN, IDC_CLOSE_WINDOW },
   { ui::VKEY_F5, ui::EF_NONE, IDC_RELOAD },
   { ui::VKEY_F5, ui::EF_CONTROL_DOWN, IDC_RELOAD_BYPASSING_CACHE },
   { ui::VKEY_F5, ui::EF_SHIFT_DOWN, IDC_RELOAD_BYPASSING_CACHE },
   { ui::VKEY_F6, ui::EF_NONE, IDC_FOCUS_NEXT_PANE },
   { 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)
   { ui::VKEY_BROWSER_BACK, ui::EF_NONE, IDC_BACK },
   { ui::VKEY_BROWSER_FORWARD, ui::EF_NONE, IDC_FORWARD },
   { ui::VKEY_BROWSER_HOME, ui::EF_NONE, IDC_HOME },
   { ui::VKEY_BROWSER_REFRESH, ui::EF_NONE, IDC_RELOAD },
   { ui::VKEY_BROWSER_REFRESH, ui::EF_CONTROL_DOWN, IDC_RELOAD_BYPASSING_CACHE },
   { ui::VKEY_BROWSER_REFRESH, ui::EF_SHIFT_DOWN, IDC_RELOAD_BYPASSING_CACHE },
 #endif  // defined(OS_LINUX)
 
 #if defined(OS_CHROMEOS)
   // On Chrome OS, VKEY_BROWSER_SEARCH is handled in Ash.
   { ui::VKEY_BACK, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
     IDC_CLEAR_BROWSING_DATA },
   { ui::VKEY_OEM_2, ui::EF_CONTROL_DOWN, IDC_HELP_PAGE_VIA_KEYBOARD },
   { ui::VKEY_OEM_2, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
     IDC_HELP_PAGE_VIA_KEYBOARD },
   { ui::VKEY_BROWSER_FAVORITES, ui::EF_NONE, IDC_SHOW_BOOKMARK_MANAGER },
   { 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
+#elif defined(OS_MACOSX)
+  // Mac-specific shortcuts are defined in a Mac section below.

[tapted, 2016/06/22 03:06:09]

I don't think this is right - we should keep the #else and filter out things
below that we can't have on Mac. i.e. the "else" here is ~"all platforms should
have these, but ChromeOS doesn't because it's special"

[themblsha, 2016/06/24 18:22:15]

Ok, added a few more rules to the GetAcceleratorList() to ignore the non-Mac
ones and removed this #elif.

+#else  // !OS_MACOSX
   { 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 },
   { ui::VKEY_LMENU, ui::EF_NONE, IDC_FOCUS_MENU_BAR },
   { ui::VKEY_MENU, ui::EF_NONE, IDC_FOCUS_MENU_BAR },
   { ui::VKEY_RMENU, ui::EF_NONE, IDC_FOCUS_MENU_BAR },
   // 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 ChromeOS, 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,
     IDC_ZOOM_MINUS },
   // For each entry here add an entry into kChromeCmdId2AshActionId below
   // if Ash has a corresponding accelerator.
 #if defined(GOOGLE_CHROME_BUILD)
   { 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,
     IDC_NEW_INCOGNITO_WINDOW },
   { ui::VKEY_T, ui::EF_CONTROL_DOWN, IDC_NEW_TAB },

[tapted, 2016/06/22 03:06:10]

e.g. this should just be changed like ui::EF_CONTROL_DOWN -> kPlatformModifier
so we avoid duplication

[themblsha, 2016/06/24 18:22:15]

Good idea, squashed a couple more of duplicate items.

   { 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
+#endif  // !OS_CHROMEOS && !OS_MACOSX
+
+#if defined(OS_MACOSX)
+  // VKEY_OEM_4 is Left Brace '[{' key.
+  { ui::VKEY_OEM_4, ui::EF_COMMAND_DOWN, IDC_BACK },
+#if defined(ENABLE_BASIC_PRINTING)

[tapted, 2016/06/22 03:06:09]

this is always true on Mac, so we don't need to check again

[themblsha, 2016/06/24 18:22:15]

Done.

+  { 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_C, ui::EF_COMMAND_DOWN, IDC_CONTENT_CONTEXT_COPY },

[tapted, 2016/06/22 03:06:09]

this isn't mapped for other platforms - why does mac need it? (same for some
others)

[themblsha, 2016/06/24 18:22:15]

This was implemented in https://codereview.chromium.org/152643007 for
https://codereview.chromium.org/161293002

So this is a static mapping of MainMenu.xib commands.

+  { ui::VKEY_X, ui::EF_COMMAND_DOWN, IDC_CONTENT_CONTEXT_CUT },
+  { ui::VKEY_V, ui::EF_COMMAND_DOWN, IDC_CONTENT_CONTEXT_PASTE },
+  { 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_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_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_N, ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN,
+    IDC_NEW_INCOGNITO_WINDOW },
+  { ui::VKEY_T, ui::EF_COMMAND_DOWN, IDC_NEW_TAB },
+  { ui::VKEY_N, ui::EF_COMMAND_DOWN, IDC_NEW_WINDOW },
+  { ui::VKEY_T, ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN, IDC_RESTORE_TAB },
+  { 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_Y, ui::EF_COMMAND_DOWN, IDC_SHOW_HISTORY },
+  { ui::VKEY_OEM_PERIOD, ui::EF_COMMAND_DOWN, IDC_STOP },
+#endif  // OS_MACOSX
 };
 const size_t kAcceleratorMapLength = arraysize(kAcceleratorMap);
 
 #if defined(USE_ASH)
 // Below we map Chrome command ids to Ash action ids for commands that have
 // an shortcut that is handled by Ash (instead of Chrome). Adding entries
 // here will show shortcut text on menus. See comment above.
 struct ChromeCmdId2AshActionId {
   const int chrome_cmd_id;
   const ash::AcceleratorAction ash_action_id;
@@ skipping 12 matching lines @@
   { IDC_TOUCH_HUD_PROJECTION_TOGGLE, ash::TOUCH_HUD_PROJECTION_TOGGLE },
 #endif
 };
 const size_t kChromeCmdId2AshActionIdLength =
     arraysize(kChromeCmdId2AshActionId);
 #endif // defined(USE_ASH)
 
 } // namespace
 
 std::vector<AcceleratorMapping> GetAcceleratorList() {
-  return std::vector<AcceleratorMapping>(
-      kAcceleratorMap, kAcceleratorMap + kAcceleratorMapLength);
+  static std::vector<AcceleratorMapping> accelerators;

[tapted, 2016/06/22 03:06:09]

Chome doesn't allow statics of non-primitive type since it places a destructor
call in the process shutdown path. You can use CR_DEFINE_STATIC_LOCAL instead

[themblsha, 2016/06/24 18:22:15]

I thought there was a compile-time warning for this in the Chromium-Style
plugin? Done.

[tapted, 2016/06/28 05:19:54]

There probably should be :). We detect static initializers on globals with a
script running in the waterfall, which picks them up, but very late. However,
AFAIK, there's nothing for statics local to a function.

+  if (accelerators.empty()) {
+    accelerators = std::vector<AcceleratorMapping>(

[tapted, 2016/06/22 03:06:09]

The idiomatic way to do this is probably

accelerators.insert(accelerators.begin(), std::begin(kAcceleratorMap),
std::end(kAcceleratorMap));

although I guess assigning an rvalue will just call vector::swap too, but if you
prefer that, it should still be constructed using std::end (and
kAcceleratorMapLength isn't used for anything else so can be deleted)

[themblsha, 2016/06/24 18:22:15]

No real preference there, thanks for the suggestion.

+        kAcceleratorMap, kAcceleratorMap + kAcceleratorMapLength);
+#if defined(OS_MACOSX)
+    // Alt, Control and accelerators without modifiers are not very typical for

[tapted, 2016/06/22 03:06:09]

I think we need a "harder" rule here for Mac -- we can't just say it's not
typical since it starts to rely on coincidence with the current set of
accelerators. E.g. we can say

"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.

then

  const kMask = ui::EF_COMMAND_DOWN | ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN;
  auto remove_start = remove_if(...[](..) {
    return (m.modifiers & kMask) == ui::EF_ALT_DOWN;
  }

then we should have something like

#if DCHECK_IS_ON()
  // For everything being removed, ensure there's another mapping.
  for (auto it = remove_start; it != accelerators.end(); ++it) {
    auto found = std::find_if(accelerators.start(), remove_start,
        [](const AcceleratorMapping& m) {
          return m.command_id == it->command_id;
        });
    DCHECK(found != accelerators.end());
  }
#endif
  accelerators.erase(..)


I'm undecided about the DCHECK_IS_ON bit - owners might not like it. A test
might be a nicer place for it, but that requires exposing some extra stuff.
Also, there might be some annoying cases that fail, but I think it's worth
trying.

+    // native Mac apps. Simplify the kAcceleratorMap table by filtering them
+    // out.
+    auto it = std::remove_if(accelerators.begin(), accelerators.end(),
+                             [](const AcceleratorMapping& m) {
+      if (m.modifiers & ui::EF_COMMAND_DOWN) {
+        // Command is Mac-specific, so this accelerator should be enabled.
+        return false;
+      }
+      return m.modifiers == ui::EF_NONE ||

[tapted, 2016/06/22 03:06:09]

why ui::EF_NONE ? Seems we might want those - most of those we don't are
CrOS-specific and already filtered out.

[themblsha, 2016/06/24 18:22:15]

Good ones seem to be:
IDC_STOP 33006 modifiers:; keycode:27
IDC_BACKSPACE_BACK 33010 modifiers:; keycode:8

But there are a bunch of unwanted ones:
IDC_FOCUS_SEARCH 39002 modifiers:; keycode:170
IDC_FOCUS_MENU_BAR 39003 modifiers:; keycode:164
IDC_FOCUS_MENU_BAR 39003 modifiers:; keycode:165
IDC_FOCUS_MENU_BAR 39003 modifiers:; keycode:18
IDC_DEV_TOOLS_TOGGLE 40237 modifiers:; keycode:123

Is #if !defined(OS_MACOSX) advisable in that case? I tried to minimize these by
using a custom find_if matcher in my code.

+             m.modifiers & ui::EF_ALT_DOWN ||
+             m.modifiers & ui::EF_CONTROL_DOWN;

[tapted, 2016/06/22 03:06:10]

Ctrl + PageUp switches tabs on Mac, so this doesn't look right.

[themblsha, 2016/06/24 18:22:15]

Good ones seem to be:
IDC_SELECT_NEXT_TAB 34016 modifiers:(Ctrl); keycode:34
IDC_SELECT_NEXT_TAB 34016 modifiers:(Ctrl); keycode:9
IDC_SELECT_PREVIOUS_TAB 34017 modifiers:(Ctrl)(Shift); keycode:9
IDC_SELECT_PREVIOUS_TAB 34017 modifiers:(Ctrl); keycode:33

But there seems to be a lot of unwanted ones:
IDC_EXIT 34031 modifiers:(Ctrl)(Shift); keycode:81
IDC_VIEW_SOURCE 35002 modifiers:(Ctrl); keycode:85
IDC_BASIC_PRINT 35007 modifiers:(Ctrl)(Shift); keycode:80
IDC_ZOOM_PLUS 38001 modifiers:(Ctrl)(Shift); keycode:187
IDC_ZOOM_MINUS 38003 modifiers:(Ctrl)(Shift); keycode:189
IDC_FOCUS_SEARCH 39002 modifiers:(Ctrl); keycode:69
IDC_FOCUS_SEARCH 39002 modifiers:(Ctrl); keycode:75
IDC_DEV_TOOLS 40004 modifiers:(Ctrl)(Shift); keycode:73
IDC_DEV_TOOLS_CONSOLE 40005 modifiers:(Ctrl)(Shift); keycode:74
IDC_SHOW_HISTORY 40010 modifiers:(Ctrl); keycode:72
IDC_SHOW_BOOKMARK_MANAGER 40011 modifiers:(Ctrl)(Shift); keycode:79
IDC_SHOW_DOWNLOADS 40012 modifiers:(Ctrl); keycode:74
IDC_CLEAR_BROWSING_DATA 40013 modifiers:(Ctrl)(Shift); keycode:46
IDC_DEV_TOOLS_INSPECT 40023 modifiers:(Ctrl)(Shift); keycode:67

+    });
+    accelerators.erase(it, accelerators.end());
+#endif  // OS_MACOSX
+  }
+  return accelerators;
 }
 
 bool GetAshAcceleratorForCommandId(int command_id,
                                    ui::Accelerator* accelerator) {
 #if defined(USE_ASH)
   for (size_t i = 0; i < kChromeCmdId2AshActionIdLength; ++i) {
     if (command_id == kChromeCmdId2AshActionId[i].chrome_cmd_id) {
       for (size_t j = 0; j < ash::kAcceleratorDataLength; ++j) {
         if (kChromeCmdId2AshActionId[i].ash_action_id ==
             ash::kAcceleratorData[j].action) {
           *accelerator = ui::Accelerator(ash::kAcceleratorData[j].keycode,
                                          ash::kAcceleratorData[j].modifiers);
           return true;
         }
       }
     }
   }
 #endif // defined(USE_ASH)
   return false;
 }
 
 bool GetStandardAcceleratorForCommandId(int command_id,
                                         ui::Accelerator* accelerator) {
   // The standard Ctrl-X, Ctrl-V and Ctrl-C are not defined as accelerators
   // 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;
 }
 
 }  // namespace chrome