Bring up the Fast User Switcher on Shift+Click on the new Avatar Button

Bring up the Fast User Switcher on Shift+Click on the new Avatar Button

BUG=403619
TEST
1. Enable the New Avatar Menu and Fast User Switcher flags in chrome://flags
2a. Left click on the new avatar button, the entire menu should appear (tutorials if applicable, current user avatar, list of profiles to switch to, options buttons)
2b. Right click on the new avatar button, only the list of available profiles should show up.

Committed: https://crrev.com/a0a82fe8d1fa9fbf10b0a4cccf7542e74c707680
Cr-Commit-Position: refs/heads/master@{#315643}

[Roger Tawa OOO till Jul 10th, 2015-01-26 20:30:39]

rogerta@chromium.org changed reviewers:
+ rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2015-01-26 20:30:41]

Looks great! Just a few nits.

https://codereview.chromium.org/869793004/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/869793004/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:332:
BrowserWindow::AVATAR_BUBBLE_MODE_DEFAULT;
Inent 331 and 332 two more spaces.

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

https://codereview.chromium.org/869793004/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:85: mode ==
profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER;
Indent 85 only 4 spaces.

https://codereview.chromium.org/869793004/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:975:
layout->AddView(CreateOtherProfilesView(other_profiles));
Add { and } to if body.

[anthonyvd, 2015-01-28 22:04:38]

https://codereview.chromium.org/869793004/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/869793004/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:332:
BrowserWindow::AVATAR_BUBBLE_MODE_DEFAULT;
On 2015/01/26 20:30:40, Roger Tawa wrote:
> Inent 331 and 332 two more spaces.

Done.

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

https://codereview.chromium.org/869793004/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:85: mode ==
profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER;
On 2015/01/26 20:30:40, Roger Tawa wrote:
> Indent 85 only 4 spaces.

Done.

https://codereview.chromium.org/869793004/diff/1/chrome/browser/ui/views/prof...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:975:
layout->AddView(CreateOtherProfilesView(other_profiles));
On 2015/01/26 20:30:40, Roger Tawa wrote:
> Add { and } to if body.

Done.

[Roger Tawa OOO till Jul 10th, 2015-01-29 23:00:35]

lgtm, some nits below.

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

https://codereview.chromium.org/869793004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:214:
BrowserWindow::AVATAR_BUBBLE_MODE_DEFAULT;
Two spaces.

https://codereview.chromium.org/869793004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:218: }
No need for { and }

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

