FocusManager should accept multiple AcceleratorTargets for each accelerator.

Originally, FocusManager automatically unregisters an old target if multiple AcceleratorTargets are registered to the same accelerator. This behavior is somewhat troublesome, and actually ShelfItemDialog hits a run-time assertion due to the conflict of registrations (issue 12401). This change modifies the behavior of FocusManager to allow multiple targets to be registered for each accelerator.

BUG=12401
TEST=See if issue 12401 is resolved.
Review URL: http://codereview.chromium.org/114065

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

3 files changed, 221 insertions, 46 deletions

diff --git a/views/focus/focus_manager.cc b/views/focus/focus_manager.cc
index 4cf6253..f504fd0 100644
--- a/views/focus/focus_manager.cc
+++ b/views/focus/focus_manager.cc
@@ -638,67 +638,73 @@ View* FocusManager::FindFocusableView(FocusTraversable* focus_traversable,
   return v;
 }
 
-AcceleratorTarget* FocusManager::RegisterAccelerator(
+void FocusManager::RegisterAccelerator(
     const Accelerator& accelerator,
     AcceleratorTarget* target) {
-  AcceleratorMap::const_iterator iter = accelerators_.find(accelerator);
-  AcceleratorTarget* previous_target = NULL;
-  if (iter != accelerators_.end())
-    previous_target = iter->second;
-
-  accelerators_[accelerator] = target;
-
-  return previous_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";
+  targets.push_front(target);
 }
 
 void FocusManager::UnregisterAccelerator(const Accelerator& accelerator,
                                          AcceleratorTarget* target) {
-  AcceleratorMap::iterator iter = accelerators_.find(accelerator);
-  if (iter == accelerators_.end()) {
+  AcceleratorMap::iterator map_iter = accelerators_.find(accelerator);
+  if (map_iter == accelerators_.end()) {
     NOTREACHED() << "Unregistering non-existing accelerator";
     return;
   }
 
-  if (iter->second != target) {
+  AcceleratorTargetList* targets = &map_iter->second;
+  AcceleratorTargetList::iterator target_iter =
+      std::find(targets->begin(), targets->end(), target);
+  if (target_iter == targets->end()) {
     NOTREACHED() << "Unregistering accelerator for wrong target";
     return;
   }
 
-  accelerators_.erase(iter);
+  targets->erase(target_iter);
 }
 
 void FocusManager::UnregisterAccelerators(AcceleratorTarget* target) {
-  for (AcceleratorMap::iterator iter = accelerators_.begin();
-       iter != accelerators_.end();) {
-    if (iter->second == target)
-      accelerators_.erase(iter++);
-    else
-      ++iter;
+  for (AcceleratorMap::iterator map_iter = accelerators_.begin();
+       map_iter != accelerators_.end(); ++map_iter) {
+    AcceleratorTargetList* targets = &map_iter->second;
+    targets->remove(target);
   }
 }
 
 bool FocusManager::ProcessAccelerator(const Accelerator& accelerator) {
-  FocusManager* focus_manager = this;
-  do {
-    AcceleratorTarget* target =
-        focus_manager->GetTargetForAccelerator(accelerator);
-    if (target && target->AcceleratorPressed(accelerator))
-      return true;
+  AcceleratorMap::iterator map_iter = accelerators_.find(accelerator);
+  if (map_iter != accelerators_.end()) {
+    // We have to copy the target list here, because an AcceleratorPressed
+    // event handler may modify the list.
+    AcceleratorTargetList targets(map_iter->second);
+    for (AcceleratorTargetList::iterator iter = targets.begin();
+         iter != targets.end(); ++iter) {
+      if ((*iter)->AcceleratorPressed(accelerator))
+        return true;
+    }
+  }
+
+  // When dealing with child windows that have their own FocusManager (such
+  // as ConstrainedWindow), we still want the parent FocusManager to process
+  // the accelerator if the child window did not process it.
+  FocusManager* parent_focus_manager = GetParentFocusManager();
+  if (parent_focus_manager)
+    return parent_focus_manager->ProcessAccelerator(accelerator);
 
-    // When dealing with child windows that have their own FocusManager (such
-    // as ConstrainedWindow), we still want the parent FocusManager to process
-    // the accelerator if the child window did not process it.
-    focus_manager = focus_manager->GetParentFocusManager();
-  } while (focus_manager);
   return false;
 }
 
-AcceleratorTarget* FocusManager::GetTargetForAccelerator(
-    const Accelerator& accelerator) const {
-  AcceleratorMap::const_iterator iter = accelerators_.find(accelerator);
-  if (iter != accelerators_.end())
-    return iter->second;
-  return NULL;
+AcceleratorTarget* FocusManager::GetCurrentTargetForAccelerator(
+    const views::Accelerator& accelerator) const {
+  AcceleratorMap::const_iterator map_iter = accelerators_.find(accelerator);
+  if (map_iter == accelerators_.end() || map_iter->second.empty())
+    return NULL;
+  return map_iter->second.front();
 }
 
 // static
diff --git a/views/focus/focus_manager.h b/views/focus/focus_manager.h
index 133c027..28abcde 100644
--- a/views/focus/focus_manager.h
+++ b/views/focus/focus_manager.h
@@ -10,6 +10,7 @@
 #endif
 #include <vector>
 #include <map>
+#include <list>
 
 #include "base/basictypes.h"
 #include "base/gfx/native_widget_types.h"
@@ -242,17 +243,17 @@ class FocusManager {
   // their own FocusManager and need to return focus to the browser when closed.
   FocusManager* GetParentFocusManager() const;
 
-  // Register a keyboard accelerator for the specified target.  If an
-  // AcceleratorTarget is already registered for that accelerator, it is
-  // returned.
+  // Register a keyboard accelerator for the specified target. If multiple
+  // targets are registered for an accelerator, a target registered later has
+  // higher priority.
   // Note that we are currently limited to accelerators that are either:
   // - a key combination including Ctrl or Alt
   // - the escape key
   // - the enter key
   // - any F key (F1, F2, F3 ...)
   // - any browser specific keys (as available on special keyboards)
-  AcceleratorTarget* RegisterAccelerator(const Accelerator& accelerator,
-                                         AcceleratorTarget* target);
+  void RegisterAccelerator(const Accelerator& accelerator,
+                           AcceleratorTarget* target);
 
   // Unregister the specified keyboard accelerator for the specified target.
   void UnregisterAccelerator(const Accelerator& accelerator,
@@ -261,7 +262,11 @@ class FocusManager {
   // Unregister all keyboard accelerator for the specified target.
   void UnregisterAccelerators(AcceleratorTarget* target);
 
-  // Activate the target associated with the specified accelerator if any.
+  // Activate the target associated with the specified accelerator.
+  // First, AcceleratorPressed handler of the most recently registered target
+  // is called, and if that handler processes the event (i.e. returns true),
+  // this method immediately returns. If not, we do the same thing on the next
+  // target, and so on.
   // Returns true if an accelerator was activated.
   bool ProcessAccelerator(const Accelerator& accelerator);
 
@@ -281,9 +286,8 @@ class FocusManager {
   // Returns the AcceleratorTarget that should be activated for the specified
   // keyboard accelerator, or NULL if no view is registered for that keyboard
   // accelerator.
-  // TODO(finnur): http://b/1307173 Make this private once the bug is fixed.
-  AcceleratorTarget* GetTargetForAccelerator(
-      const Accelerator& accelerator) const;
+  AcceleratorTarget* GetCurrentTargetForAccelerator(
+      const Accelerator& accelertor) const;
 
   // Convenience method that returns true if the passed |key_event| should
   // trigger tab traversal (if it is a TAB key press with or without SHIFT
@@ -329,7 +333,8 @@ class FocusManager {
   bool ignore_set_focus_msg_;
 
   // The accelerators and associated targets.
-  typedef std::map<Accelerator, AcceleratorTarget*> AcceleratorMap;
+  typedef std::list<AcceleratorTarget*> AcceleratorTargetList;
+  typedef std::map<Accelerator, AcceleratorTargetList> AcceleratorMap;
   AcceleratorMap accelerators_;
 
   // The list of registered keystroke listeners
diff --git a/views/focus/focus_manager_unittest.cc b/views/focus/focus_manager_unittest.cc
index 087ecfe..404535c 100644
--- a/views/focus/focus_manager_unittest.cc
+++ b/views/focus/focus_manager_unittest.cc
@@ -682,4 +682,168 @@ TEST_F(FocusManagerTest, TraversalWithNonEnabledViews) {
   */
 }
 
+// Counts accelerator calls.
+class TestAcceleratorTarget : public views::AcceleratorTarget {
+ public:
+  explicit TestAcceleratorTarget(bool process_accelerator)
+      : accelerator_count_(0), process_accelerator_(process_accelerator) {}
+
+  virtual bool AcceleratorPressed(const views::Accelerator& accelerator) {
+    ++accelerator_count_;
+    return process_accelerator_;
+  }
+
+  int accelerator_count() const { return accelerator_count_; }
+
+ private:
+  int accelerator_count_;  // number of times that the accelerator is activated
+  bool process_accelerator_;  // return value of AcceleratorPressed
+
+  DISALLOW_COPY_AND_ASSIGN(TestAcceleratorTarget);
+};
+
+TEST_F(FocusManagerTest, CallsNormalAcceleratorTarget) {
+  views::FocusManager* focus_manager =
+      views::FocusManager::GetFocusManager(test_window_->GetNativeView());
+  views::Accelerator return_accelerator(VK_RETURN, false, false, false);
+  views::Accelerator escape_accelerator(VK_ESCAPE, false, false, false);
+
+  TestAcceleratorTarget return_target(true);
+  TestAcceleratorTarget escape_target(true);
+  EXPECT_EQ(return_target.accelerator_count(), 0);
+  EXPECT_EQ(escape_target.accelerator_count(), 0);
+  EXPECT_EQ(NULL,
+            focus_manager->GetCurrentTargetForAccelerator(return_accelerator));
+  EXPECT_EQ(NULL,
+            focus_manager->GetCurrentTargetForAccelerator(escape_accelerator));
+
+  // Register targets.
+  focus_manager->RegisterAccelerator(return_accelerator, &return_target);
+  focus_manager->RegisterAccelerator(escape_accelerator, &escape_target);
+
+  // Checks if the correct target is registered.
+  EXPECT_EQ(&return_target,
+            focus_manager->GetCurrentTargetForAccelerator(return_accelerator));
+  EXPECT_EQ(&escape_target,
+            focus_manager->GetCurrentTargetForAccelerator(escape_accelerator));
+
+  // Hitting the return key.
+  EXPECT_TRUE(focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(return_target.accelerator_count(), 1);
+  EXPECT_EQ(escape_target.accelerator_count(), 0);
+
+  // Hitting the escape key.
+  EXPECT_TRUE(focus_manager->ProcessAccelerator(escape_accelerator));
+  EXPECT_EQ(return_target.accelerator_count(), 1);
+  EXPECT_EQ(escape_target.accelerator_count(), 1);
+
+  // Register another target for the return key.
+  TestAcceleratorTarget return_target2(true);
+  EXPECT_EQ(return_target2.accelerator_count(), 0);
+  focus_manager->RegisterAccelerator(return_accelerator, &return_target2);
+  EXPECT_EQ(&return_target2,
+            focus_manager->GetCurrentTargetForAccelerator(return_accelerator));
+
+  // Hitting the return key; return_target2 has the priority.
+  EXPECT_TRUE(focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(return_target.accelerator_count(), 1);
+  EXPECT_EQ(return_target2.accelerator_count(), 1);
+
+  // Register a target that does not process the accelerator event.
+  TestAcceleratorTarget return_target3(false);
+  EXPECT_EQ(return_target3.accelerator_count(), 0);
+  focus_manager->RegisterAccelerator(return_accelerator, &return_target3);
+  EXPECT_EQ(&return_target3,
+            focus_manager->GetCurrentTargetForAccelerator(return_accelerator));
+
+  // Hitting the return key.
+  // Since the event handler of return_target3 returns false, return_target2
+  // should be called too.
+  EXPECT_TRUE(focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(return_target.accelerator_count(), 1);
+  EXPECT_EQ(return_target2.accelerator_count(), 2);
+  EXPECT_EQ(return_target3.accelerator_count(), 1);
+
+  // Unregister return_target2.
+  focus_manager->UnregisterAccelerator(return_accelerator, &return_target2);
+  EXPECT_EQ(&return_target3,
+            focus_manager->GetCurrentTargetForAccelerator(return_accelerator));
+
+  // Hitting the return key. return_target3 and return_target should be called.
+  EXPECT_TRUE(focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(return_target.accelerator_count(), 2);
+  EXPECT_EQ(return_target2.accelerator_count(), 2);
+  EXPECT_EQ(return_target3.accelerator_count(), 2);
+
+  // Unregister targets.
+  focus_manager->UnregisterAccelerator(return_accelerator, &return_target);
+  focus_manager->UnregisterAccelerator(return_accelerator, &return_target3);
+  focus_manager->UnregisterAccelerator(escape_accelerator, &escape_target);
+
+  // Now there is no target registered.
+  EXPECT_EQ(NULL,
+            focus_manager->GetCurrentTargetForAccelerator(return_accelerator));
+  EXPECT_EQ(NULL,
+            focus_manager->GetCurrentTargetForAccelerator(escape_accelerator));
+
+  // Hitting the return key and the escape key. Nothing should happen.
+  EXPECT_FALSE(focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(return_target.accelerator_count(), 2);
+  EXPECT_EQ(return_target2.accelerator_count(), 2);
+  EXPECT_EQ(return_target3.accelerator_count(), 2);
+  EXPECT_FALSE(focus_manager->ProcessAccelerator(escape_accelerator));
+  EXPECT_EQ(escape_target.accelerator_count(), 1);
+}
+
+// Unregisters itself when its accelerator is invoked.
+class SelfUnregisteringAcceleratorTarget : public views::AcceleratorTarget {
+ public:
+  SelfUnregisteringAcceleratorTarget(views::Accelerator accelerator,
+                                     views::FocusManager* focus_manager)
+      : accelerator_(accelerator),
+        focus_manager_(focus_manager),
+        accelerator_count_(0) {
+  }
+
+  virtual bool AcceleratorPressed(const views::Accelerator& accelerator) {
+    ++accelerator_count_;
+    focus_manager_->UnregisterAccelerator(accelerator, this);
+    return true;
+  }
+
+  int accelerator_count() const { return accelerator_count_; }
+
+ private:
+  views::Accelerator accelerator_;
+  views::FocusManager* focus_manager_;
+  int accelerator_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(SelfUnregisteringAcceleratorTarget);
+};
+
+TEST_F(FocusManagerTest, CallsSelfDeletingAcceleratorTarget) {
+  views::FocusManager* focus_manager =
+      views::FocusManager::GetFocusManager(test_window_->GetNativeView());
+  views::Accelerator return_accelerator(VK_RETURN, false, false, false);
+  SelfUnregisteringAcceleratorTarget target(return_accelerator, focus_manager);
+  EXPECT_EQ(target.accelerator_count(), 0);
+  EXPECT_EQ(NULL,
+            focus_manager->GetCurrentTargetForAccelerator(return_accelerator));
+
+  // Register the target.
+  focus_manager->RegisterAccelerator(return_accelerator, &target);
+  EXPECT_EQ(&target,
+            focus_manager->GetCurrentTargetForAccelerator(return_accelerator));
+
+  // Hitting the return key. The target will be unregistered.
+  EXPECT_TRUE(focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(target.accelerator_count(), 1);
+  EXPECT_EQ(NULL,
+            focus_manager->GetCurrentTargetForAccelerator(return_accelerator));
+
+  // Hitting the return key again; nothing should happen.
+  EXPECT_FALSE(focus_manager->ProcessAccelerator(return_accelerator));
+  EXPECT_EQ(target.accelerator_count(), 1);
 }
+
+}  // anonymous namespace