Close avatar bubble when avatar button is clicked a second time

Currently when the avatar button is clicked a second time the avatar bubble closes then immediately opens again.

The problem is that the same mouse event that closes the bubble also causes to open again.

To fix this I've changed AvatarMenuBubbleView to track it's current instance. If a caller tries to show the bubble while an instance is already being shown then the show request is ignored.

BUG=107876
TEST=Clicked on the avatar button a second time and verified that the avatar bubble stayed closed.

Review URL: https://chromiumcodereview.appspot.com/11646008

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

7 files changed, 133 insertions, 20 deletions

diff --git a/chrome/browser/ui/views/avatar_menu_bubble_view.cc b/chrome/browser/ui/views/avatar_menu_bubble_view.cc
index f3665bd..58e8964 100644
--- a/chrome/browser/ui/views/avatar_menu_bubble_view.cc
+++ b/chrome/browser/ui/views/avatar_menu_bubble_view.cc
@@ -7,12 +7,14 @@
 #include <algorithm>
 
 #include "base/utf_string_conversions.h"
+#include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/avatar_menu_model.h"
 #include "chrome/browser/profiles/profile_info_cache.h"
 #include "chrome/browser/profiles/profile_info_util.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -27,6 +29,7 @@
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/link.h"
 #include "ui/views/controls/separator.h"
