Support full-screen and single-profile avatar bubbles.

Support full-screen and single-profile avatar bubbles.

Enable IDC_SHOW_AVATAR_MENU for normal browser windows.
(full-screen and single-profile windows are now allowed)
(non-tabbed and incognito windows are still disallowed)

Nix BrowserCommandController's use of ProfileManager.
(no commands rely on the profile count anymore)
Cleanup some BrowserCommandController code.

Inline AvatarMenuButton::ShowAvatarBubble in BrowserView.
(ShowAvatarBubbleFromAvatarButton needs to set an anchor)
(single-profile windows have no avatar icon; use hotdogs)
Anchor to the Chrome menu button in Mac single-profile too.

Take an ArrowPaintType in AvatarMenuBubbleView::ShowBubble.
(lets fullscreen hide the arrow, there's no visible anchor)
(this corresponds with fullscreen bookmark bubble behavior)

Remove a stale Windows Valgrind DrMemory test exclusion entry.
(test name was changed, but the exclusion wasn't; isn't needed)

BUG=255452, 167014
TEST=The avatar menu shows for Ctrl+Shift+M even in fullscreen and even for single-profile Chrome windows. Test with and without the experimental --new-profile-management, and in LTR and RTL Chrome UI modes.
R=pkasting@chromium.org, asvitkine@chromium.org

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

[msw, 2014-05-07 19:38:10]

Hey Peter, please take a look; thanks!

[Peter Kasting, 2014-05-07 20:58:44]

https://codereview.chromium.org/264303006/diff/1/chrome/browser/ui/views/prof...
File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (left):

https://codereview.chromium.org/264303006/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:494:
DCHECK(chrome::IsCommandEnabled(browser, IDC_SHOW_AVATAR_MENU));
So it actually makes sense to show the menu in the single profile/fullscreen
cases?  If so why don't we also enable the command at those times?

[msw, 2014-05-07 21:31:07]

https://codereview.chromium.org/264303006/diff/1/chrome/browser/ui/views/prof...
File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (left):

https://codereview.chromium.org/264303006/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:494:
DCHECK(chrome::IsCommandEnabled(browser, IDC_SHOW_AVATAR_MENU));
On 2014/05/07 20:58:44, Peter Kasting wrote:
> So it actually makes sense to show the menu in the single profile/fullscreen
> cases?  If so why don't we also enable the command at those times?

When enabling the command also enables the Ctrl+Shift+M accelerator (eye roll)
to show the avatar menu from the avatar icon. That icon doesn't exist in the
single-user or fullscreen cases. It seems wrong to additionally enable the
command when the apps page is active and then conditionally anchor the menu to
the apps page link, but your advice is welcome.

[Peter Kasting, 2014-05-07 21:33:36]

https://codereview.chromium.org/264303006/diff/1/chrome/browser/ui/views/prof...
File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (left):

https://codereview.chromium.org/264303006/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:494:
DCHECK(chrome::IsCommandEnabled(browser, IDC_SHOW_AVATAR_MENU));
On 2014/05/07 21:31:07, msw wrote:
> On 2014/05/07 20:58:44, Peter Kasting wrote:
> > So it actually makes sense to show the menu in the single profile/fullscreen
> > cases?  If so why don't we also enable the command at those times?
> 
> When enabling the command also enables the Ctrl+Shift+M accelerator (eye roll)
> to show the avatar menu from the avatar icon. That icon doesn't exist in the
> single-user or fullscreen cases. It seems wrong to additionally enable the
> command when the apps page is active and then conditionally anchor the menu to
> the apps page link, but your advice is welcome.

Consider:

F11, ctrl-D

This bookmarks the current page and pops up the bookmark bubble even though the
star is invisible.

If we're going to have a shortcut for the avatar menu, and the menu will get
shown onscreen, and the menu can already be triggered without the icon visible
anyway, I say let's just do like with the star and enable the command at those
times.  People goofy enough to know and use this keyboard shortcut will probably
be happy and no one else will notice.

[msw, 2014-05-07 21:47:42]

https://codereview.chromium.org/264303006/diff/1/chrome/browser/ui/views/prof...
File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (left):

https://codereview.chromium.org/264303006/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:494:
DCHECK(chrome::IsCommandEnabled(browser, IDC_SHOW_AVATAR_MENU));
On 2014/05/07 21:33:36, Peter Kasting wrote:
> On 2014/05/07 21:31:07, msw wrote:
> > On 2014/05/07 20:58:44, Peter Kasting wrote:
> > > So it actually makes sense to show the menu in the single
profile/fullscreen
> > > cases?  If so why don't we also enable the command at those times?
> > 
> > When enabling the command also enables the Ctrl+Shift+M accelerator (eye
roll)
> > to show the avatar menu from the avatar icon. That icon doesn't exist in the
> > single-user or fullscreen cases. It seems wrong to additionally enable the
> > command when the apps page is active and then conditionally anchor the menu
to
> > the apps page link, but your advice is welcome.
> 
> Consider:
> 
> F11, ctrl-D
> 
> This bookmarks the current page and pops up the bookmark bubble even though
the
> star is invisible.
> 
> If we're going to have a shortcut for the avatar menu, and the menu will get
> shown onscreen, and the menu can already be triggered without the icon visible
> anyway, I say let's just do like with the star and enable the command at those
> times.  People goofy enough to know and use this keyboard shortcut will
probably
> be happy and no one else will notice.

That's reasonable, but is it worth adding this functionality to the avatar
bubble when there are efforts underway to replace it altogether?

[Peter Kasting, 2014-05-07 21:50:04]

https://codereview.chromium.org/264303006/diff/1/chrome/browser/ui/views/prof...
File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (left):

https://codereview.chromium.org/264303006/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:494:
DCHECK(chrome::IsCommandEnabled(browser, IDC_SHOW_AVATAR_MENU));
On 2014/05/07 21:47:43, msw wrote:
> On 2014/05/07 21:33:36, Peter Kasting wrote:
> > On 2014/05/07 21:31:07, msw wrote:
> > > On 2014/05/07 20:58:44, Peter Kasting wrote:
> > > > So it actually makes sense to show the menu in the single
> profile/fullscreen
> > > > cases?  If so why don't we also enable the command at those times?
> > > 
> > > When enabling the command also enables the Ctrl+Shift+M accelerator (eye
> roll)
> > > to show the avatar menu from the avatar icon. That icon doesn't exist in
the
> > > single-user or fullscreen cases. It seems wrong to additionally enable the
> > > command when the apps page is active and then conditionally anchor the
menu
> to
> > > the apps page link, but your advice is welcome.
> > 
> > Consider:
> > 
> > F11, ctrl-D
> > 
> > This bookmarks the current page and pops up the bookmark bubble even though
> the
> > star is invisible.
> > 
> > If we're going to have a shortcut for the avatar menu, and the menu will get
> > shown onscreen, and the menu can already be triggered without the icon
visible
> > anyway, I say let's just do like with the star and enable the command at
those
> > times.  People goofy enough to know and use this keyboard shortcut will
> probably
> > be happy and no one else will notice.
> 
> That's reasonable, but is it worth adding this functionality to the avatar
> bubble when there are efforts underway to replace it altogether?

Are we really adding functionality?  I thought all the code already existed and
worked in all these cases, and the only issue is that you'd be making a one-line
change or so to not disable the command as frequently.

[msw, 2014-05-07 23:19:18]

https://codereview.chromium.org/264303006/diff/1/chrome/browser/ui/views/prof...
File chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc (left):

https://codereview.chromium.org/264303006/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/avatar_menu_bubble_view.cc:494:
DCHECK(chrome::IsCommandEnabled(browser, IDC_SHOW_AVATAR_MENU));
On 2014/05/07 21:50:05, Peter Kasting wrote:
> On 2014/05/07 21:47:43, msw wrote:
> > On 2014/05/07 21:33:36, Peter Kasting wrote:
> > > On 2014/05/07 21:31:07, msw wrote:
> > > > On 2014/05/07 20:58:44, Peter Kasting wrote:
> > > > > So it actually makes sense to show the menu in the single
> > profile/fullscreen
> > > > > cases?  If so why don't we also enable the command at those times?
> > > > 
> > > > When enabling the command also enables the Ctrl+Shift+M accelerator (eye
> > roll)
> > > > to show the avatar menu from the avatar icon. That icon doesn't exist in
> the
> > > > single-user or fullscreen cases. It seems wrong to additionally enable
the
> > > > command when the apps page is active and then conditionally anchor the
> menu
> > to
> > > > the apps page link, but your advice is welcome.
> > > 
> > > Consider:
> > > 
> > > F11, ctrl-D
> > > 
> > > This bookmarks the current page and pops up the bookmark bubble even
though
> > the
> > > star is invisible.
> > > 
> > > If we're going to have a shortcut for the avatar menu, and the menu will
get
> > > shown onscreen, and the menu can already be triggered without the icon
> visible
> > > anyway, I say let's just do like with the star and enable the command at
> those
> > > times.  People goofy enough to know and use this keyboard shortcut will
> > probably
> > > be happy and no one else will notice.
> > 
> > That's reasonable, but is it worth adding this functionality to the avatar
> > bubble when there are efforts underway to replace it altogether?
> 
> Are we really adding functionality?  I thought all the code already existed
and
> worked in all these cases, and the only issue is that you'd be making a
one-line
> change or so to not disable the command as frequently.

The latest patch set adds support for the command in fullscreen. The only new
real functionality is hiding the arrow for bubbles shown while the avatar icon
is hidden in fullscreen. The command is still disabled when there is only a
single profile, so this DCHECK is still incorrect. Thoughts?

[Peter Kasting, 2014-05-07 23:21:00]

On 2014/05/07 23:19:18, msw wrote:
> The latest patch set adds support for the command in fullscreen. The only new
> real functionality is hiding the arrow for bubbles shown while the avatar icon
> is hidden in fullscreen. The command is still disabled when there is only a
> single profile, so this DCHECK is still incorrect. Thoughts?

The core question is, "does the menu make sense when there is only a single
profile"?  If yes, then the command should just be enabled.  If no, then the
link shouldn't be there to trigger it either.

If the menu has options like "create a new profile" and such, then the answer
may well be "yes".

[msw, 2014-05-07 23:50:02]

On 2014/05/07 23:21:00, Peter Kasting wrote:
> On 2014/05/07 23:19:18, msw wrote:
> > The latest patch set adds support for the command in fullscreen. The only
new
> > real functionality is hiding the arrow for bubbles shown while the avatar
icon
> > is hidden in fullscreen. The command is still disabled when there is only a
> > single profile, so this DCHECK is still incorrect. Thoughts?
> 
> The core question is, "does the menu make sense when there is only a single
> profile"?  If yes, then the command should just be enabled.  If no, then the
> link shouldn't be there to trigger it either.
> 
> If the menu has options like "create a new profile" and such, then the answer
> may well be "yes".

Again, I really think it's a waste of time to polish such inconsequential
behavior for something that's actively being replaced. The command being
disabled in certain scenarios isn't really impeding any important user action,
and getting the details of newly enabled scenarios right is non-trivial. Anyway,
the menu does show an add user button, so are you proposing that:
1) BrowserCommandController should additionally enable the command for
single-user windows when the active tab state changes to show/hide the
about:apps page, and then ShowAvatarMenu (called by ExecuteCommand) should
obtain the web contents rect corresponding to the login link and conditionally
invoke BrowserWindow::ShowAvatarBubble instead of just always calling
ShowAvatarBubbleFromAvatarButton?
2) The command should never be disabled simply because there's only one profile,
and if the keyboard shortcut is then invoked when there is no avatar icon, the
bubble should be anchored to ... the chrome menu, I guess?

