[Mac] Redesign of the avatar menu button

[Mac] Redesign of the avatar menu button.
- Made the (old) avatar icon badge be a subclass of the (future, new in this CL)
avatar button, and moved the common code (opening the avatar bubble, listening to profile changes) up in the parent class.
- Made this class use a ProfileInfoCacheObserver rather than the NotificationService
- Added unit tests for the new code 
- Small change in c/b/ui/views/frame/browser_view.cc to re-use a new 
method in profiles_state.cc that checks whether a profile is a normal/guest 
mode profile (i.e. not incognito)

Screenshots: https://drive.google.com/folderview?id=0B1B1Up4p2NRMUW5zeFp6MkY5UDA&usp=sharing

This is behind the --new-profile-management flag, and shows the avatar
switcher as a menu button in the caption area. The button shows the user's
name, rather than an image.

BUG=324036
TEST=With the --new-profile-management flag on, start Chrome. Whether 
you have one user or more, you should see a button in the top right corner of 
the browser displaying the active profile's name. The guest browser should 
display "Guest", and the button should work otherwise as the existing avatar 
icon. In incognito mode, the spy dude icon should be displayed as before.

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

[noms (inactive), 2014-01-07 22:20:06]

Hiya Nico,

This CL changes the avatar icon into a button, behind the
--new-profile-management flag. The button isn't 100% visually like in the mocks,
as we're probably going to use a 9-patch for it, but for now the native NSButton
works pretty well. Please take a look & thanks!

[Nico, 2014-01-07 23:09:22]

Huh, this is a bit surprising: We used to have these buttons originally, and
then switched to the friendlier icons which everybody felt was a huge
improvement. Was this change discussed with ux?

[Nico, 2014-01-07 23:20:33]

How long do you expect the two avatar buttons to coexist? If only a short time,
does it make sense to just switch immediately? If a longer time, does it make
sense to extract a common superclass?

(When you copy code, it's useful for reviewers and future archeologists if you
say something like "Inspired by
chrome/browser/ui/cocoa/browser/avatar_button_controller.mm" in the CL
description.)

Does the button width animate nicely when it changes in width when its text
changes?

https://codereview.chromium.org/117533002/diff/250001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/new_avatar_button_controller.mm (right):

https://codereview.chromium.org/117533002/diff/250001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/new_avatar_button_controller.mm:53: class
ProfileInfoUpdateObserverBridge : public content::NotificationObserver {
In general, we try to not use notifications in new code. Since
NOTIFICATION_PROFILE_CACHED_INFO_CHANGED isn't new, it's ok though I think.

(It'd be nicer if the NotificationObserver was hidden somewhere in profile code
and was identical for all platforms, and then it'd call methods on a narrow
delegate interface which the per-platform uis then implemen)

https://codereview.chromium.org/117533002/diff/250001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/new_avatar_button_controller.mm:63:
content::NotificationService::AllSources());
Do you need to do this? I thought the point of NotificationRegistrar is that it
removes all notifications that it added on destruction automatically.

https://codereview.chromium.org/117533002/diff/250001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/new_avatar_button_controller.mm:110: [[self
view] addSubview:button_];
Since this is the only view, can button_ just be the [self view] itself? Do you
need the container?

https://codereview.chromium.org/117533002/diff/250001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/117533002/diff/250001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:275: // Whether or not to
show the new avatar button used by --new-profile-maagement.
typo maagement

Why is this behind a flag? Can it just become the new look? Why keep the old
thing around?

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

https://codereview.chromium.org/117533002/diff/250001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:564: return
profiles::IsRegularOrGuestSession(browser_.get());
is this change intentionally in this cl?

[noms (inactive), 2014-01-08 15:05:30]

Just a round of answers/attempts to clarify things before I actually touch any
code.

This has definitely been discussed with UI (it's not my initiative!). The
general motivation behind it is that a) multi-profiles are pretty hard to
discover at the moment and that needs to change and b) hamburger and lemonade
don't really inspire a sense of identity (certainly not as much as a button with
your name on it). 

Here is a set of mocks I'm working from:
https://drive.google.com/a/google.com/folderview?id=0B6x6iYCtKinEdENmTkZxZGVS...

When the text changes the button sort of grows or shrinks instantly. Is there a 
way to animate this or is this ok? It doesn't disappear or jump or anything like
that.

The old/new buttons will coexist for a while, at least throughout dogfooding.
For example, on Windows they've both beenin Canary for about 2 months, so I
expect Mac will go a similar way. I do like the idea of superclassing it, as
even in the future the spy dude will be shown for incognito mode, so really, the
"old" AvatarButtonController code will live forever. I'll get on that.

