This CL make the AcceleratorDispatcher eat the WM_KEYUP associated to

WM_KEYDOWN that resulted in an accelerator, as they would cause orphaned
KeyUp events to be sent to views.
This is Windows only for now, still needs to be ported to Linux.

BUG=23383
TEST=Run unit-tests. Ensure accelerators work as expected in Chrome.
Review URL: http://codereview.chromium.org/246074

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@28096 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 158 insertions, 5 deletions

diff --git a/views/focus/accelerator_handler.h b/views/focus/accelerator_handler.h
index 618f541..c440c6b 100644
--- a/views/focus/accelerator_handler.h
+++ b/views/focus/accelerator_handler.h
@@ -5,10 +5,16 @@
 #ifndef VIEWS_FOCUS_ACCELERATOR_HANDLER_H_
 #define VIEWS_FOCUS_ACCELERATOR_HANDLER_H_
 
+#include "build/build_config.h"
+
 #if defined(OS_LINUX)
 #include <gdk/gdk.h>
 #endif
 
+#if defined(OS_WIN)
+#include <set>
+#endif
+
 #include "base/message_loop.h"
 
 namespace views {
@@ -27,8 +33,10 @@ class AcceleratorHandler : public MessageLoopForUI::Dispatcher {
 #endif
 
  private:
-#if defined(OS_LINUX)
-  // Last key pressed and consumed as an accelerator.
+#if defined(OS_WIN)
+  // The keys currently pressed and consumed by the FocusManager.
+  std::set<WPARAM> pressed_keys_;
+#else  // OS_LINUX
   guint last_key_pressed_;
 #endif
 
diff --git a/views/focus/accelerator_handler_win.cc b/views/focus/accelerator_handler_win.cc
index f617c6f..ee5c5b7 100644
--- a/views/focus/accelerator_handler_win.cc
+++ b/views/focus/accelerator_handler_win.cc
@@ -29,9 +29,22 @@ bool AcceleratorHandler::Dispatch(const MSG& msg) {
                          msg.lParam & 0xFFFF,
                          (msg.lParam & 0xFFFF0000) >> 16);
           process_message = focus_manager->OnKeyEvent(event);
-          // TODO(jcampan): http://crbug.com/23383 We should not translate and
-          //                dispatch the associated WM_KEYUP if process_message
-          //                is true.
+          if (!process_message) {
+            // Record that this key is pressed so we can remember not to
+            // translate and dispatch the associated WM_KEYUP.
+            pressed_keys_.insert(msg.wParam);
+          }
+          break;
+        }
+        case WM_KEYUP:
+        case WM_SYSKEYUP: {
+          std::set<WPARAM>::iterator iter = pressed_keys_.find(msg.wParam);
+          if (iter != pressed_keys_.end()) {
+            // Don't translate/dispatch the KEYUP since we have eaten the
+            // associated KEYDOWN.
+            pressed_keys_.erase(iter);
+            return true;
+          }
           break;
         }
       }
diff --git a/views/focus/focus_manager_unittest.cc b/views/focus/focus_manager_unittest.cc
index 63ba287..40abff6 100644
--- a/views/focus/focus_manager_unittest.cc
+++ b/views/focus/focus_manager_unittest.cc
@@ -208,6 +208,16 @@ class FocusManagerTest : public testing::Test, public WindowDelegate {
     GetFocusManager()->AddFocusChangeListener(listener);
   }
 
+#if defined(OS_WIN)
+  void PostKeyDown(base::KeyboardCode key_code) {
+    ::PostMessage(window_->GetNativeWindow(), WM_KEYDOWN, key_code, 0);
+  }
+
+  void PostKeyUp(base::KeyboardCode key_code) {
+    ::PostMessage(window_->GetNativeWindow(), WM_KEYUP, key_code, 0);
+  }
+#endif
+
  private:
   FocusChangeListener* focus_change_listener_;
   MessageLoopForUI message_loop_;
@@ -1170,4 +1180,126 @@ TEST_F(FocusManagerTest, CallsSelfDeletingAcceleratorTarget) {
   EXPECT_EQ(target.accelerator_count(), 1);
 }
 
