Chrome user menu shouldn't close if a tab steals focus

Chrome user menu shouldn't close if a tab steals focus.

This CL is dedicated to fix above issue and achieve below behaviors:
1.  User Menu does not close during sign-in flow: Things can't steal focus; the user menu stays open while you're typing / in the signin flow.
2.  Sign-in confirmation is modal: The user menu is not closeable until the user interacts with the signin confirmation window.  Sync should be delayed until the user interacts with the dialogue.
3.  Blue information box: This shouldn't go away until the user explicitly interacts with it (not you, what's new, "x").  Doesn't affect modality/close-ability of the user menu.

BUG=341196
Committed: https://crrev.com/131615b8cfd0e9b3045f3384e06ec3d8eb4f272a
Cr-Commit-Position: refs/heads/master@{#330789}

[gogerald1, 2015-05-08 10:46:02]

gogerald@chromium.org changed reviewers:
+ msw@chromium.org, rogerta@chromium.org

[gogerald1, 2015-05-08 12:30:01]

Please review it.

[Roger Tawa OOO till Jul 10th, 2015-05-08 14:51:17]

Hi Ganggui,

Some questions below.  Thanks,

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

https://codereview.chromium.org/1136693002/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/new_avatar_button.cc:124:
ProfileChooserView::Hide();
If the member |suppress_mouse_released_action_| is no longer used it should
removed.

https://codereview.chromium.org/1136693002/diff/1/chrome/browser/ui/views/pro...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1136693002/diff/1/chrome/browser/ui/views/pro...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:694: 
Nit: since this is a multi-line/complex if-then-else you need all the { and }.

I'm not sure about the logic.  For example, as is this code will cause the menu
to be sticky if |view_mode_| is BUBBLE_VIEW_MODE_ACCOUNT_MANAGEMENT or
BUBBLE_VIEW_MODE_ACCOUNT_REMOVAL.  It should only be sticky for the three web
view modes.

Also, for the tutorial, this makes the welcome and error tutorials sticky, which
is not what we want.

Did I miss something?

https://codereview.chromium.org/1136693002/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/inline_login_handler.cc (right):

https://codereview.chromium.org/1136693002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/inline_login_handler.cc:147: }
I guess you can' call ProfileChooserView::Hide() directly because that only
handles the views case.  Does the code as written also handle mac?

[gogerald1, 2015-05-13 20:32:03]

gogerald@chromium.org changed reviewers:
+ groby@chromium.org

[gogerald1, 2015-05-13 20:32:54]

Hi, Please review them, work for windows/Linux and Mac

[Roger Tawa OOO till Jul 10th, 2015-05-14 18:55:03]

A few more questions Ganggui.  Thanks.

https://codereview.chromium.org/1136693002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1136693002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2512: if
(switches::IsNewAvatarMenu()) {
The check for switches::IsNewAvatarMenu() is missing from the cocoa code.  Is it
needed there?

https://codereview.chromium.org/1136693002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2518: }
You don't need the calls to IsShowing() in both cases, since the Hide() methods
already do that.

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

https://codereview.chromium.org/1136693002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/new_avatar_button.cc:131: else
This change makes it so that clicking a second time on the same avatar button
causes the popup to close and reopen.  We should maintain the old behaviour
which was to close the popup.

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

https://codereview.chromium.org/1136693002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:695:
profile_bubble_->set_close_on_deactivate(close_on_deactivate_for_testing_);
If you added { and } to the if block, you also need the { and } around the else
block, even if it's a trivial one line.

https://codereview.chromium.org/1136693002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_handler.cc (right):

https://codereview.chromium.org/1136693002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler.cc:151: }
Nit: don't need { and } for this simple if statement.

https://codereview.chromium.org/1136693002/diff/20001/chrome/test/base/test_b...
File chrome/test/base/test_browser_window.h (right):

https://codereview.chromium.org/1136693002/diff/20001/chrome/test/base/test_b...
chrome/test/base/test_browser_window.h:147: void
CloseAvatarBubbleFromAvatarButton() override{};
Nit space before {

[gogerald1, 2015-05-14 21:35:22]

Addressed Roger's comments

https://codereview.chromium.org/1136693002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1136693002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2512: if
(switches::IsNewAvatarMenu()) {
On 2015/05/14 18:55:03, Roger Tawa wrote:
> The check for switches::IsNewAvatarMenu() is missing from the cocoa code.  Is
it
> needed there?
Can't, since both menus in cocoa code have common derived close
avatarButtonController with Hide() interface, then both of them override it,
however, in Linux/Window new and old menus both independently declared and
implemented a static function Hide().

https://codereview.chromium.org/1136693002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2518: }
On 2015/05/14 18:55:02, Roger Tawa wrote:
> You don't need the calls to IsShowing() in both cases, since the Hide()
methods
> already do that.

Done.

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

https://codereview.chromium.org/1136693002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/new_avatar_button.cc:131: else
On 2015/05/14 18:55:03, Roger Tawa wrote:
> This change makes it so that clicking a second time on the same avatar button
> causes the popup to close and reopen.  We should maintain the old behaviour
> which was to close the popup.

Done.

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

https://codereview.chromium.org/1136693002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:695:
profile_bubble_->set_close_on_deactivate(close_on_deactivate_for_testing_);
On 2015/05/14 18:55:03, Roger Tawa wrote:
> If you added { and } to the if block, you also need the { and } around the
else
> block, even if it's a trivial one line.

Done.

https://codereview.chromium.org/1136693002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_handler.cc (right):

https://codereview.chromium.org/1136693002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler.cc:151: }
On 2015/05/14 18:55:03, Roger Tawa wrote:
> Nit: don't need { and } for this simple if statement.

Done.

https://codereview.chromium.org/1136693002/diff/20001/chrome/test/base/test_b...
File chrome/test/base/test_browser_window.h (right):

https://codereview.chromium.org/1136693002/diff/20001/chrome/test/base/test_b...
chrome/test/base/test_browser_window.h:147: void
CloseAvatarBubbleFromAvatarButton() override{};
On 2015/05/14 18:55:03, Roger Tawa wrote:
> Nit space before {

Done.

[Roger Tawa OOO till Jul 10th, 2015-05-15 14:29:18]

lgtm after addressing comments below.

Thanks Ganggui!

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

https://codereview.chromium.org/1136693002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/new_avatar_button.cc:129:
ProfileChooserView::Hide();
Nit: need { and } around if and else blocks.
Nit: create a local variable for
ProfileChooserView::IsShowingInBrowser(browser_)?

https://codereview.chromium.org/1136693002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1136693002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:525: if
(profile_bubble_->browser_ == browser || NULL == browser)
Use nullptr at lines 522 and 525, and the constant should be on RHS of ==.

https://codereview.chromium.org/1136693002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/1136693002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.h:40: enum ShowingType {
Please add a comment explaining how this is used.  This should go inside the
ProfileChooserView class so it does not get added to the global namespace.

[groby-ooo-7-16, 2015-05-15 16:16:24]

c/b/ui/cocoa LGTM % nit

https://codereview.chromium.org/1136693002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm (right):

https://codereview.chromium.org/1136693002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:222: if
(menuController_ != nil)
nit: No need to check - you can send messages to nil objects without harm.
(It'll just be ignored)

[gogerald1, 2015-05-15 22:05:13]

Patchset #4 (id:60001) has been deleted

[msw, 2015-05-19 22:57:49]

Just some nits, sorry for the delay.

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

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/new_avatar_button.cc:123: // Prevent the bubble
from being re-shown if it's already showing in current
nit: "in the current"

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:525: } else {
nit: no else after return; drop curly braces.

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:528: else
ditto nit: no else after return.

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.h:67: // designate
browser.
nit: designated

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.h:70: IS_SHOWING = 1,
nit: explicit assignment of consecutive values isn't needed after "= 0" above.

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_handler.cc (right):

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler.cc:143:
signin_metrics::Source source_ = signin::GetSourceForPromoURL(main_frame_url);
nit: name this |source|; don't add the underscore postfix for this stack-local.

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler.cc:150:
browser->window()->CloseAvatarBubbleFromAvatarButton();
Add a comment somewhere nearby explaining why this closes the bubble...

https://codereview.chromium.org/1136693002/diff/80001/chrome/test/base/test_b...
File chrome/test/base/test_browser_window.h (right):

https://codereview.chromium.org/1136693002/diff/80001/chrome/test/base/test_b...
chrome/test/base/test_browser_window.h:147: void
CloseAvatarBubbleFromAvatarButton() override {};
nit: no semicolon after function definition

[gogerald1, 2015-05-19 23:35:13]

address Michael's comments

https://codereview.chromium.org/1136693002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm (right):

https://codereview.chromium.org/1136693002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:222: if
(menuController_ != nil)
On 2015/05/15 16:16:24, groby wrote:
> nit: No need to check - you can send messages to nil objects without harm.
> (It'll just be ignored)

Done.

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

https://codereview.chromium.org/1136693002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/new_avatar_button.cc:129:
ProfileChooserView::Hide();
On 2015/05/15 14:29:17, Roger Tawa wrote:
> Nit: need { and } around if and else blocks.
> Nit: create a local variable for
> ProfileChooserView::IsShowingInBrowser(browser_)?

Acknowledged. use local variable to save one function call, after that the if
else is just a single sentence, so it will be more clear without {}.

https://codereview.chromium.org/1136693002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1136693002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:525: if
(profile_bubble_->browser_ == browser || NULL == browser)
On 2015/05/15 14:29:17, Roger Tawa wrote:
> Use nullptr at lines 522 and 525, and the constant should be on RHS of ==.

Done.

https://codereview.chromium.org/1136693002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/1136693002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.h:40: enum ShowingType {
On 2015/05/15 14:29:17, Roger Tawa wrote:
> Please add a comment explaining how this is used.  This should go inside the
> ProfileChooserView class so it does not get added to the global namespace.

Done.

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

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/new_avatar_button.cc:123: // Prevent the bubble
from being re-shown if it's already showing in current
On 2015/05/19 22:57:48, msw wrote:
> nit: "in the current"

Done.

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:525: } else {
On 2015/05/19 22:57:48, msw wrote:
> nit: no else after return; drop curly braces.

Done.

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:528: else
On 2015/05/19 22:57:48, msw wrote:
> ditto nit: no else after return.

Done.

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.h:67: // designate
browser.
On 2015/05/19 22:57:48, msw wrote:
> nit: designated

Done.

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.h:70: IS_SHOWING = 1,
Yes, but list to make them obvious,
On 2015/05/19 22:57:49, msw wrote:
> nit: explicit assignment of consecutive values isn't needed after "= 0" above.

Acknowledged.

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/inline_login_handler.cc (right):

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler.cc:143:
signin_metrics::Source source_ = signin::GetSourceForPromoURL(main_frame_url);
On 2015/05/19 22:57:49, msw wrote:
> nit: name this |source|; don't add the underscore postfix for this
stack-local.

Done.

https://codereview.chromium.org/1136693002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/inline_login_handler.cc:150:
browser->window()->CloseAvatarBubbleFromAvatarButton();
On 2015/05/19 22:57:49, msw wrote:
> Add a comment somewhere nearby explaining why this closes the bubble...

Done.

https://codereview.chromium.org/1136693002/diff/80001/chrome/test/base/test_b...
File chrome/test/base/test_browser_window.h (right):

https://codereview.chromium.org/1136693002/diff/80001/chrome/test/base/test_b...
chrome/test/base/test_browser_window.h:147: void
CloseAvatarBubbleFromAvatarButton() override {};
On 2015/05/19 22:57:49, msw wrote:
> nit: no semicolon after function definition

Done.

[msw, 2015-05-20 00:18:12]

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.cc:124: // current
nit: fix line wrapping...

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:517: return
profile_bubble_ != NULL;
optional nit: nullptr

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.h:67: // designated
browser.
nit: sorry, grammatically, this should be "in a designated"

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler.cc (right):

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler.cc:146: // Close avatar
bubble through any browser since it is a singleton.
nit: consider: "Close the singleton avatar bubble to reveal the full tab UI."

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler.cc:151:
browser->window()->CloseAvatarBubbleFromAvatarButton();
Please duplicate/inline CloseAvatarBubbleFromAvatarButton code here, instead of
using FindAnyBrowser. I think that's better/okay for a couple reasons:
1) Finding a window to close a singleton UI with a static helper is silly.
2) AvatarMenuBubbleView is going away soon (afaik), so this will just become
ProfileChooserView::Hide(), which reduces the pain of duplicating the code.

[gogerald1, 2015-05-20 14:06:24]

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.cc:124: // current
On 2015/05/20 00:18:11, msw wrote:
> nit: fix line wrapping...

Done.

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:517: return
profile_bubble_ != NULL;
On 2015/05/20 00:18:11, msw wrote:
> optional nit: nullptr

Acknowledged. 
it is not the change in this cl, we could do it in another dedicated cl.

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.h:67: // designated
browser.
On 2015/05/20 00:18:12, msw wrote:
> nit: sorry, grammatically, this should be "in a designated"

Done.

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler.cc (right):

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler.cc:146: // Close avatar
bubble through any browser since it is a singleton.
On 2015/05/20 00:18:12, msw wrote:
> nit: consider: "Close the singleton avatar bubble to reveal the full tab UI."

Done.

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler.cc:151:
browser->window()->CloseAvatarBubbleFromAvatarButton();
On 2015/05/20 00:18:12, msw wrote:
> Please duplicate/inline CloseAvatarBubbleFromAvatarButton code here, instead
of
> using FindAnyBrowser. I think that's better/okay for a couple reasons:
> 1) Finding a window to close a singleton UI with a static helper is silly.
> 2) AvatarMenuBubbleView is going away soon (afaik), so this will just become
> ProfileChooserView::Hide(), which reduces the pain of duplicating the code.

Can't do that, since it is not allowed to include ProfileChooserView.h
(ui/views/..) here (ui/webui/...) by rule and this code should work for both
linux/windows and mac

[msw, 2015-05-20 16:54:09]

Okay, lgtm

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/inline_login_handler.cc (right):

https://codereview.chromium.org/1136693002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/inline_login_handler.cc:151:
browser->window()->CloseAvatarBubbleFromAvatarButton();
On 2015/05/20 14:06:24, gogerald1 wrote:
> On 2015/05/20 00:18:12, msw wrote:
> > Please duplicate/inline CloseAvatarBubbleFromAvatarButton code here, instead
> of
> > using FindAnyBrowser. I think that's better/okay for a couple reasons:
> > 1) Finding a window to close a singleton UI with a static helper is silly.
> > 2) AvatarMenuBubbleView is going away soon (afaik), so this will just become
> > ProfileChooserView::Hide(), which reduces the pain of duplicating the code.
> 
> Can't do that, since it is not allowed to include ProfileChooserView.h
> (ui/views/..) here (ui/webui/...) by rule and this code should work for both
> linux/windows and mac

Acknowledged.

[gogerald1, 2015-05-20 17:46:44]

gogerald@chromium.org changed reviewers:
+ jcivelli@chromium.org

[gogerald1, 2015-05-20 17:46:45]

Hi Jay, could you help review "test_browser_window.h" in this cl.

[Jay Civelli, 2015-05-20 17:49:03]

LGTM for chrome/test

[gogerald1, 2015-05-20 18:29:25]

The CQ bit was checked by gogerald@chromium.org

[gogerald1, 2015-05-20 18:29:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
rogerta@chromium.org, groby@chromium.org
Link to the patchset: https://codereview.chromium.org/1136693002/#ps120001
(title: "address Michael's nit comments")

[commit-bot: I haz the power, 2015-05-20 18:29:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1136693002/120001

[commit-bot: I haz the power, 2015-05-20 20:01:35]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-05-20 20:02:27]

Patchset 6 (id:??) landed as
https://crrev.com/131615b8cfd0e9b3045f3384e06ec3d8eb4f272a
Cr-Commit-Position: refs/heads/master@{#330789}

[gogerald1, 2015-07-14 14:15:49]

A revert of this CL (patchset #6 id:120001) has been created in
https://codereview.chromium.org/1235173002/ by gogerald@chromium.org.

The reason for reverting is: This CL reveals issues can not be solved
immediately, so we revert it to allow time to solve them or find alternative
solutions..

[msw, 2015-07-14 16:49:57]

On 2015/07/14 14:15:49, gogerald1 wrote:
> A revert of this CL (patchset #6 id:120001) has been created in
> https://codereview.chromium.org/1235173002/ by mailto:gogerald@chromium.org.
> 
> The reason for reverting is: This CL reveals issues can not be solved
> immediately, so we revert it to allow time to solve them or find alternative
> solutions..

Your revert message should give a more concrete reason for the revert. Why
exactly didn't the CL fix the bug, or what new problem did it introduce? You
wrote the same revert message on another CL,
https://codereview.chromium.org/1237123002/ neither has enough detail to let
anyone understand why the cl is reverted.