https://codereview.chromium.org/117533002/diff/250001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/new_avatar_button_controller.mm (right):

https://codereview.chromium.org/117533002/diff/250001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/new_avatar_button_controller.mm:53: class
ProfileInfoUpdateObserverBridge : public content::NotificationObserver {
I can also make this an AvatarMenuObserver (like the views code does), and get
notified that way. Would that be better?

 On 2014/01/07 23:20:33, Nico wrote:
> In general, we try to not use notifications in new code. Since
> NOTIFICATION_PROFILE_CACHED_INFO_CHANGED isn't new, it's ok though I think.
> 
> (It'd be nicer if the NotificationObserver was hidden somewhere in profile
code
> and was identical for all platforms, and then it'd call methods on a narrow
> delegate interface which the per-platform uis then implemen)

https://codereview.chromium.org/117533002/diff/250001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/new_avatar_button_controller.mm:110: [[self
view] addSubview:button_];
Not really. The only reason I added the container is to match the
AvatarButtonController implementation, so that the outside code used them in
identical ways. Do you think that's worth it?

On 2014/01/07 23:20:33, Nico wrote:
> Since this is the only view, can button_ just be the [self view] itself? Do
you
> need the container?

https://codereview.chromium.org/117533002/diff/250001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/117533002/diff/250001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:275: // Whether or not to
show the new avatar button used by --new-profile-maagement.
This isn't ready to become the new look immediately. It's part of the massive
new avatar menu/button redesign (the bubble has changed too), and I think they
all need to go in at once for this to make sense.

On 2014/01/07 23:20:33, Nico wrote:
> typo maagement
> 
> Why is this behind a flag? Can it just become the new look? Why keep the old
> thing around?

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

https://codereview.chromium.org/117533002/diff/250001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:564: return
profiles::IsRegularOrGuestSession(browser_.get());
Yes, but I can move it in a separate CL if you want. I had to make the same
checks in the Mac code, so I realized it would be easier if I made a utility
function in profile_state (and then use it correctly here, rather than duplicate
the code again). Should I move it out?

On 2014/01/07 23:20:33, Nico wrote:
> is this change intentionally in this cl?

[Nico, 2014-01-08 19:01:50]

Ok.

On Wed, Jan 8, 2014 at 7:05 AM, <noms@chromium.org> wrote:

> Just a round of answers/attempts to clarify things before I actually touch
> any
> code.
>
> This has definitely been discussed with UI (it's not my initiative!). The
> general motivation behind it is that a) multi-profiles are pretty hard to
> discover at the moment and that needs to change and b) hamburger and
> lemonade
> don't really inspire a sense of identity (certainly not as much as a
> button with
> your name on it).
>
> Here is a set of mocks I'm working from:
> https://drive.google.com/a/google.com/folderview?id=
> 0B6x6iYCtKinEdENmTkZxZGVSU2c&usp=sharing&tid=0B6x6iYCtKinEUmw1elM5OGpPNzA
>
> When the text changes the button sort of grows or shrinks instantly. Is
> there a
> way to animate this or is this ok? It doesn't disappear or jump or
> anything like
> that.
>

I suppose it's ok for this CL, but in general we try to not have sudden
changes in UI as far as I know


