Add managed user avatar menu for mac.

Add managed user avatar menu (cocoa).

The mocks for this can be found at the beginning of the bug
description. Please look at the screenshots at the end of the bug description.
The colors do not correspond to the mocks yet, they will be added as a theme.

BUG=241387
TEST=unit_tests

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

[Adrian Kuegel, 2013-05-31 17:11:40]

Sailesh, can you please review this CL?
Btw, the compile error of the tryjob is caused by the trybots not having the
required revision yet on which I rebased this. Locally it compiled.

[sail, 2013-05-31 17:25:11]

https://codereview.chromium.org/15932008/diff/24035/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/15932008/diff/24035/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:84: #if
defined(ENABLE_MANAGED_USERS)
Can we get rid of this ifdef from the codebase before this CL goes in?

[Adrian Kuegel, 2013-05-31 20:28:16]

https://codereview.chromium.org/15932008/diff/24035/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/15932008/diff/24035/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:84: #if
defined(ENABLE_MANAGED_USERS)
Yes. I removed it, and it looks like it also works without it. We only have this
flag set to false for iOS and android, so if this file is not included there,
anyway, we don't need this ifdef.

[sail, 2013-05-31 20:41:44]

https://codereview.chromium.org/15932008/diff/24035/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/15932008/diff/24035/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:84: #if
defined(ENABLE_MANAGED_USERS)
On 2013/05/31 20:28:16, Adrian Kuegel wrote:
> Yes. I removed it, and it looks like it also works without it. We only have
this
> flag set to false for iOS and android, so if this file is not included there,
> anyway, we don't need this ifdef.

Can't we just kill this ifdef though?

[Adrian Kuegel, 2013-05-31 20:48:12]

It cannot be killed so fast, since it is used in many places. But it is not
anymore in this CL (if I didn't overlook something).

[Adrian Kuegel, 2013-06-03 16:18:32]

As you have probably seen, I have started removing those ifdefs in another CL.
Since there is no such ifdef in this CL anymore, it should be independent of the
other CL landing. So could you please take another look?

[Robert Sesek, 2013-06-03 19:10:57]

Some high-level comments:

* I think it'd be better to encapsulate AvatarLabelController in
AvatarButtonController. BrowserWindowController already knows too much about
everything, and since the label is always shown with the button, they should
really just be one unit.
* The avatar label should not have a background.
* The managed user text is using Helvetica. It should be using the label font
instead (Lucida Grande).

Also, please be sure you find the most appropriate reviewer for your changes. A
quick blame of these files would have indicated that I authored them originally
and should definitely be included in the review.

[Adrian Kuegel, 2013-06-04 15:49:14]

@Robert: Thanks for your review. Sorry for not including you as a reviewer. I
asked Sailesh to review it because he was already familiar with the feature I am
working on, and he reviewed the profiles part of it in the CL for views. But I
guess I should have asked you, too.

About the background: I am not sure if I understand your concern. If it is a UI
concern, then please consider that we will also change the frame background by
our theme (still tbd) according to the mocks we got from UI people. And in the
mocks the label has a background (although with a different color). Also, these
changes are not visible to normal users (only to those which enabled a flag and
created a managed user profile).

I use now the labelFont method to determine the font, I hope that is correct.
Also, I have put the label code in the AvatarButtonController. I also updated
the screenshots in the bug description.

On 2013/06/03 19:10:57, rsesek wrote:
> Some high-level comments:
> 
> * I think it'd be better to encapsulate AvatarLabelController in
> AvatarButtonController. BrowserWindowController already knows too much about
> everything, and since the label is always shown with the button, they should
> really just be one unit.
> * The avatar label should not have a background.
> * The managed user text is using Helvetica. It should be using the label font
> instead (Lucida Grande).
> 
> Also, please be sure you find the most appropriate reviewer for your changes.
A
> quick blame of these files would have indicated that I authored them
originally
> and should definitely be included in the review.

[Robert Sesek, 2013-06-04 20:59:09]

On 2013/06/04 15:49:14, Adrian Kuegel wrote:
> @Robert: Thanks for your review. Sorry for not including you as a reviewer. I
> asked Sailesh to review it because he was already familiar with the feature I
am
> working on, and he reviewed the profiles part of it in the CL for views. But I
> guess I should have asked you, too.

No harm, no foul. Sailesh is definitely a capable reviewer for this code, but
you should always include original authors.

> About the background: I am not sure if I understand your concern. If it is a
UI
> concern, then please consider that we will also change the frame background by
> our theme (still tbd) according to the mocks we got from UI people. And in the
> mocks the label has a background (although with a different color). Also,
these
> changes are not visible to normal users (only to those which enabled a flag
and
> created a managed user profile).

Ah, I didn't realize the background color was intentional. It looks kinda weird
:/.

> I use now the labelFont method to determine the font, I hope that is correct.
> Also, I have put the label code in the AvatarButtonController. I also updated
> the screenshots in the bug description.

Hm, that still looks like Helvetica in the menu/bubble.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/avatar_button_controller.h (right):

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_button_controller.h:10: #include
"base/memory/scoped_nsobject.h"
#import

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_button_controller.h:46: @property
(readonly, nonatomic) NSTextField* labelView;
nit: (and please fix the one above) no space before ( for @property

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/avatar_button_controller.mm (right):

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_button_controller.mm:144:
initWithFrame:NSMakeRect(0, 0, CGFLOAT_MAX, CGFLOAT_MAX)]);
Since you're calling -sizeToFit, NSZeroRect is okay.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right):

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:33:
updateWidthAdjust:(CGFloat*)widthAdjust
nit: this line should only be indented 4 spaces, and then align the colons of
the other params

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:304:
attributes:nil] autorelease];
Chromium has a preference for scoped_nsobject<T> to better control lifetime of
locals, rather than autoreleasing.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:307:
NSMakeRect(5, yOffset, width - 10, CGFLOAT_MAX)] autorelease];
Why CGFLOAT_MAX instead of 0 since you're just calling -sizeToFit below?

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:331: -
(NSButton*)configureSwitchUserButton:(CGFloat)yOffset {
The order of the implementations should match the declaration order.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:334: 90, 16)]);
Where do these numbers come from?

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:344: return
[newButton.release() autorelease];
scoped_nsobject<T>::autorelease()

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:84: 
nit: extra blank line

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:1740: [[[[self window]
contentView] superview] addSubview:label];
Why not just have this be in the view that AvatarButtonController provides?

