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..4cba6047eee6cefa941a44bae38a0dcf9a54651f 100644
--- a/ash/accelerators/accelerator_controller.cc
+++ b/ash/accelerators/accelerator_controller.cc
@@ -467,12 +467,14 @@ bool HandleToggleAppList(ui::KeyboardCode key_code,
   // If something else was pressed between the Search key (LWIN)
   // 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))
+  if (key_code != ui::VKEY_LWIN ||
+      previous_event_type != ui::ET_KEY_PRESSED ||
+      previous_key_code != ui::VKEY_LWIN ||
+      accelerator.type() != ui::ET_KEY_RELEASED)

[Jun Mukai, 2014/11/15 01:56:33]

This means accelerator is not handled at all if key code is not VKEY_LWIN.  Is
that true?
accelerator_table has a mapping of VKEY_BROWSER_SEARCH.  Isn't it currently
used?

[Jun Mukai, 2014/11/15 01:56:33]

This was weird in the previous code.  Multi-lined if should have curly braces.
Could you add that?

[afakhry, 2014/11/15 02:40:30]

Done.

[afakhry, 2014/11/15 02:40:31]

I added a test for VKEY_BROWSER_SEARCH and pressed.

     return false;
-  if (key_code == ui::VKEY_LWIN)
-    base::RecordAction(base::UserMetricsAction("Accel_Search_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
@@ -657,19 +659,6 @@ bool HandleToggleCapsLock(ui::KeyboardCode key_code,
 
 #endif  // defined(OS_CHROMEOS)
 
-class AutoSet {
- public:
-  AutoSet(ui::Accelerator* scoped, ui::Accelerator new_value)
-      : scoped_(scoped), new_value_(new_value) {}
-  ~AutoSet() { *scoped_ = new_value_; }
-
- private:
-  ui::Accelerator* scoped_;
-  const ui::Accelerator new_value_;
-
-  DISALLOW_COPY_AND_ASSIGN(AutoSet);
-};
-
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -684,7 +673,6 @@ AcceleratorController::~AcceleratorController() {
 }
 
 void AcceleratorController::Init() {
-  previous_accelerator_.set_type(ui::ET_UNKNOWN);
   for (size_t i = 0; i < kActionsAllowedAtLoginOrLockScreenLength; ++i) {
     actions_allowed_at_login_screen_.insert(
         kActionsAllowedAtLoginOrLockScreen[i]);
@@ -738,8 +726,6 @@ void AcceleratorController::UnregisterAll(ui::AcceleratorTarget* target) {
 }
 
 bool AcceleratorController::Process(const ui::Accelerator& accelerator) {
-  AutoSet auto_set(&previous_accelerator_, accelerator);
-
   if (ime_control_delegate_) {
     return accelerator_manager_->Process(
         ime_control_delegate_->RemapAccelerator(accelerator));
@@ -798,8 +784,10 @@ 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();
+  const ui::Accelerator& previous_accelerator =
+      ui::AcceleratorHistory::GetInstance()->GetPreviousAccelerator();
+  const ui::EventType previous_event_type = previous_accelerator.type();
+  const ui::KeyboardCode previous_key_code = previous_accelerator.key_code();
 
   // You *MUST* return true when some action is performed. Otherwise, this
   // function might be called *twice*, via BrowserView::PreHandleKeyboardEvent