>
> The old/new buttons will coexist for a while, at least throughout
> dogfooding.
> For example, on Windows they've both beenin Canary for about 2 months, so I
> expect Mac will go a similar way. I do like the idea of superclassing it,
> as
> even in the future the spy dude will be shown for incognito mode, so
> really, the
> "old" AvatarButtonController code will live forever. I'll get on that.
>
>
>
>
> https://codereview.chromium.org/117533002/diff/250001/
> chrome/browser/ui/cocoa/browser/new_avatar_button_controller.mm
> File chrome/browser/ui/cocoa/browser/new_avatar_button_controller.mm
> (right):
>
> https://codereview.chromium.org/117533002/diff/250001/
> chrome/browser/ui/cocoa/browser/new_avatar_button_controller.mm#newcode53
> chrome/browser/ui/cocoa/browser/new_avatar_button_controller.mm:53:
> class ProfileInfoUpdateObserverBridge : public
> content::NotificationObserver {
> I can also make this an AvatarMenuObserver (like the views code does),
> and get notified that way. Would that be better?
>
>
>  On 2014/01/07 23:20:33, Nico wrote:
>
>> In general, we try to not use notifications in new code. Since
>> NOTIFICATION_PROFILE_CACHED_INFO_CHANGED isn't new, it's ok though I
>>
> think.
>
>  (It'd be nicer if the NotificationObserver was hidden somewhere in
>>
> profile code
>
>> and was identical for all platforms, and then it'd call methods on a
>>
> narrow
>
>> delegate interface which the per-platform uis then implemen)
>>
>
> https://codereview.chromium.org/117533002/diff/250001/
> chrome/browser/ui/cocoa/browser/new_avatar_button_controller.mm#newcode110
> chrome/browser/ui/cocoa/browser/new_avatar_button_controller.mm:110:
> [[self view] addSubview:button_];
> Not really. The only reason I added the container is to match the
> AvatarButtonController implementation, so that the outside code used
> them in identical ways. Do you think that's worth it?
>
>
> On 2014/01/07 23:20:33, Nico wrote:
>
>> Since this is the only view, can button_ just be the [self view]
>>
> itself? Do you
>
>> need the container?
>>
>
> https://codereview.chromium.org/117533002/diff/250001/
> chrome/browser/ui/cocoa/browser_window_controller.h
> File chrome/browser/ui/cocoa/browser_window_controller.h (right):
>
> https://codereview.chromium.org/117533002/diff/250001/
> chrome/browser/ui/cocoa/browser_window_controller.h#newcode275
> chrome/browser/ui/cocoa/browser_window_controller.h:275: // Whether or
> not to show the new avatar button used by --new-profile-maagement.
> This isn't ready to become the new look immediately. It's part of the
> massive new avatar menu/button redesign (the bubble has changed too),
> and I think they all need to go in at once for this to make sense.
>
>
> On 2014/01/07 23:20:33, Nico wrote:
>
>> typo maagement
>>
>
>  Why is this behind a flag? Can it just become the new look? Why keep
>>
> the old
>
>> thing around?
>>
>
> https://codereview.chromium.org/117533002/diff/250001/
> chrome/browser/ui/views/frame/browser_view.cc
> File chrome/browser/ui/views/frame/browser_view.cc (right):
>
> https://codereview.chromium.org/117533002/diff/250001/
> chrome/browser/ui/views/frame/browser_view.cc#newcode564
> chrome/browser/ui/views/frame/browser_view.cc:564: return
> profiles::IsRegularOrGuestSession(browser_.get());
> Yes, but I can move it in a separate CL if you want. I had to make the
> same checks in the Mac code, so I realized it would be easier if I made
> a utility function in profile_state (and then use it correctly here,
> rather than duplicate the code again). Should I move it out?


If you keep it here, please add something like "tweak that one weird thing
in the views code too" to the cl description.


>
>
> On 2014/01/07 23:20:33, Nico wrote:
>
>> is this change intentionally in this cl?
>>
>
> https://codereview.chromium.org/117533002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[noms (inactive), 2014-01-09 23:14:53]

Holy refactoring Batman! I've made the (old) icon badge a subclass of the (new)
just-the-profile-name button. Do you think I should've added a common parent
class instead? I'm open to suggestions.

[noms (inactive), 2014-01-13 21:13:38]

ping :)

[Nico, 2014-01-14 03:56:02]