[Adrian Kuegel, 2013-06-05 16:01:38]

Yes, I forgot to change the font for the managed user info in the avatar menu, I
just changed it for the avatar label. I fixed that now.
Unfortunately I didn't have access to a mac today (I don't have a mac myself),
so I could only use the trybot to verify if my changes compile. Could you please
still take a look at the code if you see other things I should fix?

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/avatar_button_controller.h (right):

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_button_controller.h:10: #include
"base/memory/scoped_nsobject.h"
On 2013/06/04 20:59:10, rsesek wrote:
> #import

Done.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_button_controller.h:46: @property
(readonly, nonatomic) NSTextField* labelView;
On 2013/06/04 20:59:10, rsesek wrote:
> nit: (and please fix the one above) no space before ( for @property

Done.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/avatar_button_controller.mm (right):

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_button_controller.mm:144:
initWithFrame:NSMakeRect(0, 0, CGFLOAT_MAX, CGFLOAT_MAX)]);
On 2013/06/04 20:59:10, rsesek wrote:
> Since you're calling -sizeToFit, NSZeroRect is okay.

Done.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right):

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:33:
updateWidthAdjust:(CGFloat*)widthAdjust
On 2013/06/04 20:59:10, rsesek wrote:
> nit: this line should only be indented 4 spaces, and then align the colons of
> the other params

I changed it so all colons align with the colon in the first line. Is that ok,
too?

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:304:
attributes:nil] autorelease];
On 2013/06/04 20:59:10, rsesek wrote:
> Chromium has a preference for scoped_nsobject<T> to better control lifetime of
> locals, rather than autoreleasing.

Done.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:307:
NSMakeRect(5, yOffset, width - 10, CGFLOAT_MAX)] autorelease];
On 2013/06/04 20:59:10, rsesek wrote:
> Why CGFLOAT_MAX instead of 0 since you're just calling -sizeToFit below?

I am not familiar with cocoa code, that's why :) I changed it now.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:331: -
(NSButton*)configureSwitchUserButton:(CGFloat)yOffset {
On 2013/06/04 20:59:10, rsesek wrote:
> The order of the implementations should match the declaration order.

Done.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:334: 90, 16)]);
I just copied them from above (configureNewUserButton). But I guess we should
make it dependent on kBubbleMinSize and adjust the width according to the
(possible) resize.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:344: return
[newButton.release() autorelease];
On 2013/06/04 20:59:10, rsesek wrote:
> scoped_nsobject<T>::autorelease()

Done.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:84: 
On 2013/06/04 20:59:10, rsesek wrote:
> nit: extra blank line

Removed.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:1740: [[[[self window]
contentView] superview] addSubview:label];
On 2013/06/04 20:59:10, rsesek wrote:
> Why not just have this be in the view that AvatarButtonController provides?

Done.

[Robert Sesek, 2013-06-05 20:59:17]

This looks pretty good. I'd like to see screenshots again, though.

