Remove the deprecated Alt+Shift shortcut completely

ash/common/accelerators/accelerator_controller.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/common/accelerators/accelerator_controller.h"
 
 #include <utility>
 
 #include "ash/common/accelerators/accelerator_commands.h"
 #include "ash/common/accelerators/accelerator_controller_delegate.h"
@@ skipping 155 matching lines @@
   // Touchscreen users have better window switching options. It would be
   // preferable if we could tell whether this event actually came from a virtual
   // keyboard, but there's no easy way to do so, thus we block Alt+Tab when the
   // virtual keyboard is showing, even if it came from a real keyboard. See
   // http://crbug.com/638269
   keyboard::KeyboardController* keyboard_controller =
       keyboard::KeyboardController::GetInstance();
   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();
 }
 
 void HandleOpenFeedbackPage() {
   base::RecordAction(UserMetricsAction("Accel_Open_Feedback_Page"));
   WmShell::Get()->new_window_controller()->OpenFeedbackPage();
 }
 
@@ skipping 422 matching lines @@
 
 ////////////////////////////////////////////////////////////////////////////////
 // AcceleratorController, ui::AcceleratorTarget implementation:
 
 bool AcceleratorController::AcceleratorPressed(
     const ui::Accelerator& accelerator) {
   std::map<ui::Accelerator, AcceleratorAction>::const_iterator it =
       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|.
+  if (!CanPerformAction(action, accelerator))
+    return false;
 
-        // Record UMA stats.
-        RecordUmaHistogram(data->uma_histogram_name, DEPRECATED_USED);
+  // 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;
 
-        // 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);
-      }
-    }
-
-    PerformAction(action, accelerator);
-    return ShouldActionConsumeKeyEvent(action);
-  }
-  return false;
+  PerformAction(action, accelerator);
+  return ShouldActionConsumeKeyEvent(action);
 }
 
 bool AcceleratorController::CanHandleAccelerators() const {
   return true;
 }
 
 void AcceleratorController::BindRequest(
     mojom::AcceleratorControllerRequest request) {
   bindings_.AddBinding(this, std::move(request));
 }
@@ skipping 479 matching lines @@
   }
   if (wm_shell->mru_window_tracker()->BuildMruWindowList().empty() &&
       actions_needing_window_.find(action) != actions_needing_window_.end()) {
     wm_shell->accessibility_delegate()->TriggerAccessibilityAlert(
         A11Y_ALERT_WINDOW_NEEDED);
     return RESTRICTION_PREVENT_PROCESSING_AND_PROPAGATION;
   }
   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