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 <algorithm>
+#include <initializer_list>
+#include <set>

[tapted, 2016/11/09 22:29:51]

(remember to update the #includes)

+#include <tuple>
+
 #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/common/accelerators/accelerator_table.h"  // nogncheck
 #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.

[tapted, 2016/11/09 22:29:52]

This comment was one of the motivations for the filtering approach. I don't
there's a neat way to maintain this requirement without filtering.

A better alternative might be a completely separate kAcceleratorMap for Mac?

Or we could drop the other {alphabetical, chrome_dll.rc} ordering requirements
and just cluster things by platform.

[themblsha, 2016/11/10 14:18:05]

BTW is there any reason that chrome_dll.rc is not strictly alphabetical? I've
tried re-sorting it and it results in this diff:
https://gist.github.com/mblsha/514db7237dbbedc20d2ae9f21f6eee81

[Peter Kasting, 2016/11/10 17:56:38]

Not that I know of.

I vote for "sort that list", and that takes care of some of the issues with "try
to keep this list in that list's order".

[themblsha, 2016/11/11 12:20:32]

Ok, I'll create a separate CL for that.

 // 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 },
-  { 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_K, ui::EF_CONTROL_DOWN, IDC_FOCUS_SEARCH },
-  { ui::VKEY_E, ui::EF_CONTROL_DOWN, IDC_FOCUS_SEARCH },
+  { ui::VKEY_L, kPlatformModifier, IDC_FOCUS_LOCATION },
   { 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 },
-  { 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},
-#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_O, kPlatformModifier, IDC_OPEN_FILE },
+  { ui::VKEY_P, kPlatformModifier, IDC_PRINT },
+  { 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)
   { 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 },
@@ skipping 29 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_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 },
   { 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
+#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 },
   { 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},
-  // 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,
-    IDC_ZOOM_MINUS },
+  { ui::VKEY_M, ui::EF_SHIFT_DOWN | kPlatformModifier, IDC_SHOW_AVATAR_MENU },
   // 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
+#if !defined(OS_MACOSX)
   { ui::VKEY_Q, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_EXIT },
-  { ui::VKEY_N, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
+#endif  // !OS_MACOSX
+  { 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.
+  { 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 },
+#else
+  // Control modifier is rarely used on Mac, so we allow it only in several

[tapted, 2016/11/09 22:29:51]

This is now in !Mac, so the comment doesn't look right here.

[themblsha, 2016/11/10 14:18:05]

I've now also added a bit about Alt-shortcuts. My intention was to describe why
all these shortcuts are bunched up in !Mac section so it would be easier to find
a correct place for new accelerators in the future.

I've tried re-wording it, but it becomes less concise and less clear of the
intention.

+  // specific cases. The rest shouldn't be used on Mac and are listed below.
+  { ui::VKEY_K, ui::EF_CONTROL_DOWN, IDC_FOCUS_SEARCH },
+  { ui::VKEY_E, ui::EF_CONTROL_DOWN, IDC_FOCUS_SEARCH },
+  { ui::VKEY_I, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_DEV_TOOLS },

[tapted, 2016/11/09 22:29:52]

(E.g. Moving this down violates the desire to keep this in the same order as
chrome_dll.rc)

[themblsha, 2016/11/10 14:18:05]

Ohcrap. I've re-sorted the accelerators in this section now, but I don't know
how to satisfy the same ordering as with chrome_dll.rc :/

+  { 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 },
+#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_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_U, ui::EF_CONTROL_DOWN, IDC_VIEW_SOURCE },
+  { ui::VKEY_DELETE, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
+    IDC_CLEAR_BROWSING_DATA },
+  // 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,
+    IDC_ZOOM_MINUS },
+#endif  // OS_MACOSX
 };
-const size_t kAcceleratorMapLength = arraysize(kAcceleratorMap);
 
 const int kRepeatableCommandIds[] = {
   IDC_FIND_NEXT,
   IDC_FIND_PREVIOUS,
   IDC_FOCUS_NEXT_PANE,
   IDC_FOCUS_PREVIOUS_PANE,
   IDC_MOVE_TAB_NEXT,
   IDC_MOVE_TAB_PREVIOUS,
   IDC_SELECT_NEXT_TAB,
   IDC_SELECT_PREVIOUS_TAB,
@@ skipping 19 matching lines @@
   { IDC_RESTORE_TAB,          ash::RESTORE_TAB },
   { IDC_TASK_MANAGER,         ash::SHOW_TASK_MANAGER },
 #if defined(OS_CHROMEOS)
   { IDC_TOUCH_HUD_PROJECTION_TOGGLE, ash::TOUCH_HUD_PROJECTION_TOGGLE },
 #endif
 };
 const size_t kChromeCmdId2AshActionIdLength =
     arraysize(kChromeCmdId2AshActionId);
 #endif // defined(USE_ASH)
 
+std::vector<AcceleratorMapping> GetAllAcceleratorMappings() {
+  return std::vector<AcceleratorMapping>(std::begin(kAcceleratorMap),
+                                         std::end(kAcceleratorMap));
+}
+
 } // namespace
 
+std::vector<AcceleratorMapping> GetUnfilteredAcceleratorListForTesting() {
+  return GetAllAcceleratorMappings();
+}
+
 std::vector<AcceleratorMapping> GetAcceleratorList() {
-  return std::vector<AcceleratorMapping>(
-      kAcceleratorMap, kAcceleratorMap + kAcceleratorMapLength);
+  static bool is_accelerator_list_initialized = false;
+  CR_DEFINE_STATIC_LOCAL(std::vector<AcceleratorMapping>, accelerators,
+                         (GetAllAcceleratorMappings()));
+  if (is_accelerator_list_initialized)
+    return accelerators;
+
+  is_accelerator_list_initialized = true;
+#if defined(OS_MACOSX)
+  auto remove_accelerators = [](const AcceleratorMapping& m) {

[sky, 2016/11/09 18:30:32]

Isn't this unnecessary now? A NDEBUG block if you want to verify that is fine.

[themblsha, 2016/11/10 14:18:05]

We still need to remove the Alt+VKEY and Alt+Shift+VKEY combinations. There's no
whitelist so they're completely pruned.

[Peter Kasting, 2016/11/10 17:56:38]

If we still need this lambda regardless, then I'm skeptical of the tradeoff that
simplifies this lambda but adds more ifdefs above.  Is there an updated version
of the diff just between those two approaches?

[themblsha, 2016/11/11 12:20:32]

No, lambda is not required anymore. Here's a complete diff:
https://gist.github.com/mblsha/945b8039d0b2e9c60e9f41aa18c19945

+    // 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.
+    constexpr int kNonShiftMask =
+        ui::EF_COMMAND_DOWN | ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN;
+    if ((m.modifiers & kNonShiftMask) == ui::EF_ALT_DOWN)
+      return true;
+
+    return false;
+  };
+  accelerators.erase(std::remove_if(accelerators.begin(), accelerators.end(),
+                                    remove_accelerators),
+                     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;
 }
 
 bool IsCommandRepeatable(int command_id) {
   for (size_t i = 0; i < kRepeatableCommandIdsLength; ++i) {
     if (kRepeatableCommandIds[i] == command_id)
       return true;
   }
   return false;
 }
 
 }  // namespace chrome