[Peter Kasting, 2014-05-07 23:57:00]

On 2014/05/07 23:50:02, msw wrote:
> Anyway,
> the menu does show an add user button, so are you proposing that:
> 1) BrowserCommandController should additionally enable the command for
> single-user windows when the active tab state changes to show/hide the
> about:apps page, and then ShowAvatarMenu (called by ExecuteCommand) should
> obtain the web contents rect corresponding to the login link and conditionally
> invoke BrowserWindow::ShowAvatarBubble instead of just always calling
> ShowAvatarBubbleFromAvatarButton?
> 2) The command should never be disabled simply because there's only one
profile,
> and if the keyboard shortcut is then invoked when there is no avatar icon, the
> bubble should be anchored to ... the chrome menu, I guess?

(2).  Just enable it all the time and be done.  And again, I assumed we already
have an idea of how to show the menu when there's no avatar icon to target,
since we do it today sometimes.

This whole conversation has been me continually trying to suggest a path that's
intended to be very few lines of code and no meaningful added functionality over
today.  I've tried to make my assumptions that that's an accurate summary true
several times.  So when you keep repeating that you don't want to make
significant functional additions, you're complaining about something that I
haven't been trying to make you do.

If my assumptions are false, then make a different call.

[msw, 2014-05-09 01:10:12]