+class MessageTrackingView : public View {
+ public:
+  MessageTrackingView() : accelerator_pressed_(false) {
+ }
+
+  virtual bool OnKeyPressed(const KeyEvent& e) {
+    keys_pressed_.push_back(e.GetKeyCode());
+    return true;
+  }
+
+  virtual bool OnKeyReleased(const KeyEvent& e) {
+    keys_released_.push_back(e.GetKeyCode());
+    return true;
+  }
+
+  virtual bool AcceleratorPressed(const Accelerator& accelerator) {
+    accelerator_pressed_ = true;
+    return true;
+  }
+
+  void Reset() {
+    accelerator_pressed_ = false;
+    keys_pressed_.clear();
+    keys_released_.clear();
+  }
+
+  const std::vector<base::KeyboardCode>& keys_pressed() const {
+    return keys_pressed_;
+  }
+
+  const std::vector<base::KeyboardCode>& keys_released() const {
+    return keys_released_;
+  }
+
+  bool accelerator_pressed() const {
+    return accelerator_pressed_;
+  }
+
+ private:
+  bool accelerator_pressed_;
+  std::vector<base::KeyboardCode> keys_pressed_;
+  std::vector<base::KeyboardCode> keys_released_;
+
+  DISALLOW_COPY_AND_ASSIGN(MessageTrackingView);
+};
+
+#if defined(OS_WIN)
+// Tests that the keyup messages are eaten for accelerators.
+TEST_F(FocusManagerTest, IgnoreKeyupForAccelerators) {
+  FocusManager* focus_manager = GetFocusManager();
+  MessageTrackingView* mtv = new MessageTrackingView();
+  mtv->AddAccelerator(Accelerator(base::VKEY_0, false, false, false));
+  mtv->AddAccelerator(Accelerator(base::VKEY_1, false, false, false));
+  content_view_->AddChildView(mtv);
+  focus_manager->SetFocusedView(mtv);
+
+  // First send a non-accelerator key sequence.
+  PostKeyDown(base::VKEY_9);
+  PostKeyUp(base::VKEY_9);
+  AcceleratorHandler accelerator_handler;
+  MessageLoopForUI::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+  MessageLoopForUI::current()->Run(&accelerator_handler);
+  // Make sure we get a key-up and key-down.
+  ASSERT_EQ(1, mtv->keys_pressed().size());
+  EXPECT_EQ(base::VKEY_9, mtv->keys_pressed().at(0));
+  ASSERT_EQ(1, mtv->keys_released().size());
+  EXPECT_EQ(base::VKEY_9, mtv->keys_released().at(0));
+  EXPECT_FALSE(mtv->accelerator_pressed());
+  mtv->Reset();
+
+  // Same thing with repeat and more than one key at once.
+  PostKeyDown(base::VKEY_9);
+  PostKeyDown(base::VKEY_9);
+  PostKeyDown(base::VKEY_8);
+  PostKeyDown(base::VKEY_9);
+  PostKeyDown(base::VKEY_7);
+  PostKeyUp(base::VKEY_9);
+  PostKeyUp(base::VKEY_7);
+  PostKeyUp(base::VKEY_8);
+  MessageLoopForUI::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+  MessageLoopForUI::current()->Run(&accelerator_handler);
+  // Make sure we get a key-up and key-down.
+  ASSERT_EQ(5, mtv->keys_pressed().size());
+  EXPECT_EQ(base::VKEY_9, mtv->keys_pressed().at(0));
+  EXPECT_EQ(base::VKEY_9, mtv->keys_pressed().at(1));
+  EXPECT_EQ(base::VKEY_8, mtv->keys_pressed().at(2));
+  EXPECT_EQ(base::VKEY_9, mtv->keys_pressed().at(3));
+  EXPECT_EQ(base::VKEY_7, mtv->keys_pressed().at(4));
+  ASSERT_EQ(3, mtv->keys_released().size());
+  EXPECT_EQ(base::VKEY_9, mtv->keys_released().at(0));
+  EXPECT_EQ(base::VKEY_7, mtv->keys_released().at(1));
+  EXPECT_EQ(base::VKEY_8, mtv->keys_released().at(2));
+  EXPECT_FALSE(mtv->accelerator_pressed());
+  mtv->Reset();
+
+  // Now send an accelerator key sequence.
+  PostKeyDown(base::VKEY_0);
+  PostKeyUp(base::VKEY_0);
+  MessageLoopForUI::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+  MessageLoopForUI::current()->Run(&accelerator_handler);
+  EXPECT_TRUE(mtv->keys_pressed().empty());
+  EXPECT_TRUE(mtv->keys_released().empty());
+  EXPECT_TRUE(mtv->accelerator_pressed());
+  mtv->Reset();
+
+  // Same thing with repeat and more than one key at once.
+  PostKeyDown(base::VKEY_0);
+  PostKeyDown(base::VKEY_1);
+  PostKeyDown(base::VKEY_1);
+  PostKeyDown(base::VKEY_0);
+  PostKeyDown(base::VKEY_0);
+  PostKeyUp(base::VKEY_1);
+  PostKeyUp(base::VKEY_0);
+  MessageLoopForUI::current()->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+  MessageLoopForUI::current()->Run(&accelerator_handler);
+  EXPECT_TRUE(mtv->keys_pressed().empty());
+  EXPECT_TRUE(mtv->keys_released().empty());
+  EXPECT_TRUE(mtv->accelerator_pressed());
+  mtv->Reset();
+}
+#endif
+
 }  // namespace views