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, 13 insertions, 23 deletions

diff --git a/chrome/browser/views/find_bar_win.cc b/chrome/browser/views/find_bar_win.cc
index 4b2d7ec..f3bfdd7 100644
--- a/chrome/browser/views/find_bar_win.cc
+++ b/chrome/browser/views/find_bar_win.cc
@@ -90,7 +90,7 @@ FindBarWin::FindBarWin(BrowserView* browser_view)
     : browser_view_(browser_view),
       find_dialog_animation_offset_(0),
       focus_manager_(NULL),
-      old_accel_target_for_esc_(NULL),
+      esc_accel_target_registered_(false),
       find_bar_controller_(NULL) {
   gfx::NativeView parent_view = browser_view->GetWidget()->GetNativeView();
 
@@ -574,7 +574,7 @@ void FindBarWin::SetFocusChangeListener(gfx::NativeView parent_view) {
   // FocusManager, which means we need to clean up old listener and start a new
   // one with the new FocusManager.
   if (focus_manager_) {
-    if (old_accel_target_for_esc_)
+    if (esc_accel_target_registered_)
       UnregisterEscAccelerator();
     focus_manager_->RemoveFocusChangeListener(this);
   }
@@ -603,15 +603,10 @@ FindBarTesting* FindBarWin::GetFindBarTesting() {
 
 void FindBarWin::RegisterEscAccelerator() {
 #if defined(OS_WIN)
+  DCHECK(!esc_accel_target_registered_);
   views::Accelerator escape(VK_ESCAPE, false, false, false);
-
-  // TODO(finnur): Once we fix issue 1307173 we should not remember any old
-  // accelerator targets and just Register and Unregister when needed.
-  views::AcceleratorTarget* old_target =
-      focus_manager_->RegisterAccelerator(escape, this);
-
-  if (!old_accel_target_for_esc_)
-    old_accel_target_for_esc_ = old_target;
+  focus_manager_->RegisterAccelerator(escape, this);
+  esc_accel_target_registered_ = true;
 #else
   NOTIMPLEMENTED();
 #endif
@@ -619,14 +614,10 @@ void FindBarWin::RegisterEscAccelerator() {
 
 void FindBarWin::UnregisterEscAccelerator() {
 #if defined(OS_WIN)
-  // TODO(finnur): Once we fix issue 1307173 we should not remember any old
-  // accelerator targets and just Register and Unregister when needed.
-  DCHECK(old_accel_target_for_esc_ != NULL);
+  DCHECK(esc_accel_target_registered_);
   views::Accelerator escape(VK_ESCAPE, false, false, false);
-  views::AcceleratorTarget* current_target =
-      focus_manager_->GetTargetForAccelerator(escape);
-  if (current_target == this)
-    focus_manager_->RegisterAccelerator(escape, old_accel_target_for_esc_);
+  focus_manager_->UnregisterAccelerator(escape, this);
+  esc_accel_target_registered_ = false;
 #else
   NOTIMPLEMENTED();
 #endif
diff --git a/chrome/browser/views/find_bar_win.h b/chrome/browser/views/find_bar_win.h
index d59c4c23..db7b3e7 100644
--- a/chrome/browser/views/find_bar_win.h
+++ b/chrome/browser/views/find_bar_win.h
@@ -166,9 +166,8 @@ class FindBarWin : public views::AcceleratorTarget,
   // The focus manager we register with to keep track of focus changes.
   views::FocusManager* focus_manager_;
 
-  // Stores the previous accelerator target for the Escape key, so that we can
-  // restore the state once we loose focus.
-  views::AcceleratorTarget* old_accel_target_for_esc_;
+  // True if the accelerator target for Esc key is registered.
+  bool esc_accel_target_registered_;
 
   // Tracks and stores the last focused view which is not the FindBarView
   // or any of its children. Used to restore focus once the FindBarView is
diff --git a/chrome/browser/views/find_bar_win_browsertest.cc b/chrome/browser/views/find_bar_win_browsertest.cc
index 724b7c6..98a8111 100644
--- a/chrome/browser/views/find_bar_win_browsertest.cc
+++ b/chrome/browser/views/find_bar_win_browsertest.cc
@@ -563,7 +563,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, AcceleratorRestoring) {
   // See where Escape is registered.
   views::Accelerator escape(VK_ESCAPE, false, false, false);
   views::AcceleratorTarget* old_target =
-      focus_manager->GetTargetForAccelerator(escape);
+      focus_manager->GetCurrentTargetForAccelerator(escape);
   EXPECT_TRUE(old_target != NULL);
 
   // Open the Find box.
@@ -571,7 +571,7 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, AcceleratorRestoring) {
 
   // Our Find bar should be the new target.
   views::AcceleratorTarget* new_target =
-      focus_manager->GetTargetForAccelerator(escape);
+      focus_manager->GetCurrentTargetForAccelerator(escape);
 
   EXPECT_TRUE(new_target != NULL);
   EXPECT_NE(new_target, old_target);
@@ -580,5 +580,5 @@ IN_PROC_BROWSER_TEST_F(FindInPageControllerTest, AcceleratorRestoring) {
   browser()->find_bar()->EndFindSession();
 
   // The accelerator for Escape should be back to what it was before.
-  EXPECT_EQ(old_target, focus_manager->GetTargetForAccelerator(escape));
+  EXPECT_EQ(old_target, focus_manager->GetCurrentTargetForAccelerator(escape));
 }