Regression: Search+Key pops up app launcher

ash/accelerators/accelerator_controller.cc

Index: ash/accelerators/accelerator_controller.cc
diff --git a/ash/accelerators/accelerator_controller.cc b/ash/accelerators/accelerator_controller.cc
index aefbdbd5b483306cf83e1970767380b024f51afb..5884bdca55ddc61b47e1a26041c890c75b16805f 100644
--- a/ash/accelerators/accelerator_controller.cc
+++ b/ash/accelerators/accelerator_controller.cc
@@ -468,20 +468,22 @@ bool HandleToggleAppList(ui::KeyboardCode key_code,
   // being pressed and released, then ignore the release of the
   // Search key.
   if (key_code == ui::VKEY_LWIN &&
-      (previous_event_type == ui::ET_KEY_RELEASED ||
-       previous_key_code != ui::VKEY_LWIN))
-    return false;

[Jun Mukai, 2014/11/14 02:09:48]

Please keep early-exit code if you don't have to.  Seems conditions are not
changed.  Early-exit reduces indent depth and makes code concise.

-  if (key_code == ui::VKEY_LWIN)

[Jun Mukai, 2014/11/14 02:09:48]

What is the motivation of removing this condition?

[afakhry, 2014/11/14 17:30:57]

Because in my code, I already made sure that key_code == ui::VKEY_LWIN. No need
to redo the check.

+      previous_event_type == ui::ET_KEY_PRESSED &&
+      previous_key_code == ui::VKEY_LWIN) {
     base::RecordAction(base::UserMetricsAction("Accel_Search_LWin"));
-  // When spoken feedback is enabled, we should neither toggle the list nor
-  // consume the key since Search+Shift is one of the shortcuts the a11y
-  // feature uses. crbug.com/132296
-  DCHECK_EQ(ui::VKEY_LWIN, accelerator.key_code());
-  if (Shell::GetInstance()->accessibility_delegate()->
-      IsSpokenFeedbackEnabled())
-    return false;
-  ash::Shell::GetInstance()->ToggleAppList(NULL);
-  return true;
+
+    // When spoken feedback is enabled, we should neither toggle the list nor
+    // consume the key since Search+Shift is one of the shortcuts the a11y
+    // feature uses. crbug.com/132296
+    DCHECK_EQ(ui::VKEY_LWIN, accelerator.key_code());
+    if (Shell::GetInstance()->accessibility_delegate()->
+        IsSpokenFeedbackEnabled())
+      return false;
+    ash::Shell::GetInstance()->ToggleAppList(NULL);
+    return true;
+  }
+
+  return false;
 }
 
 bool HandleToggleFullscreen(ui::KeyboardCode key_code) {
@@ -659,12 +661,15 @@ bool HandleToggleCapsLock(ui::KeyboardCode key_code,
 
 class AutoSet {
  public:
-  AutoSet(ui::Accelerator* scoped, ui::Accelerator new_value)
-      : scoped_(scoped), new_value_(new_value) {}
-  ~AutoSet() { *scoped_ = new_value_; }
+  AutoSet(ui::AcceleratorEvent* scoped, ui::Accelerator new_value)
+      : scoped_(scoped), new_value_(new_value) {
+  }
+  ~AutoSet() {
+    *scoped_ = ui::AcceleratorEvent(new_value_);
+  }
 
  private:
-  ui::Accelerator* scoped_;
+  ui::AcceleratorEvent* scoped_;
   const ui::Accelerator new_value_;
 
   DISALLOW_COPY_AND_ASSIGN(AutoSet);
@@ -684,7 +689,7 @@ AcceleratorController::~AcceleratorController() {
 }
 
 void AcceleratorController::Init() {
-  previous_accelerator_.set_type(ui::ET_UNKNOWN);
+  previous_accelerator_.accelerator.set_type(ui::ET_UNKNOWN);
   for (size_t i = 0; i < kActionsAllowedAtLoginOrLockScreenLength; ++i) {
     actions_allowed_at_login_screen_.insert(
         kActionsAllowedAtLoginOrLockScreen[i]);
@@ -798,8 +803,9 @@ bool AcceleratorController::PerformAction(int action,
     return true;
   }
   // Type of the previous accelerator. Used by NEXT_IME and DISABLE_CAPS_LOCK.
-  const ui::EventType previous_event_type = previous_accelerator_.type();
-  const ui::KeyboardCode previous_key_code = previous_accelerator_.key_code();
+  ui::Accelerator accel = GetPreviousAccelerator();
+  const ui::EventType previous_event_type = accel.type();
+  const ui::KeyboardCode previous_key_code = accel.key_code();
 
   // You *MUST* return true when some action is performed. Otherwise, this
   // function might be called *twice*, via BrowserView::PreHandleKeyboardEvent
@@ -1109,6 +1115,14 @@ void AcceleratorController::SetKeyboardBrightnessControlDelegate(
       keyboard_brightness_control_delegate.Pass();
 }
 
+const ui::Accelerator& AcceleratorController::GetPreviousAccelerator() const {

[Jun Mukai, 2014/11/14 02:09:48]

Why do we keep two different previous accelerators?

[afakhry, 2014/11/14 17:30:57]

This is needed for some unit tests that directly calls
AcceleratorController::Process() without going through the events system.

+  const ui::AcceleratorEvent& accel =
+      ui::AcceleratorHistory::GetInstance()->GetPreviousAccelerator();
+  if (accel.time_stamp > previous_accelerator_.time_stamp)
+    return accel.accelerator;
+  return previous_accelerator_.accelerator;
+}
+
 bool AcceleratorController::CanHandleAccelerators() const {
   return true;
 }