Close avatar bubble when avatar button is clicked a second time

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.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175220

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

https://codereview.chromium.org/11646008/diff/1/chrome/browser/ui/views/avata...
File chrome/browser/ui/views/avatar_menu_button.cc (right):

https://codereview.chromium.org/11646008/diff/1/chrome/browser/ui/views/avata...
chrome/browser/ui/views/avatar_menu_button.cc:178:
show_avatar_bubble_closure_.callback());
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.

https://codereview.chromium.org/11646008/diff/1/chrome/browser/ui/views/avata...
chrome/browser/ui/views/avatar_menu_button.cc:181: void
AvatarMenuButton::ShowAvatarBubbleImpl() {
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]

https://codereview.chromium.org/11646008/diff/1/chrome/browser/ui/views/avata...
File chrome/browser/ui/views/avatar_menu_button.cc (right):

https://codereview.chromium.org/11646008/diff/1/chrome/browser/ui/views/avata...
chrome/browser/ui/views/avatar_menu_button.cc:178:
show_avatar_bubble_closure_.callback());
On 2012/12/28 18:55:55, Peter Kasting wrote:
> 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.

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

https://codereview.chromium.org/11646008/diff/1/chrome/browser/ui/views/avata...
chrome/browser/ui/views/avatar_menu_button.cc:181: void
AvatarMenuButton::ShowAvatarBubbleImpl() {
On 2012/12/28 18:55:55, Peter Kasting wrote:
> 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.)

Done.

[Peter Kasting, 2013-01-04 00:58:43]

LGTM, but please update the change description.

https://codereview.chromium.org/11646008/diff/5001/chrome/browser/ui/views/av...
File chrome/browser/ui/views/avatar_menu_bubble_view.h (right):

https://codereview.chromium.org/11646008/diff/5001/chrome/browser/ui/views/av...
chrome/browser/ui/views/avatar_menu_bubble_view.h:33: static void
ShowBubble(views::View* anchor_view,
Nit: Write a comment to note why we want this sort of function (i.e. that we
want to close when the button is clicked and the bubble is open; why this makes
that happen; and why without this we'd wind up not doing that).

(At least for me, the part that's least clear is why this causes the bubble to
close when the bubble is clicked a second time, as opposed to doing nothing.)

https://codereview.chromium.org/11646008/diff/5001/chrome/browser/ui/views/av...
File chrome/browser/ui/views/avatar_menu_button_browsertest.cc (right):

https://codereview.chromium.org/11646008/diff/5001/chrome/browser/ui/views/av...
chrome/browser/ui/views/avatar_menu_button_browsertest.cc:35: // Verify that
clicking the avatar button a second time hides the avatar bubble.
Nit: Technically, this doesn't actually verify that, it just verifies that we
don't re-show.  Since we already have that comment below, I'd probably just nuke
this.

[sail, 2013-01-04 19:23:08]

https://codereview.chromium.org/11646008/diff/5001/chrome/browser/ui/views/av...
File chrome/browser/ui/views/avatar_menu_bubble_view.h (right):

https://codereview.chromium.org/11646008/diff/5001/chrome/browser/ui/views/av...
chrome/browser/ui/views/avatar_menu_bubble_view.h:33: static void
ShowBubble(views::View* anchor_view,
On 2013/01/04 00:58:43, Peter Kasting wrote:
> Nit: Write a comment to note why we want this sort of function (i.e. that we
> want to close when the button is clicked and the bubble is open; why this
makes
> that happen; and why without this we'd wind up not doing that).
> 
> (At least for me, the part that's least clear is why this causes the bubble to
> close when the bubble is clicked a second time, as opposed to doing nothing.)

Done.

https://codereview.chromium.org/11646008/diff/5001/chrome/browser/ui/views/av...
File chrome/browser/ui/views/avatar_menu_button_browsertest.cc (right):

https://codereview.chromium.org/11646008/diff/5001/chrome/browser/ui/views/av...
chrome/browser/ui/views/avatar_menu_button_browsertest.cc:35: // Verify that
clicking the avatar button a second time hides the avatar bubble.
On 2013/01/04 00:58:43, Peter Kasting wrote:
> Nit: Technically, this doesn't actually verify that, it just verifies that we
> don't re-show.  Since we already have that comment below, I'd probably just
nuke
> this.

Done.

[commit-bot: I haz the power, 2013-01-04 19:23:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11646008/11001

[commit-bot: I haz the power, 2013-01-04 19:47:58]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-01-04 20:11:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/11646008/12009

[commit-bot: I haz the power, 2013-01-04 22:57:45]

Change committed as 175220