+#include "ui/views/widget/widget.h"
 
 namespace {
 
@@ -387,6 +390,38 @@ bool ProfileItemView::IsHighlighted() {
 
 // AvatarMenuBubbleView -------------------------------------------------------
 
+// static
+AvatarMenuBubbleView* AvatarMenuBubbleView::avatar_bubble_ = NULL;
+
+// static
+void AvatarMenuBubbleView::ShowBubble(
+    views::View* anchor_view,
+    views::BubbleBorder::ArrowLocation arrow_location,
+    views::BubbleBorder::BubbleAlignment border_alignment,
+    const gfx::Rect& anchor_rect,
+    Browser* browser) {
+  if (IsShowing())
+    return;
+
+  DCHECK(chrome::IsCommandEnabled(browser, IDC_SHOW_AVATAR_MENU));
+  avatar_bubble_ = new AvatarMenuBubbleView(
+      anchor_view, arrow_location, anchor_rect, browser);
+  views::BubbleDelegateView::CreateBubble(avatar_bubble_);
+  avatar_bubble_->SetAlignment(border_alignment);
+  avatar_bubble_->Show();
+}
+
+// static
+bool AvatarMenuBubbleView::IsShowing() {
+  return avatar_bubble_ != NULL;
+}
+
+// static
+void AvatarMenuBubbleView::Hide() {
+  if (IsShowing())
+    avatar_bubble_->GetWidget()->Close();
+}
+
 AvatarMenuBubbleView::AvatarMenuBubbleView(
     views::View* anchor_view,
     views::BubbleBorder::ArrowLocation arrow_location,
@@ -520,6 +555,11 @@ void AvatarMenuBubbleView::Init() {
   AddAccelerator(ui::Accelerator(ui::VKEY_UP, ui::EF_NONE));
 }
 
+void AvatarMenuBubbleView::WindowClosing() {
+  DCHECK_EQ(avatar_bubble_, this);
+  avatar_bubble_ = NULL;
+}
+
 void AvatarMenuBubbleView::OnAvatarMenuModelChanged(
     AvatarMenuModel* avatar_menu_model) {
   // Unset all our child view references and call RemoveAllChildViews() which
diff --git a/chrome/browser/ui/views/avatar_menu_bubble_view.h b/chrome/browser/ui/views/avatar_menu_bubble_view.h
index 5490ac2..560c184 100644
--- a/chrome/browser/ui/views/avatar_menu_bubble_view.h
+++ b/chrome/browser/ui/views/avatar_menu_bubble_view.h
@@ -30,10 +30,24 @@ class AvatarMenuBubbleView : public views::BubbleDelegateView,
                              public views::LinkListener,
                              public AvatarMenuModelObserver {
  public:
-  AvatarMenuBubbleView(views::View* anchor_view,
-                       views::BubbleBorder::ArrowLocation arrow_location,
-                       const gfx::Rect& anchor_rect,
-                       Browser* browser);
+  // Helper function to show the bubble and ensure that it doesn't reshow.
+  // Normally this bubble is shown when there's a mouse down event on a button.
+  // If the bubble is already showing when the user clicks on the button then
+  // this will cause two things to happen:
+  // - (1) the button will show a new instance of the bubble
+  // - (2) the old instance of the bubble will get a deactivate event and
+  //   close
+  // To prevent this reshow this function checks if an instance of the bubble
+  // is already showing and do nothing. This means that (1) will do nothing
+  // and (2) will correctly hide the old bubble instance.
+  static void ShowBubble(views::View* anchor_view,
+                         views::BubbleBorder::ArrowLocation arrow_location,
+                         views::BubbleBorder::BubbleAlignment border_alignment,
+                         const gfx::Rect& anchor_rect,
+                         Browser* browser);
+  static bool IsShowing();
+  static void Hide();
+
   virtual ~AvatarMenuBubbleView();
 
   // views::View implementation.
@@ -51,12 +65,18 @@ class AvatarMenuBubbleView : public views::BubbleDelegateView,
   // BubbleDelegate implementation.
   virtual gfx::Rect GetAnchorRect() OVERRIDE;
   virtual void Init() OVERRIDE;
+  virtual void WindowClosing() OVERRIDE;
 
   // AvatarMenuModelObserver implementation.
   virtual void OnAvatarMenuModelChanged(
       AvatarMenuModel* avatar_menu_model) OVERRIDE;
 
  private:
+  AvatarMenuBubbleView(views::View* anchor_view,
+                       views::BubbleBorder::ArrowLocation arrow_location,
+                       const gfx::Rect& anchor_rect,
+                       Browser* browser);
+
   views::Link* add_profile_link_;
   scoped_ptr<AvatarMenuModel> avatar_menu_model_;
   gfx::Rect anchor_rect_;
@@ -64,6 +84,8 @@ class AvatarMenuBubbleView : public views::BubbleDelegateView,
   std::vector<views::CustomButton*> item_views_;
   views::Separator* separator_;
 
+  static AvatarMenuBubbleView* avatar_bubble_;
+
   DISALLOW_COPY_AND_ASSIGN(AvatarMenuBubbleView);
 };
 
diff --git a/chrome/browser/ui/views/avatar_menu_button.cc b/chrome/browser/ui/views/avatar_menu_button.cc
index f1385b0..bd3cb1c 100644
--- a/chrome/browser/ui/views/avatar_menu_button.cc
+++ b/chrome/browser/ui/views/avatar_menu_button.cc
@@ -4,7 +4,6 @@
 
 #include "chrome/browser/ui/views/avatar_menu_button.h"
 
-#include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/command_updater.h"
 #include "chrome/browser/managed_mode/managed_mode.h"
@@ -13,7 +12,6 @@
 #include "chrome/browser/profiles/profile_info_util.h"
 #include "chrome/browser/profiles/profile_metrics.h"
 #include "chrome/browser/ui/browser.h"
-#include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/views/avatar_menu_bubble_view.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/common/chrome_notification_types.h"
@@ -160,16 +158,11 @@ void AvatarMenuButton::OnMenuButtonClicked(views::View* source,
 }
 
 void AvatarMenuButton::ShowAvatarBubble() {
-  DCHECK(chrome::IsCommandEnabled(browser_, IDC_SHOW_AVATAR_MENU));
-
   gfx::Point origin;
   views::View::ConvertPointToScreen(this, &origin);
   gfx::Rect bounds(origin, size());
-
-  AvatarMenuBubbleView* bubble = new AvatarMenuBubbleView(this,
-      views::BubbleBorder::TOP_LEFT, bounds, browser_);
-  views::BubbleDelegateView::CreateBubble(bubble);
-  bubble->Show();
+  AvatarMenuBubbleView::ShowBubble(this, views::BubbleBorder::TOP_LEFT,
+      views::BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR, bounds, browser_);
 
   ProfileMetrics::LogProfileOpenMethod(ProfileMetrics::ICON_AVATAR_BUBBLE);
 }
diff --git a/chrome/browser/ui/views/avatar_menu_button.h b/chrome/browser/ui/views/avatar_menu_button.h
index 54b6452..5ebdbee 100644
--- a/chrome/browser/ui/views/avatar_menu_button.h
+++ b/chrome/browser/ui/views/avatar_menu_button.h
@@ -51,8 +51,6 @@ class AvatarMenuButton : public views::MenuButton,
   virtual void OnMenuButtonClicked(views::View* source,
                                    const gfx::Point& point) OVERRIDE;
 
-  void ButtonClicked();
-
   Browser* browser_;
   bool incognito_;
   scoped_ptr<ui::MenuModel> menu_model_;
diff --git a/chrome/browser/ui/views/avatar_menu_button_browsertest.cc b/chrome/browser/ui/views/avatar_menu_button_browsertest.cc
new file mode 100644
index 0000000..4ec9e01
--- /dev/null
+++ b/chrome/browser/ui/views/avatar_menu_button_browsertest.cc
@@ -0,0 +1,62 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/views/avatar_menu_button.h"
+
+#include "base/path_service.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/views/avatar_menu_bubble_view.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/testing_browser_process.h"
+
+namespace {
+
+void CreateTestingProfile() {
+  ProfileManager* profile_manager = g_browser_process->profile_manager();
+  EXPECT_EQ(1u, profile_manager->GetNumberOfProfiles());
+
+  FilePath path;
+  PathService::Get(chrome::DIR_USER_DATA, &path);
+  path = path.AppendASCII("test_profile");
+  if (!file_util::PathExists(path))
+    CHECK(file_util::CreateDirectory(path));
+  Profile* profile =
+      Profile::CreateProfile(path, NULL, Profile::CREATE_MODE_SYNCHRONOUS);
+  profile_manager->RegisterTestingProfile(profile, true);
+
+  EXPECT_EQ(2u, profile_manager->GetNumberOfProfiles());
+}
+
+typedef InProcessBrowserTest AvatarMenuButtonTest;
+
+IN_PROC_BROWSER_TEST_F(AvatarMenuButtonTest, HideOnSecondClick) {
+  if (!ProfileManager::IsMultipleProfilesEnabled())
+    return;
+
+  CreateTestingProfile();
+
+  BrowserView* browser_view = reinterpret_cast<BrowserView*>(
+      browser()->window());
+  AvatarMenuButton* button = browser_view->frame()->GetAvatarMenuButton();
+  ASSERT_TRUE(button);
+
+  // Verify that clicking once shows the avatar bubble.
+  static_cast<views::MenuButtonListener*>(button)->OnMenuButtonClicked(
+      NULL, gfx::Point());
+  MessageLoop::current()->RunUntilIdle();
+  EXPECT_TRUE(AvatarMenuBubbleView::IsShowing());
+
+  // Verify that clicking again doesn't reshow it.
+  static_cast<views::MenuButtonListener*>(button)->OnMenuButtonClicked(
+      NULL, gfx::Point());
+  // Hide the bubble manually. In the browser this would normally happen during
+  // the event processing.
+  AvatarMenuBubbleView::Hide();
+  MessageLoop::current()->RunUntilIdle();
+  EXPECT_FALSE(AvatarMenuBubbleView::IsShowing());
+}
+
+}  // namespace
diff --git a/chrome/browser/ui/views/frame/browser_view.cc b/chrome/browser/ui/views/frame/browser_view.cc
index f2238f5..a220942 100644
--- a/chrome/browser/ui/views/frame/browser_view.cc
+++ b/chrome/browser/ui/views/frame/browser_view.cc
@@ -2745,11 +2745,8 @@ void BrowserView::ShowAvatarBubble(WebContents* web_contents,
   views::View::ConvertPointToScreen(GetTabContentsContainerView(), &origin);
   gfx::Rect bounds(origin, rect.size());
 
-  AvatarMenuBubbleView* bubble = new AvatarMenuBubbleView(this,
-      views::BubbleBorder::TOP_RIGHT, bounds, browser_.get());
-  views::BubbleDelegateView::CreateBubble(bubble);
-  bubble->SetAlignment(views::BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE);
-  bubble->Show();
+  AvatarMenuBubbleView::ShowBubble(this, views::BubbleBorder::TOP_RIGHT,
+      views::BubbleBorder::ALIGN_EDGE_TO_ANCHOR_EDGE, bounds, browser_.get());
 }
 
 void BrowserView::ShowAvatarBubbleFromAvatarButton() {
diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi
index bf98e14..dbf20dd 100644
--- a/chrome/chrome_tests.gypi
+++ b/chrome/chrome_tests.gypi
@@ -1221,6 +1221,7 @@
         'browser/ui/startup/startup_browser_creator_browsertest.cc',
         'browser/ui/tab_modal_confirm_dialog_browsertest.cc',
         'browser/ui/tab_modal_confirm_dialog_browsertest.h',
+        'browser/ui/views/avatar_menu_button_browsertest.cc',
         'browser/ui/views/browser_actions_container_browsertest.cc',
         'browser/ui/views/find_bar_controller_browsertest.cc',
         'browser/ui/views/frame/app_non_client_frame_view_ash_browsertest.cc',