Deprecating high-conflict accelerators

ash/accelerators/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 "ash/accelerators/accelerator_table.h"
 
+#include "ash/strings/grit/ash_strings.h"
 #include "base/basictypes.h"
 
 namespace ash {
 
 const AcceleratorData kAcceleratorData[] = {
   // We have to define 3 entries for Shift+Alt. VKEY_[LR]MENU might be sent to
   // the accelerator controller when RenderWidgetHostViewAura is focused, and
   // VKEY_MENU might be when it's not (e.g. when NativeWidgetAura is focused).
   { false, ui::VKEY_LMENU, ui::EF_SHIFT_DOWN, NEXT_IME },
   { false, ui::VKEY_MENU, ui::EF_SHIFT_DOWN, NEXT_IME },
@@ skipping 29 matching lines @@
   { true, ui::VKEY_MEDIA_LAUNCH_APP1, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
     TAKE_PARTIAL_SCREENSHOT },
   { true, ui::VKEY_BRIGHTNESS_DOWN, ui::EF_NONE, BRIGHTNESS_DOWN },
   { true, ui::VKEY_BRIGHTNESS_DOWN, ui::EF_ALT_DOWN, KEYBOARD_BRIGHTNESS_DOWN },
   { true, ui::VKEY_BRIGHTNESS_UP, ui::EF_NONE, BRIGHTNESS_UP },
   { true, ui::VKEY_BRIGHTNESS_UP, ui::EF_ALT_DOWN, KEYBOARD_BRIGHTNESS_UP },
   { true, ui::VKEY_BRIGHTNESS_DOWN, ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN,
     MAGNIFY_SCREEN_ZOOM_OUT},
   { true, ui::VKEY_BRIGHTNESS_UP, ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN,
     MAGNIFY_SCREEN_ZOOM_IN},
-  { true, ui::VKEY_L, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, LOCK_SCREEN },
   // The lock key on Chrome OS keyboards produces F13 scancodes.
   { true, ui::VKEY_F13, ui::EF_NONE, LOCK_PRESSED },
   { false, ui::VKEY_F13, ui::EF_NONE, LOCK_RELEASED },
   // Generic keyboards can use VKEY_SLEEP to mimic ChromeOS keyboard's lock key.
   { true, ui::VKEY_SLEEP, ui::EF_NONE, LOCK_PRESSED },
   { false, ui::VKEY_SLEEP, ui::EF_NONE, LOCK_RELEASED },
   { true, ui::VKEY_POWER, ui::EF_NONE, POWER_PRESSED },
   { false, ui::VKEY_POWER, ui::EF_NONE, POWER_RELEASED },
   { true, ui::VKEY_M, ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN,
     OPEN_FILE_MANAGER },
@@ skipping 26 matching lines @@
   { true, ui::VKEY_8, ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN | ui::EF_SHIFT_DOWN,
     TOGGLE_TOUCH_VIEW_TESTING },
   // Single shift release turns off caps lock.
   { false, ui::VKEY_LSHIFT, ui::EF_NONE, DISABLE_CAPS_LOCK },
   { false, ui::VKEY_SHIFT, ui::EF_NONE, DISABLE_CAPS_LOCK },
   { false, ui::VKEY_RSHIFT, ui::EF_NONE, DISABLE_CAPS_LOCK },
   { false, ui::VKEY_LWIN, ui::EF_ALT_DOWN, TOGGLE_CAPS_LOCK },
   { true, ui::VKEY_VOLUME_MUTE, ui::EF_NONE, VOLUME_MUTE },
   { true, ui::VKEY_VOLUME_DOWN, ui::EF_NONE, VOLUME_DOWN },
   { true, ui::VKEY_VOLUME_UP, ui::EF_NONE, VOLUME_UP },
+#else   // defined(OS_CHROMEOS)
+    // This key has been deprecated on CrOS. It is instead included below in the

[oshima, 2015/06/16 05:54:17]

indent

[afakhry, 2015/06/16 22:30:33]

That was done by 'git cl format ash', not sure why! If I don't do it I get a
warning when uploading.

I'll do it anyway.

+    // |kDeprecatedAccelerators|.
+    {true, ui::VKEY_ESCAPE, ui::EF_SHIFT_DOWN, SHOW_TASK_MANAGER},
 #endif  // defined(OS_CHROMEOS)
   { true, ui::VKEY_I, ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN, OPEN_FEEDBACK_PAGE },
 #if !defined(OS_WIN)
   { true, ui::VKEY_Q, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, EXIT },
 #endif
   { true, ui::VKEY_N, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN,
     NEW_INCOGNITO_WINDOW },
   { true, ui::VKEY_N, ui::EF_CONTROL_DOWN, NEW_WINDOW },
   { true, ui::VKEY_T, ui::EF_CONTROL_DOWN, NEW_TAB },
   { true, ui::VKEY_OEM_MINUS,
@@ skipping 20 matching lines @@
   { true, ui::VKEY_OEM_2, ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN,
     SHOW_KEYBOARD_OVERLAY },
   { true, ui::VKEY_OEM_2,
     ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN,
     SHOW_KEYBOARD_OVERLAY },
   { true, ui::VKEY_F14, ui::EF_NONE, SHOW_KEYBOARD_OVERLAY },
   { true, ui::VKEY_N, ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN,
     SHOW_MESSAGE_CENTER_BUBBLE },
   { true, ui::VKEY_S, ui::EF_SHIFT_DOWN | ui::EF_ALT_DOWN,
     SHOW_SYSTEM_TRAY_BUBBLE },
-  { true, ui::VKEY_ESCAPE, ui::EF_SHIFT_DOWN, SHOW_TASK_MANAGER },
   { true, ui::VKEY_1, ui::EF_ALT_DOWN, LAUNCH_APP_0 },
   { true, ui::VKEY_2, ui::EF_ALT_DOWN, LAUNCH_APP_1 },
   { true, ui::VKEY_3, ui::EF_ALT_DOWN, LAUNCH_APP_2 },
   { true, ui::VKEY_4, ui::EF_ALT_DOWN, LAUNCH_APP_3 },
   { true, ui::VKEY_5, ui::EF_ALT_DOWN, LAUNCH_APP_4 },
   { true, ui::VKEY_6, ui::EF_ALT_DOWN, LAUNCH_APP_5 },
   { true, ui::VKEY_7, ui::EF_ALT_DOWN, LAUNCH_APP_6 },
   { true, ui::VKEY_8, ui::EF_ALT_DOWN, LAUNCH_APP_7 },
   { true, ui::VKEY_9, ui::EF_ALT_DOWN, LAUNCH_LAST_APP },
 
@@ skipping 21 matching lines @@
   { false, ui::VKEY_PRIOR, ui::EF_SHIFT_DOWN, ACCESSIBLE_FOCUS_PREVIOUS},
   { false, ui::VKEY_END, ui::EF_SHIFT_DOWN, ACCESSIBLE_FOCUS_NEXT},
   { false, ui::VKEY_NEXT, ui::EF_SHIFT_DOWN, ACCESSIBLE_FOCUS_NEXT},
 
   // TODO(yusukes): Handle VKEY_MEDIA_STOP, and
   // VKEY_MEDIA_LAUNCH_MAIL.
 };
 
 const size_t kAcceleratorDataLength = arraysize(kAcceleratorData);
 
+// Instructions for how to deprecate and replace an Accelerator:
+//
+// 1- Remove the old deprecated accelerator from the above list.
+// 2- Add an entry for it in the following |kDeprecatedAccelerators| list.
+// 3- That entry should contain the following:
+//    - The deprecated accelerator data you removed from above.
+//    - The new accelerator that should replace it.
+//        - Both the new and old accelerators MUST map to the same
+//          |AcceleratorAction|.
+//    - Define a histogram for this action in |histograms.xml| in the form
+//      "Ash.Accelerators.{ActionName}" and include the name of this histogram
+//      in this entry.
+//    - The ID of the localized notification message to give the users telling
+//      them about the deprecation (Add one in |ash_chromeos_strings.grdp|.
+//      Search for the comment <!-- Deprecated Accelerators Messages -->).
+//    - {true or false} whether the notification should be displayed every time
+//      the user uses the deprecated accelerator.
+//    - {true or false} whether the deprecated accelerator is still enabled (we
+//      don't disable a deprecated accelerator abruptly).
+// 4- Don't forget to update the keyboard overlay.
+
+const DeprecatedAcceleratorData kDeprecatedAccelerators[] = {

[oshima, 2015/06/16 05:54:17]

instead of defining both here, it's be better to separate new and old
accelerator info so that we can just remove old ones once we're done.

[afakhry, 2015/06/16 22:30:33]

Let's discuss tomorrow, then.

+#if defined(OS_CHROMEOS)
+    {{true, ui::VKEY_L, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, LOCK_SCREEN},
+     {true, ui::VKEY_L, ui::EF_COMMAND_DOWN, LOCK_SCREEN},
+     "Ash.Accelerators.LockScreen",
+     IDS_DEPRECATED_LOCK_SCREEN_MSG,
+     true,
+     true},
+    {{true, ui::VKEY_ESCAPE, ui::EF_SHIFT_DOWN, SHOW_TASK_MANAGER},
+     {true, ui::VKEY_ESCAPE, ui::EF_COMMAND_DOWN, SHOW_TASK_MANAGER},
+     "Ash.Accelerators.ShowTaskManager",
+     IDS_DEPRECATED_SHOW_TASK_MANAGER_MSG,
+     true,
+     true},
+#endif  // defined(OS_CHROMEOS)
+};
+
+const size_t kDeprecatedAcceleratorsLength = arraysize(kDeprecatedAccelerators);
+
 const AcceleratorData kDebugAcceleratorData[] = {
 #if defined(OS_CHROMEOS)
   // Extra shortcut for debug build to control magnifier on linux desktop.
   { true, ui::VKEY_BRIGHTNESS_DOWN, ui::EF_CONTROL_DOWN,
     MAGNIFY_SCREEN_ZOOM_OUT},
   { true, ui::VKEY_BRIGHTNESS_UP, ui::EF_CONTROL_DOWN, MAGNIFY_SCREEN_ZOOM_IN},
   // Extra shortcuts to lock the screen on linux desktop.
   { true, ui::VKEY_L, ui::EF_ALT_DOWN, LOCK_SCREEN },
   { true, ui::VKEY_POWER, ui::EF_SHIFT_DOWN, LOCK_PRESSED },
   { false, ui::VKEY_POWER, ui::EF_SHIFT_DOWN, LOCK_RELEASED },
@@ skipping 223 matching lines @@
     WINDOW_MINIMIZE,
     TOGGLE_FULLSCREEN,
     TOGGLE_MAXIMIZED,
     WINDOW_POSITION_CENTER,
     ROTATE_WINDOW,
 };
 
 const size_t kActionsNeedingWindowLength = arraysize(kActionsNeedingWindow);
 
 }  // namespace ash