On 2014/05/07 23:57:00, Peter Kasting wrote:
> On 2014/05/07 23:50:02, msw wrote:
> > Anyway,
> > the menu does show an add user button, so are you proposing that:
> > 1) BrowserCommandController should additionally enable the command for
> > single-user windows when the active tab state changes to show/hide the
> > about:apps page, and then ShowAvatarMenu (called by ExecuteCommand) should
> > obtain the web contents rect corresponding to the login link and
conditionally
> > invoke BrowserWindow::ShowAvatarBubble instead of just always calling
> > ShowAvatarBubbleFromAvatarButton?
> > 2) The command should never be disabled simply because there's only one
> profile,
> > and if the keyboard shortcut is then invoked when there is no avatar icon,
the
> > bubble should be anchored to ... the chrome menu, I guess?
> 
> (2).  Just enable it all the time and be done.  And again, I assumed we
already
> have an idea of how to show the menu when there's no avatar icon to target,
> since we do it today sometimes.

There's no generalized good placement for an un-anchored bubble. The fullscreen
case works without much change, because the avatar icon is simply hidden and
still provides a reasonable anchor rect for a reasonable bubble placement (in
the top left for LTR and top-right for RTL). I've revised the code to support
the single-profile case, where there is no avatar icon at all, by anchoring to
the chrome menu; this required some refactoring.

> This whole conversation has been me continually trying to suggest a path
that's
> intended to be very few lines of code and no meaningful added functionality
over
> today.  I've tried to make my assumptions that that's an accurate summary true
> several times.  So when you keep repeating that you don't want to make
> significant functional additions, you're complaining about something that I
> haven't been trying to make you do.
> 
> If my assumptions are false, then make a different call.

Sorry for my complaints; this initially seemed like a simple overeager DCHECK
for a dying scenario. I think the end result of supporting fullscreen and
single-profile is a marginal improvement, and it does actually apply to the new
profile management UI.

Please take another look; I'll loop in others if you approve.

[msw, 2014-05-12 23:04:05]

Friendly ping, please take a look as you have time; thanks!

[Peter Kasting, 2014-05-12 23:38:24]

LGTM

[msw, 2014-05-13 08:11:42]

Hey Alexei, please review c/b/ui/cocoa/browser_window_cocoa.mm; thanks!

[Alexei Svitkine (slow), 2014-05-13 14:42:20]

cocoa lgtm

[msw, 2014-05-13 14:43:03]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-05-13 14:43:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/264303006/170001

[commit-bot: I haz the power, 2014-05-13 16:47:16]

Change committed as 270126