Fix keyboard accelerator registration issue in views::View.

View should not register the same accelerator target multiple times.

BUG=13275
TEST=None
Review URL: http://codereview.chromium.org/118242

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

4 files changed, 121 insertions, 18 deletions

diff --git a/views/focus/focus_manager.cc b/views/focus/focus_manager.cc
index bab4922..04258bf 100644
--- a/views/focus/focus_manager.cc
+++ b/views/focus/focus_manager.cc
@@ -638,10 +638,8 @@ void FocusManager::RegisterAccelerator(
     const Accelerator& accelerator,
     AcceleratorTarget* target) {
   AcceleratorTargetList& targets = accelerators_[accelerator];
-  // TODO(yutak): View::RegisterAccelerators() seems to register the same target
-  // multiple times. Should uncomment below after View is fixed.
-  // DCHECK(std::find(targets.begin(), targets.end(), target) == targets.end())
-  //     << "Registering the same target multiple times";
+  DCHECK(std::find(targets.begin(), targets.end(), target) == targets.end())
+      << "Registering the same target multiple times";
   targets.push_front(target);
 }
 
diff --git a/views/view.cc b/views/view.cc
index 1730bb9..985064cd 100644
--- a/views/view.cc
+++ b/views/view.cc
@@ -56,6 +56,7 @@ View::View()
       registered_for_visible_bounds_notification_(false),
       next_focusable_view_(NULL),
       previous_focusable_view_(NULL),
+      registered_accelerator_count_(0),
       context_menu_controller_(NULL),
 #if defined(OS_WIN)
       accessibility_(NULL),
