Avatar Bubble doesn't re-pop when trying to click it away.

Avatar Bubble doesn't re-pop when trying to click it away.

If the Avatar Bubble (profile chooser) is open and the Avatar Button
is clicked, the bubble should disappear.

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

[Mike Lerman, 2014-08-20 19:39:30]

I originally tried storing the suppress_mouse_released_action_ state in the
browser_non_client_frame_view, but this didn't work. The NewAvatarButton gets
the MousePressed notification before the ProfileChooserView gets OnDestroy
called, but the BrowserNonClientFrameView gets the MousePressed notification
after. So the OnMousePressed call went into NewAvatarButton!

[Mike Lerman, 2014-08-20 19:40:34]

I originally tried storing the suppress_mouse_released_action_ state in the
browser_non_client_frame_view, but this didn't work. The NewAvatarButton gets
the MousePressed notification before the ProfileChooserView gets OnDestroy
called, but the BrowserNonClientFrameView gets the MousePressed notification
after. So the OnMousePressed call went into NewAvatarButton!

[Mike Lerman, 2014-08-20 19:42:50]

Hi Mike, Scott,

This CL implements the ProfileChooserView issue we had talked about over email
earlier this week. This was much easier than using MenuRunner!

Kind sirs, please review my CL. Thanks!

Mike

[msw, 2014-08-20 21:12:37]

https://codereview.chromium.org/488253002/diff/1/chrome/browser/ui/views/prof...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/488253002/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/new_avatar_button.cc:117: // If the avatar
bubble is showing then don't reshow it when the mouse is
nit: aim for one-liners, like "Prevent the bubble from being re-shown on release
if it's already showing."

https://codereview.chromium.org/488253002/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/new_avatar_button.cc:123: return true;
Return MenuButton::OnMousePressed. Remove comment and blank line above.

https://codereview.chromium.org/488253002/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/new_avatar_button.cc:126: bool
NewAvatarButton::IsMouseReleaseActionSuppressed() {
Remove this and instead override NewAvatarButton::OnMouseReleased to only call
MenuButton::OnMouseReleased if the flag isn't set, then clear the flag.

https://codereview.chromium.org/488253002/diff/1/chrome/browser/ui/views/prof...
File chrome/browser/ui/views/profiles/new_avatar_button.h (right):

https://codereview.chromium.org/488253002/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/new_avatar_button.h:31: virtual bool
OnMousePressed(const ui::MouseEvent& event) OVERRIDE;
nit: add comment "// views::MenuButton:"

https://codereview.chromium.org/488253002/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/new_avatar_button.h:64: // This is used to
check if the profile chooser view was showing during the
nit: s/profile chooser view/bubble/, ditto below for "profile chooser".

[Mike Lerman, 2014-08-21 15:15:36]

Thanks for the pointers, Mike!

https://codereview.chromium.org/488253002/diff/1/chrome/browser/ui/views/prof...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/488253002/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/new_avatar_button.cc:117: // If the avatar
bubble is showing then don't reshow it when the mouse is
On 2014/08/20 21:12:36, msw wrote:
> nit: aim for one-liners, like "Prevent the bubble from being re-shown on
release
> if it's already showing."

Done.

https://codereview.chromium.org/488253002/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/new_avatar_button.cc:123: return true;
On 2014/08/20 21:12:36, msw wrote:
> Return MenuButton::OnMousePressed. Remove comment and blank line above.

Done.

https://codereview.chromium.org/488253002/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/new_avatar_button.cc:126: bool
NewAvatarButton::IsMouseReleaseActionSuppressed() {
On 2014/08/20 21:12:36, msw wrote:
> Remove this and instead override NewAvatarButton::OnMouseReleased to only call
> MenuButton::OnMouseReleased if the flag isn't set, then clear the flag.

That is much cleaner. I didn't imagine swallowing the MouseReleased event here
would prevent the ButtonPressed event on the browser_frame objects, but I'm glad
it does!

https://codereview.chromium.org/488253002/diff/1/chrome/browser/ui/views/prof...
File chrome/browser/ui/views/profiles/new_avatar_button.h (right):

https://codereview.chromium.org/488253002/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/new_avatar_button.h:31: virtual bool
OnMousePressed(const ui::MouseEvent& event) OVERRIDE;
On 2014/08/20 21:12:36, msw wrote:
> nit: add comment "// views::MenuButton:"

Done.

https://codereview.chromium.org/488253002/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/new_avatar_button.h:64: // This is used to
check if the profile chooser view was showing during the
On 2014/08/20 21:12:36, msw wrote:
> nit: s/profile chooser view/bubble/, ditto below for "profile chooser".

Done.

[Mike Lerman, 2014-08-21 19:27:47]

Ping Mike - if you have no further comments, I'll try to submit this today to
make canary, so it can merged to M38 tomorrow. Thanks!

[msw, 2014-08-21 19:57:44]

lgtm with nits

https://codereview.chromium.org/488253002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/488253002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:117: // Prevent the avatar
bubble being re-shown on release if already showing.
nit: "Prevent the bubble from being re-shown if it's already showing."

https://codereview.chromium.org/488253002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:123: if
(suppress_mouse_released_action_) {
nit: remove curly braces for this if/else.

https://codereview.chromium.org/488253002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/new_avatar_button.h (right):

https://codereview.chromium.org/488253002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.h:31: // Overridden from
Views::MenuButton
nit: remove "Overridden from" to match the ProfileInfoCacheObserver and
SigninErrorController::Observer comments below.

[Mike Lerman, 2014-08-21 20:02:32]

Thanks.

https://codereview.chromium.org/488253002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/488253002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:117: // Prevent the avatar
bubble being re-shown on release if already showing.
On 2014/08/21 19:57:44, msw wrote:
> nit: "Prevent the bubble from being re-shown if it's already showing."

Done.

https://codereview.chromium.org/488253002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:123: if
(suppress_mouse_released_action_) {
On 2014/08/21 19:57:44, msw wrote:
> nit: remove curly braces for this if/else.

Done.

https://codereview.chromium.org/488253002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/profiles/new_avatar_button.h (right):

https://codereview.chromium.org/488253002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.h:31: // Overridden from
Views::MenuButton
On 2014/08/21 19:57:44, msw wrote:
> nit: remove "Overridden from" to match the ProfileInfoCacheObserver and
> SigninErrorController::Observer comments below.

Done.

[Mike Lerman, 2014-08-21 20:02:38]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2014-08-21 20:05:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mlerman@chromium.org/488253002/40001

[commit-bot: I haz the power, 2014-08-22 00:08:52]

Committed patchset #3 (40001) as 291260