Remove the deprecated Alt+Shift shortcut completely

ash/common/accelerators/accelerator_controller.cc

Index: ash/common/accelerators/accelerator_controller.cc
diff --git a/ash/common/accelerators/accelerator_controller.cc b/ash/common/accelerators/accelerator_controller.cc
index 11471e730a1566759d07dc6e53c12c6147b1494f..567c056ef8a0ead3ec67429682684e47662204d2 100644
--- a/ash/common/accelerators/accelerator_controller.cc
+++ b/ash/common/accelerators/accelerator_controller.cc
@@ -173,26 +173,6 @@ bool CanHandleCycleMru(const ui::Accelerator& accelerator) {
   return !(keyboard_controller && keyboard_controller->keyboard_visible());
 }
 
-// We must avoid showing the Deprecated NEXT_IME notification erronously.
-bool ShouldShowDeprecatedNextImeNotification(
-    const ui::Accelerator& previous_accelerator) {
-  // We only show the deprecation notification if the previous accelerator key
-  // is ONLY either Shift, or Alt.
-  const ui::KeyboardCode previous_key_code = previous_accelerator.key_code();
-  switch (previous_key_code) {
-    case ui::VKEY_SHIFT:
-    case ui::VKEY_LSHIFT:
-    case ui::VKEY_RSHIFT:
-    case ui::VKEY_MENU:
-    case ui::VKEY_LMENU:
-    case ui::VKEY_RMENU:
-      return true;
-
-    default:
-      return false;
-  }
-}
-
 void HandleNextIme(ImeControlDelegate* ime_control_delegate) {
   base::RecordAction(UserMetricsAction("Accel_Next_Ime"));
   ime_control_delegate->HandleNextIme();
@@ -635,45 +615,16 @@ bool AcceleratorController::AcceleratorPressed(
       accelerators_.find(accelerator);
   DCHECK(it != accelerators_.end());
   AcceleratorAction action = it->second;
-  if (CanPerformAction(action, accelerator)) {
-    // Handling the deprecated accelerators (if any) only if action can be
-    // performed.
-    auto itr = actions_with_deprecations_.find(action);
-    if (itr != actions_with_deprecations_.end()) {
-      const DeprecatedAcceleratorData* data = itr->second;
-      if (deprecated_accelerators_.count(accelerator)) {
-        // This accelerator has been deprecated and should be treated according
-        // to its |DeprecatedAcceleratorData|.
-
-        // Record UMA stats.
-        RecordUmaHistogram(data->uma_histogram_name, DEPRECATED_USED);
-
-        // We always display the notification as long as this entry exists,
-        // except for NEXT_IME, we must check to avoid showing it for the wrong
-        // shortcut, as Alt+Shift is tricky and trigger the action on release.
-        if (delegate_ &&
-            (action != NEXT_IME ||
-             (action == NEXT_IME &&
-              ShouldShowDeprecatedNextImeNotification(
-                  accelerator_history_->previous_accelerator())))) {
-          delegate_->ShowDeprecatedAcceleratorNotification(
-              data->uma_histogram_name, data->notification_message_id,
-              data->old_shortcut_id, data->new_shortcut_id);
-        }
-
-        if (!data->deprecated_enabled)
-          return false;
-      } else {
-        // This is a new accelerator replacing the old deprecated one.
-        // Record UMA stats and proceed normally.
-        RecordUmaHistogram(data->uma_histogram_name, NEW_USED);
-      }
-    }
+  if (!CanPerformAction(action, accelerator))
+    return false;
 
-    PerformAction(action, accelerator);
-    return ShouldActionConsumeKeyEvent(action);
-  }
-  return false;
+  // Handling the deprecated accelerators (if any) only if action can be
+  // performed.
+  if (!MaybeDeprecatedAcceleratorPressed(action, accelerator))

[James Cook, 2017/01/27 16:40:30]

nit: I wonder if there's a better name for this function. It's hard for me to
interpret the return value. Do you think it would be clearer if that was called
MaybeHandleDeprecatedAccelerator() and invert the return value?  Then the return
value means "I handled it" or "I consumed it". It's just hard for me to reason
about why that function returning false makes this function return false.

[afakhry, 2017/01/27 17:27:41]

I thought of that too, I didn't want to name it *Handle*DeprecatedAccelerator()
since it doesn't actually handle the accelerator, rather does the pre-handling
(UMA reporting, showing the notification, and determining if the accelerator's
action should be performed).

We can either change it to make it actually handle the accelerator, but then
we'd be handling the accelerator in two places,
or, instead of returning a confusing bool, we return an enum value that tells us
what to do next, whether proceed with handling the accelerator, or stop.

I went for the second solution. Please let me know what you think.

+    return false;
+
+  PerformAction(action, accelerator);
+  return ShouldActionConsumeKeyEvent(action);
 }
 
 bool AcceleratorController::CanHandleAccelerators() const {
@@ -1173,4 +1124,43 @@ AcceleratorController::GetAcceleratorProcessingRestriction(int action) {
   return RESTRICTION_NONE;
 }
 
+bool AcceleratorController::MaybeDeprecatedAcceleratorPressed(
+    AcceleratorAction action,
+    const ui::Accelerator& accelerator) const {
+  auto itr = actions_with_deprecations_.find(action);
+  if (itr == actions_with_deprecations_.end()) {
+    // The action is not associated with any deprecated accelerators, and hence
+    // should be performed normally.
+    return true;
+  }
+
+  // This action is associated with new and deprecated accelerators, find which
+  // one is |accelerator|.
+  const DeprecatedAcceleratorData* data = itr->second;
+  if (!deprecated_accelerators_.count(accelerator)) {
+    // This is a new accelerator replacing the old deprecated one.
+    // Record UMA stats and proceed normally to perform it.
+    RecordUmaHistogram(data->uma_histogram_name, NEW_USED);
+    return true;
+  }
+
+  // This accelerator has been deprecated and should be treated according
+  // to its |DeprecatedAcceleratorData|.
+
+  // Record UMA stats.
+  RecordUmaHistogram(data->uma_histogram_name, DEPRECATED_USED);
+
+  if (delegate_) {
+    // We always display the notification as long as this |data| entry exists.
+    delegate_->ShowDeprecatedAcceleratorNotification(
+        data->uma_histogram_name, data->notification_message_id,
+        data->old_shortcut_id, data->new_shortcut_id);
+  }
+
+  if (!data->deprecated_enabled)
+    return false;
+
+  return true;
+}
+
 }  // namespace ash