@@ -652,7 +653,7 @@ void View::ViewHierarchyChangedImpl(bool register_accelerators,
     if (is_add) {
       // If you get this registration, you are part of a subtree that has been
       // added to the view hierarchy.
-      RegisterAccelerators();
+      RegisterPendingAccelerators();
     } else {
       if (child == this)
         UnregisterAccelerators();
@@ -923,8 +924,14 @@ void View::PrintFocusHierarchyImp(int indent) {
 void View::AddAccelerator(const Accelerator& accelerator) {
   if (!accelerators_.get())
     accelerators_.reset(new std::vector<Accelerator>());
+
+  std::vector<Accelerator>::iterator iter =
+      std::find(accelerators_->begin(), accelerators_->end(), accelerator);
+  DCHECK(iter == accelerators_->end())
+      << "Registering the same accelerator multiple times";
+
   accelerators_->push_back(accelerator);
-  RegisterAccelerators();
+  RegisterPendingAccelerators();
 }
 
 void View::RemoveAccelerator(const Accelerator& accelerator) {
@@ -936,7 +943,14 @@ void View::RemoveAccelerator(const Accelerator& accelerator) {
     return;
   }
 
+  int index = iter - accelerators_->begin();
   accelerators_->erase(iter);
+  if (index >= registered_accelerator_count_) {
+    // The accelerator is not registered to FocusManager.
+    return;
+  }
+  --registered_accelerator_count_;
+
   RootView* root_view = GetRootView();
   if (!root_view) {
     // We are not part of a view hierarchy, so there is nothing to do as we
@@ -958,16 +972,16 @@ void View::RemoveAccelerator(const Accelerator& accelerator) {
 }
 
 void View::ResetAccelerators() {
-  if (accelerators_.get()) {
+  if (accelerators_.get())
     UnregisterAccelerators();
-    accelerators_->clear();
-    accelerators_.reset();
-  }
 }
 
-void View::RegisterAccelerators() {
-  if (!accelerators_.get())
+void View::RegisterPendingAccelerators() {
+  if (!accelerators_.get() ||
+      registered_accelerator_count_ == accelerators_->size()) {
+    // No accelerators are waiting for registration.
     return;
+  }
 
   RootView* root_view = GetRootView();
   if (!root_view) {
@@ -986,10 +1000,12 @@ void View::RegisterAccelerators() {
     NOTREACHED();
     return;
   }
-  for (std::vector<Accelerator>::const_iterator iter = accelerators_->begin();
+  std::vector<Accelerator>::const_iterator iter;
+  for (iter = accelerators_->begin() + registered_accelerator_count_;
        iter != accelerators_->end(); ++iter) {
     focus_manager->RegisterAccelerator(*iter, this);
   }
+  registered_accelerator_count_ = accelerators_->size();
 #endif
 }
 
@@ -1008,6 +1024,9 @@ void View::UnregisterAccelerators() {
       // nothing to unregister.
       focus_manager->UnregisterAccelerators(this);
     }
+    accelerators_->clear();
+    accelerators_.reset();
+    registered_accelerator_count_ = 0;
 #endif
   }
 }
diff --git a/views/view.h b/views/view.h
index 03291c6..967ce22 100644
--- a/views/view.h
+++ b/views/view.h
@@ -1127,9 +1127,11 @@ class View : public AcceleratorTarget {
   void PrintViewHierarchyImp(int indent);
   void PrintFocusHierarchyImp(int indent);
 
-  // Registers/unregister this view's keyboard accelerators with the
-  // FocusManager.
-  void RegisterAccelerators();
+  // Registers this view's keyboard accelerators that are not registered to
+  // FocusManager yet, if possible.
+  void RegisterPendingAccelerators();
+
+  // Unregisters all the keyboard accelerators associated with this view.
   void UnregisterAccelerators();
 
   // This View's bounds in the parent coordinate system.
@@ -1174,8 +1176,11 @@ class View : public AcceleratorTarget {
   // Next view to be focused when the Shift-Tab key combination is pressed.
   View* previous_focusable_view_;
 
-  // The list of accelerators.
+  // The list of accelerators. List elements in the range
+  // [0, registered_accelerator_count_) are already registered to FocusManager,
+  // and the rest are not yet.
   scoped_ptr<std::vector<Accelerator> > accelerators_;
+  int registered_accelerator_count_;
 
   // The menu controller.
   ContextMenuController* context_menu_controller_;
diff --git a/views/view_unittest.cc b/views/view_unittest.cc
index cd0184e..91f3149 100644
--- a/views/view_unittest.cc
+++ b/views/view_unittest.cc
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <map>
+
 #include "app/gfx/canvas.h"
 #include "app/gfx/path.h"
 #include "base/clipboard.h"
@@ -109,7 +111,7 @@ class EmptyWindow : public CWindowImpl<EmptyWindow,
 ////////////////////////////////////////////////////////////////////////////////
 class TestView : public View {
  public:
-   TestView() : View(){
+  TestView() : View() {
   }
 
   virtual ~TestView() {}
@@ -123,6 +125,7 @@ class TestView : public View {
     location_.x = 0;
     location_.y = 0;
     last_clip_.setEmpty();
+    accelerator_count_map_.clear();
   }
 
   virtual void DidChangeBounds(const gfx::Rect& previous,
@@ -132,6 +135,7 @@ class TestView : public View {
   virtual bool OnMouseDragged(const MouseEvent& event);
   virtual void OnMouseReleased(const MouseEvent& event, bool canceled);
   virtual void Paint(gfx::Canvas* canvas);
+  virtual bool AcceleratorPressed(const Accelerator& accelerator);
 
   // DidChangeBounds test
   bool did_change_bounds_;
@@ -150,6 +154,9 @@ class TestView : public View {
 
   // Painting
   SkRect last_clip_;
+
+  // Accelerators
+  std::map<Accelerator, int> accelerator_count_map_;
 };
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -702,6 +709,80 @@ TEST_F(ViewTest, TextfieldCutCopyPaste) {
 }
 #endif
 
+////////////////////////////////////////////////////////////////////////////////
+// Accelerators
+////////////////////////////////////////////////////////////////////////////////
+bool TestView::AcceleratorPressed(const Accelerator& accelerator) {
+  accelerator_count_map_[accelerator]++;
+  return true;
+}
+
+#if defined(OS_WIN)
+TEST_F(ViewTest, ActivateAccelerator) {
+  // Register a keyboard accelerator before the view is added to a window.
+  views::Accelerator return_accelerator(VK_RETURN, false, false, false);
+  TestView* view = new TestView();
+  view->Reset();
+  view->AddAccelerator(return_accelerator);
+  EXPECT_EQ(view->accelerator_count_map_[return_accelerator], 0);
+
+  // Create a window and add the view as its child.
+  WidgetWin window;
+  window.Init(NULL, gfx::Rect(0, 0, 100, 100), true);
+  window.set_delete_on_destroy(false);
+  window.set_window_style(WS_OVERLAPPEDWINDOW);
+  RootView* root = window.GetRootView();
+  root->AddChildView(view);
+
+  // Get the focus manager.
+  views::FocusManager* focus_manager =
+      views::FocusManager::GetFocusManager(window.GetNativeView());
+  ASSERT_TRUE(focus_manager);
+
+  // Hit the return key and see if it takes effect.
+  EXPECT_TRUE(focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(view->accelerator_count_map_[return_accelerator], 1);
+
+  // Hit the escape key. Nothing should happen.
+  views::Accelerator escape_accelerator(VK_ESCAPE, false, false, false);
+  EXPECT_FALSE(focus_manager->ProcessAccelerator(escape_accelerator));
+  EXPECT_EQ(view->accelerator_count_map_[return_accelerator], 1);
+  EXPECT_EQ(view->accelerator_count_map_[escape_accelerator], 0);
+
+  // Now register the escape key and hit it again.
+  view->AddAccelerator(escape_accelerator);
+  EXPECT_TRUE(focus_manager->ProcessAccelerator(escape_accelerator));
+  EXPECT_EQ(view->accelerator_count_map_[return_accelerator], 1);
+  EXPECT_EQ(view->accelerator_count_map_[escape_accelerator], 1);
+
+  // Remove the return key accelerator.
+  view->RemoveAccelerator(return_accelerator);
+  EXPECT_FALSE(focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(view->accelerator_count_map_[return_accelerator], 1);
+  EXPECT_EQ(view->accelerator_count_map_[escape_accelerator], 1);
+
+  // Add it again. Hit the return key and the escape key.
+  view->AddAccelerator(return_accelerator);
+  EXPECT_TRUE(focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(view->accelerator_count_map_[return_accelerator], 2);
+  EXPECT_EQ(view->accelerator_count_map_[escape_accelerator], 1);
+  EXPECT_TRUE(focus_manager->ProcessAccelerator(escape_accelerator));
+  EXPECT_EQ(view->accelerator_count_map_[return_accelerator], 2);
+  EXPECT_EQ(view->accelerator_count_map_[escape_accelerator], 2);
+
+  // Remove all the accelerators.
+  view->ResetAccelerators();
+  EXPECT_FALSE(focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(view->accelerator_count_map_[return_accelerator], 2);
+  EXPECT_EQ(view->accelerator_count_map_[escape_accelerator], 2);
+  EXPECT_FALSE(focus_manager->ProcessAccelerator(escape_accelerator));
+  EXPECT_EQ(view->accelerator_count_map_[return_accelerator], 2);
+  EXPECT_EQ(view->accelerator_count_map_[escape_accelerator], 2);
+
+  window.CloseNow();
+}
+#endif
+
 #if defined(OS_WIN)
 ////////////////////////////////////////////////////////////////////////////////
 // Mouse-wheel message rerouting