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 d1a069f68382935906782042104dc3d7c7059c10..99b4226d24a2ef61e9257d2922a678930474efca 100644 |
--- a/chrome/browser/ui/views/accelerator_table.cc |
+++ b/chrome/browser/ui/views/accelerator_table.cc |
@@ -22,7 +22,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 |
@@ -35,42 +34,40 @@ const ui::EventFlags kPlatformModifier = ui::EF_CONTROL_DOWN; |
// 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
themblsha
2016/06/24 18:22:15
No good reason, probably clumsiness on my part: th
|
- { 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 work
themblsha
2016/06/24 18:22:15
Because it isn't present in the accelerators_cocoa
tapted
2016/06/28 05:19:54
This isn't a good rationale. How does Cmd+9 work o
|
+ { 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 }, |
@@ -118,7 +115,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 }, |
@@ -127,13 +124,14 @@ const AcceleratorMapping kAcceleratorMap[] = { |
{ 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
themblsha
2016/06/24 18:22:15
It was that same original commit at fault again. S
|
+ { 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
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 }, |
@@ -146,6 +144,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 +167,9 @@ 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 |
+#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 #
themblsha
2016/06/24 18:22:15
Ok, added a few more rules to the GetAcceleratorLi
|
+#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 }, |
@@ -178,7 +179,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 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, |
@@ -194,7 +195,55 @@ const AcceleratorMapping kAcceleratorMap[] = { |
{ 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_CONTR
themblsha
2016/06/24 18:22:15
Good idea, squashed a couple more of duplicate ite
|
{ 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 ch
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 m
themblsha
2016/06/24 18:22:15
This was implemented in https://codereview.chromiu
|
+ { 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); |
@@ -227,8 +276,28 @@ const size_t kChromeCmdId2AshActionIdLength = |
} // 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
themblsha
2016/06/24 18:22:15
I thought there was a compile-time warning for thi
tapted
2016/06/28 05:19:54
There probably should be :). We detect static init
|
+ if (accelerators.empty()) { |
+ accelerators = std::vector<AcceleratorMapping>( |
tapted
2016/06/22 03:06:09
The idiomatic way to do this is probably
accelera
themblsha
2016/06/24 18:22:15
No real preference there, thanks for the suggestio
|
+ 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
|
+ // 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
themblsha
2016/06/24 18:22:15
Good ones seem to be:
IDC_STOP 33006 modifiers:; k
|
+ 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'
themblsha
2016/06/24 18:22:15
Good ones seem to be:
IDC_SELECT_NEXT_TAB 34016 mo
|
+ }); |
+ accelerators.erase(it, accelerators.end()); |
+#endif // OS_MACOSX |
+ } |
+ return accelerators; |
} |
bool GetAshAcceleratorForCommandId(int command_id, |
@@ -256,13 +325,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; |