https://codereview.chromium.org/869793004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:334:
static_cast<const ui::MouseEvent&>(event).IsLeftMouseButton()){
Space before {

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

https://codereview.chromium.org/869793004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:85: mode ==
profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER;
Two more spaces.

https://codereview.chromium.org/869793004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:973: view_mode_ ==
profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER) {
Do we still need this view_mode_ check?

[anthonyvd, 2015-01-30 15:02:20]

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

https://codereview.chromium.org/869793004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:214:
BrowserWindow::AVATAR_BUBBLE_MODE_DEFAULT;
On 2015/01/29 23:00:35, Roger Tawa wrote:
> Two spaces.

Done.

https://codereview.chromium.org/869793004/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:218: }
On 2015/01/29 23:00:35, Roger Tawa wrote:
> No need for { and }

Done.

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

https://codereview.chromium.org/869793004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:334:
static_cast<const ui::MouseEvent&>(event).IsLeftMouseButton()){
On 2015/01/29 23:00:35, Roger Tawa wrote:
> Space before {

Done.

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

https://codereview.chromium.org/869793004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:85: mode ==
profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER;
On 2015/01/29 23:00:35, Roger Tawa wrote:
> Two more spaces.

Done.

https://codereview.chromium.org/869793004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:973: view_mode_ ==
profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER) {
On 2015/01/29 23:00:35, Roger Tawa wrote:
> Do we still need this view_mode_ check?

Done.

[anthonyvd, 2015-01-30 21:32:30]

anthonyvd@chromium.org changed reviewers:
+ asvitkine@chromium.org, noms@chromium.org

[anthonyvd, 2015-01-30 21:32:31]

Hi guys,  could you please take a look at:

noms@: profile_window.cc
asvitkine@: both files under chrome/browser/cocoa/profiles

Thanks a lot!

[Alexei Svitkine (slow), 2015-02-02 17:22:31]

Please associate this CL with a crbug and add a TEST= line that gives steps how
this change can be tested from the UI. Thanks!

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

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:216: if([NSEvent
modifierFlags] & NSCommandKeyMask)
Nit: space after if

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1269:
if(viewMode_ != profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER) {
Space after if

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1283:
switches::IsFastUserSwitching()) ||
Nit: Align with viewMode_ above.

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1327: if
(viewMode_ != profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER &&
Nit: Maybe make a BOOL local variable isFastProfileChooser and change all of
these to !isFastProfileChooser to make the code more concise?

[noms (inactive), 2015-02-03 15:29:50]

Looking good! Some initial nits :)

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_window.cc:505: *bubble_view_mode =
profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER;
nit: no {}

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:331:
BrowserWindow::AVATAR_BUBBLE_MODE_DEFAULT;
nit: indent by 2 more

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:335:
!static_cast<const ui::MouseEvent&>(event).IsRightMouseButton()) {
I'm wondering if this can this be simplified to

if (!switches::IsFastUserSwitching() && event.IsRightMouseButton())
  // show fast switch bubble
else 
 // show normal bubble

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:341: if
(event.IsMouseEvent() &&
You should probably return early if !event.IsMouseEvent(), no? Or, if this can
never happen, change it to a DCHECK (how could you even trigger a button click
from not a mouse event?)

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

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:53:
ui::EF_LEFT_MOUSE_BUTTON | ui::EF_RIGHT_MOUSE_BUTTON);
nit: indent by 2 more

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

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1007:
PopulateMinimalProfileChooserView(layout, avatar_menu);
nit: no {}

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

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.h:128: void
PopulateCompleteProfileChooserView(views::GridLayout* layout,
nit: add comments explaining what "minimal" and "complete" means.

[anthonyvd, 2015-02-03 21:53:21]

Addressed those comments, thanks guys :)

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_window.cc:505: *bubble_view_mode =
profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER;
On 2015/02/03 15:29:49, Monica Dinculescu wrote:
> nit: no {}

Done.

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

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_base_controller.mm:216: if([NSEvent
modifierFlags] & NSCommandKeyMask)
On 2015/02/02 17:22:30, asvitkine (slow this week) wrote:
> Nit: space after if

Done.

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1269:
if(viewMode_ != profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER) {
On 2015/02/02 17:22:31, asvitkine (slow this week) wrote:
> Space after if

Done.

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1283:
switches::IsFastUserSwitching()) ||
On 2015/02/02 17:22:30, asvitkine (slow this week) wrote:
> Nit: Align with viewMode_ above.

Done.

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1327: if
(viewMode_ != profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER &&
On 2015/02/02 17:22:31, asvitkine (slow this week) wrote:
> Nit: Maybe make a BOOL local variable isFastProfileChooser and change all of
> these to !isFastProfileChooser to make the code more concise?

Done.

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/opaque_browser_frame_view.cc (right):

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:331:
BrowserWindow::AVATAR_BUBBLE_MODE_DEFAULT;
On 2015/02/03 15:29:49, Monica Dinculescu wrote:
> nit: indent by 2 more

Done.

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:335:
!static_cast<const ui::MouseEvent&>(event).IsRightMouseButton()) {
On 2015/02/03 15:29:49, Monica Dinculescu wrote:
> I'm wondering if this can this be simplified to
> 
> if (!switches::IsFastUserSwitching() && event.IsRightMouseButton())
>   // show fast switch bubble
> else 
>  // show normal bubble 
>  

Done.

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/opaque_browser_frame_view.cc:341: if
(event.IsMouseEvent() &&
On 2015/02/03 15:29:49, Monica Dinculescu wrote:
> You should probably return early if !event.IsMouseEvent(), no? Or, if this can
> never happen, change it to a DCHECK (how could you even trigger a button click
> from not a mouse event?)

Done.

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

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:53:
ui::EF_LEFT_MOUSE_BUTTON | ui::EF_RIGHT_MOUSE_BUTTON);
On 2015/02/03 15:29:49, Monica Dinculescu wrote:
> nit: indent by 2 more

Done.

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

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:1007:
PopulateMinimalProfileChooserView(layout, avatar_menu);
On 2015/02/03 15:29:49, Monica Dinculescu wrote:
> nit: no {}

Done.

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

https://codereview.chromium.org/869793004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/profile_chooser_view.h:128: void
PopulateCompleteProfileChooserView(views::GridLayout* layout,
On 2015/02/03 15:29:50, Monica Dinculescu wrote:
> nit: add comments explaining what "minimal" and "complete" means.

Done.

[Alexei Svitkine (slow), 2015-02-06 15:23:31]

https://codereview.chromium.org/869793004/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/869793004/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1330:
currentProfileView) {
Don't wrap if it fits. Please fix throughout.

[noms (inactive), 2015-02-10 18:31:18]

profiles lgtm

[anthonyvd, 2015-02-10 18:47:59]

New patchsets have been uploaded after l-g-t-m from noms@chromium.org

[anthonyvd, 2015-02-10 18:49:33]

https://codereview.chromium.org/869793004/diff/80001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm (right):

https://codereview.chromium.org/869793004/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/profile_chooser_controller.mm:1330:
currentProfileView) {
On 2015/02/06 15:23:31, Alexei Svitkine wrote:
> Don't wrap if it fits. Please fix throughout.

Done.

[anthonyvd, 2015-02-10 18:52:16]

anthonyvd@chromium.org changed reviewers:
+ sky@chromium.org

[anthonyvd, 2015-02-10 18:52:17]

sky@ Could you please take a look at
  browser_window.h
  profile_chooser_constants.h
  chrome/browser/ui/views/*

Thanks a lot!

[Alexei Svitkine (slow), 2015-02-10 19:49:51]

lgtm

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

https://codereview.chromium.org/869793004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:988: AvatarMenu*
avatar_menu) {
Can this be a const pointer or even const ref? (If so, layout should be last
param since it's a modifiable param).

https://codereview.chromium.org/869793004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:992: if (!item.active)
{
nit: Just inline the call into the if and remove the {}

if (!avatar_menu->GetItemAt(i).active)
  other_profiles.push_back(i);

[sky, 2015-02-10 20:19:45]

LGTM

[anthonyvd, 2015-02-10 20:25:08]

New patchsets have been uploaded after l-g-t-m from
asvitkine@chromium.org,sky@chromium.org

[anthonyvd, 2015-02-10 21:27:18]

The CQ bit was checked by anthonyvd@chromium.org

[commit-bot: I haz the power, 2015-02-10 21:28:11]

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

[commit-bot: I haz the power, 2015-02-10 21:32:34]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-10 21:33:15]

Patchset 7 (id:??) landed as
https://crrev.com/a0a82fe8d1fa9fbf10b0a4cccf7542e74c707680
Cr-Commit-Position: refs/heads/master@{#315643}