Much better!

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:106: return (profile->IsGuestSession()
|| !profile->IsOffTheRecord());
nit: no parens

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/avatar_button_controller.h (right):

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/avatar_button_controller.h:41: // Initializer to
be used by child classes.
initWithBrowser: is marked as "designated initializer" (
https://developer.apple.com/library/ios/documentation/general/conceptual/devp...
"The designated initializer of a subclass must invoke the designated initializer
of its superclass by sending a message to super"), so this comment contradicts
that.

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/avatar_button_controller_unittest.mm
(right):

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/avatar_button_controller_unittest.mm:20: static
const char kDefaultProfileName[] = "default";
nit: const has implicit internal linkage, no need for "static"

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:333: badgeYOffset =
(tabStripHeight - [avatarButton frame].size.height) /2;
NSHeight([avatarButton frame])


…don't you need to call setFrameSize: in this branch?

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:374: +
kAvatarTabStripShrink;
Isn't this what the else-if 4 lines above does? Shouldn't this be an else to the
isKindOfClass: in there? (And shouldn't the isKindOfClass: there be spelled
![self shouldUseNewAvatarButton]?)

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:594: NSView*
avatarButtonView = [avatarButtonController_ view];
Nice change!

[noms (inactive), 2014-01-15 22:07:46]

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.cc (right):

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:106: return (profile->IsGuestSession()
|| !profile->IsOffTheRecord());
On 2014/01/14 03:56:02, Nico wrote:
> nit: no parens

Done.

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/avatar_button_controller.h (right):

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/avatar_button_controller.h:41: // Initializer to
be used by child classes.
On 2014/01/14 03:56:02, Nico wrote:
> initWithBrowser: is marked as "designated initializer" (
>
https://developer.apple.com/library/ios/documentation/general/conceptual/devp...
> "The designated initializer of a subclass must invoke the designated
initializer
> of its superclass by sending a message to super"), so this comment contradicts
> that.

Done.

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser/avatar_button_controller_unittest.mm
(right):

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser/avatar_button_controller_unittest.mm:20: static
const char kDefaultProfileName[] = "default";
On 2014/01/14 03:56:02, Nico wrote:
> nit: const has implicit internal linkage, no need for "static"

Done.

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:333: badgeYOffset =
(tabStripHeight - [avatarButton frame].size.height) /2;
I don't know how to test this (tabstrip needs to be smaller than the icon/button
height), but I trust it's here for a reason, so I've added it to the other
branch too.

On 2014/01/14 03:56:02, Nico wrote:
> NSHeight([avatarButton frame])
> 
> 
> …don't you need to call setFrameSize: in this branch?

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:374: +
kAvatarTabStripShrink;
Oops! Yes on all counts. On 2014/01/14 03:56:02, Nico wrote:
> Isn't this what the else-if 4 lines above does? Shouldn't this be an else to
the
> isKindOfClass: in there? (And shouldn't the isKindOfClass: there be spelled
> ![self shouldUseNewAvatarButton]?)

https://codereview.chromium.org/117533002/diff/570001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:594: NSView*
avatarButtonView = [avatarButtonController_ view];
To preemptively balance the oops above, obviously.
On 2014/01/14 03:56:02, Nico wrote:
> Nice change!

[noms (inactive), 2014-01-21 21:33:04]

pong

[Nico, 2014-01-23 00:51:33]

lgtm, some optional comments below (the last one is a tiny bit larger; maybe you
can do that in a follow-up if you want to do it.)

I think this CL is looking much nicer than the earlier revisions. Thanks!

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser/avatar_base_controller.mm (right):

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/avatar_base_controller.mm:95: -
(NSButton*)buttonView {
Maybe

  CHECK(button_get()); // Subclasses must set this.

?

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/avatar_base_controller.mm:139: DCHECK(sender ==
button_.get());
DCHECK_EQ if it compiles

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser/avatar_button_controller.mm (right):

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/avatar_button_controller.mm:45: [self
setView:container];
I'd [self setView:button_], as there's only one subview. `container` doesn't do
anything useful and takes up memory.

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser/avatar_button_controller_unittest.mm
(right):

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/avatar_button_controller_unittest.mm:28:
switches::kNewProfileManagement));
This DCHECK seems pointless, given that the switch is added 1 line further up.

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:340: 
The two ifs in this file seem slightly icky; I'd try to replace them with a
virtual method call (in c++ parlance) instead, given that you have a class
hierarchy already.

[noms (inactive), 2014-01-23 18:24:26]

Thanks Nico! Addressed all but the bigger comment -- I'll take a look at it in
the next CL.

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser/avatar_base_controller.mm (right):

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/avatar_base_controller.mm:95: -
(NSButton*)buttonView {
On 2014/01/23 00:51:33, Nico wrote:
> Maybe
> 
>   CHECK(button_get()); // Subclasses must set this.
> 
> ?

Done.

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/avatar_base_controller.mm:139: DCHECK(sender ==
button_.get());
On 2014/01/23 00:51:33, Nico wrote:
> DCHECK_EQ if it compiles

Done.

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser/avatar_button_controller.mm (right):

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/avatar_button_controller.mm:45: [self
setView:container];
Oops! Done. On 2014/01/23 00:51:33, Nico wrote:
> I'd [self setView:button_], as there's only one subview. `container` doesn't
do
> anything useful and takes up memory.

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser/avatar_button_controller_unittest.mm
(right):

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser/avatar_button_controller_unittest.mm:28:
switches::kNewProfileManagement));
Misplaced paranoia. I've removed it.
On 2014/01/23 00:51:33, Nico wrote:
> This DCHECK seems pointless, given that the switch is added 1 line further up.

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/117533002/diff/1120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:340: 
I have another small CL to fix some avatar button related UI, so I will fix them
then.

On 2014/01/23 00:51:33, Nico wrote:
> The two ifs in this file seem slightly icky; I'd try to replace them with a
> virtual method call (in c++ parlance) instead, given that you have a class
> hierarchy already.

[commit-bot: I haz the power, 2014-01-23 18:26:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noms@chromium.org/117533002/1280003

[commit-bot: I haz the power, 2014-01-23 22:12:15]

Change committed as 246700