I'm also OOO tomorrow, btw.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right):

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:33:
updateWidthAdjust:(CGFloat*)widthAdjust
On 2013/06/05 16:01:38, Adrian Kuegel wrote:
> On 2013/06/04 20:59:10, rsesek wrote:
> > nit: this line should only be indented 4 spaces, and then align the colons
of
> > the other params
> 
> I changed it so all colons align with the colon in the first line. Is that ok,
> too?

Yup.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:1740: [[[[self window]
contentView] superview] addSubview:label];
On 2013/06/05 16:01:38, Adrian Kuegel wrote:
> On 2013/06/04 20:59:10, rsesek wrote:
> > Why not just have this be in the view that AvatarButtonController provides?
> 
> Done.

Not sure this was actually done.

https://codereview.chromium.org/15932008/diff/60002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/avatar_button_controller.mm (right):

https://codereview.chromium.org/15932008/diff/60002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_button_controller.mm:150:
l10n_util::GetNSString(IDS_MANAGED_USER_AVATAR_LABEL)];
nit: just indent 4 spaces

https://codereview.chromium.org/15932008/diff/60002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right):

https://codereview.chromium.org/15932008/diff/60002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:46: -
(IBAction)switchProfile:(id)sender;
nit: put this in the public .h since it's in the public section of the impl

[Adrian Kuegel, 2013-06-07 15:31:26]

I uploaded new screenshots to the bug description. Please take another look.
I will add the limited user icon you can see in the mocks in a separate CL. It
would be great if this CL can land as soon as possible, because we need the
functionality in place by monday. I will be available a few more hours today to
fix potential issues that are remaining.

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/15932008/diff/47002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser_window_controller.mm:1740: [[[[self window]
contentView] superview] addSubview:label];
On 2013/06/05 20:59:17, rsesek wrote:
> On 2013/06/05 16:01:38, Adrian Kuegel wrote:
> > On 2013/06/04 20:59:10, rsesek wrote:
> > > Why not just have this be in the view that AvatarButtonController
provides?
> > 
> > Done.
> 
> Not sure this was actually done.

Yes, you are right, this was not working so far. This was a bit more difficult
than I expected. I added now a container which contains both the avatar button
and the avatar label (if both are present; otherwise of course it only contains
the avatar button).

https://codereview.chromium.org/15932008/diff/60002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/avatar_button_controller.mm (right):

https://codereview.chromium.org/15932008/diff/60002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_button_controller.mm:150:
l10n_util::GetNSString(IDS_MANAGED_USER_AVATAR_LABEL)];
On 2013/06/05 20:59:17, rsesek wrote:
> nit: just indent 4 spaces

Done.

https://codereview.chromium.org/15932008/diff/60002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm (right):

https://codereview.chromium.org/15932008/diff/60002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_menu_bubble_controller.mm:46: -
(IBAction)switchProfile:(id)sender;
On 2013/06/05 20:59:17, rsesek wrote:
> nit: put this in the public .h since it's in the public section of the impl

Done.

[Robert Sesek, 2013-06-07 16:06:15]

Nice work! Just needs a test now.

https://codereview.chromium.org/15932008/diff/75002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/avatar_button_controller_unittest.mm
(right):

https://codereview.chromium.org/15932008/diff/75002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_button_controller_unittest.mm:78: }
Can you add a test for the managed mode label?

[Adrian Kuegel, 2013-06-07 16:14:09]

https://codereview.chromium.org/15932008/diff/75002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/avatar_button_controller_unittest.mm
(right):

https://codereview.chromium.org/15932008/diff/75002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_button_controller_unittest.mm:78: }
On 2013/06/07 16:06:15, rsesek wrote:
> Can you add a test for the managed mode label?

Sure, I will do that now. Thanks for the fast reply :)

[Adrian Kuegel, 2013-06-07 16:49:56]

https://codereview.chromium.org/15932008/diff/75002/chrome/browser/ui/cocoa/b...
File chrome/browser/ui/cocoa/browser/avatar_button_controller_unittest.mm
(right):

https://codereview.chromium.org/15932008/diff/75002/chrome/browser/ui/cocoa/b...
chrome/browser/ui/cocoa/browser/avatar_button_controller_unittest.mm:78: }
On 2013/06/07 16:14:10, Adrian Kuegel wrote:
> On 2013/06/07 16:06:15, rsesek wrote:
> > Can you add a test for the managed mode label?
> 
> Sure, I will do that now. Thanks for the fast reply :)

Done.

[Robert Sesek, 2013-06-07 17:07:00]

LGTM

[commit-bot: I haz the power, 2013-06-07 17:10:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akuegel@chromium.org/15932008/32011

[commit-bot: I haz the power, 2013-06-07 21:24:32]

Change committed as 204934