Close avatar bubble when avatar button is clicked a second time

chrome/browser/ui/views/avatar_menu_button.cc

Index: chrome/browser/ui/views/avatar_menu_button.cc
diff --git a/chrome/browser/ui/views/avatar_menu_button.cc b/chrome/browser/ui/views/avatar_menu_button.cc
index f1385b046762477962113c6c39e8739f4382c76f..20fa0ac5e69a1e66fd575afac864cf2918831373 100644
--- a/chrome/browser/ui/views/avatar_menu_button.cc
+++ b/chrome/browser/ui/views/avatar_menu_button.cc
@@ -100,6 +100,8 @@ AvatarMenuButton::AvatarMenuButton(Browser* browser, bool incognito)
 }
 
 AvatarMenuButton::~AvatarMenuButton() {
+  if (bubble_widget_)
+    OnWidgetClosing(bubble_widget_);
 }
 
 void AvatarMenuButton::OnPaint(gfx::Canvas* canvas) {
@@ -159,7 +161,27 @@ void AvatarMenuButton::OnMenuButtonClicked(views::View* source,
     ShowAvatarBubble();
 }
 
+void AvatarMenuButton::OnWidgetClosing(views::Widget* widget) {
+  bubble_widget_->RemoveObserver(this);
+  bubble_widget_ = NULL;
+  // If the avatar bubble is about to be shown the cancel the operation. This
+  // prevents the bubble from closing and immediately reopening when the user
+  // clicks the avatar button a second time.
+  show_avatar_bubble_closure_.Cancel();
+}
+
 void AvatarMenuButton::ShowAvatarBubble() {
+  show_avatar_bubble_closure_.Reset(
+      base::Bind(&AvatarMenuButton::ShowAvatarBubbleImpl,
+      base::Unretained(this)));
+  MessageLoop::current()->PostTask(FROM_HERE,
+                                   show_avatar_bubble_closure_.callback());

[Peter Kasting, 2012/12/28 18:55:55]

This has three problems:

(1) It makes the menu opening more laggy by whatever the UI loop latency is.  In
theory that latency is "low" but in practice it can definitely be noticeable, at
least enough to make something feel just a tiny bit off.
(2) This solution is somewhat complex.
(3) I presume this works because on mouse down, the old bubble is first hidden
(but not destroyed), then OnMenuButtonClicked() is reached (creating a new
bubble), then the old widget is destroyed (calling OnWidgetClosing())?  Relying
on this destruction order seems fragile because we could easily change the
"hide, destroy asynchronously" behavior to "destroy now" and break this code.

It seems like maybe a better solution here is to simply add some kind of
OnMousePressed() handler on the menubutton that says "hey, the bubble is
currently open, ignore the next OnMenuButtonClicked() call" and then calls
through to the superclass implementation (which will then call back to
OnMenuButtonClicked()).  This still needs to detect whether the menu is already
open -- I'm not sure of the best way to do that, perhaps something like
|bubble_widget_|, perhaps some sort of static method on AvatarMenuBubbleView,
perhaps passing |this| to AvatarMenuBubbleView so it can call back when it's
hidden/destroyed -- but seems like it at least avoids the asynchronicity.

[sail, 2013/01/02 21:06:01]

Done.
I changed this to match the bookmark code by having a static reference to the
open bubble.

+}
+
+void AvatarMenuButton::ShowAvatarBubbleImpl() {

[Peter Kasting, 2012/12/28 18:55:55]

There's nearly identical code to this in browser_view.cc.  Can we make it call
through to here (or better yet, make all its callers call here) instead of
duplicating code?  While I don't _think_ that code needs the fix here, it would
be nice not to have the confusion.

(Incidentally, the same applies to GTK.)

[sail, 2013/01/02 21:06:01]

Done.

+  if (bubble_widget_)
+    return;
+
   DCHECK(chrome::IsCommandEnabled(browser_, IDC_SHOW_AVATAR_MENU));
 
   gfx::Point origin;
@@ -168,7 +190,8 @@ void AvatarMenuButton::ShowAvatarBubble() {
 
   AvatarMenuBubbleView* bubble = new AvatarMenuBubbleView(this,
       views::BubbleBorder::TOP_LEFT, bounds, browser_);
-  views::BubbleDelegateView::CreateBubble(bubble);
+  bubble_widget_ = views::BubbleDelegateView::CreateBubble(bubble);
+  bubble_widget_->AddObserver(this);
   bubble->Show();
 
   ProfileMetrics::LogProfileOpenMethod(ProfileMetrics::ICON_AVATAR_BUBBLE);