New avatar menu: For supervised users, show "(Supervised)" in the avatar button

New avatar menu: For supervised users, show "(Supervised)" in the avatar button.

Mocks at https://folio.googleplex.com/chrome-ux/mocks/064-chrome-away-from-chrome/supervised/mirror_v2

BUG=310640, 372812
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273640

[Marc Treib, 2014-05-21 16:19:55]

PTAL!

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

https://codereview.chromium.org/287103004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:78: : MenuButton(listener,
GetButtonText(browser->profile()), NULL, true),
@noms: Is there any reason why this didn't use profiles::GetAvatarNameForProfile
before?

[noms (inactive), 2014-05-21 17:00:57]

lgtm

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

https://codereview.chromium.org/287103004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:78: : MenuButton(listener,
GetButtonText(browser->profile()), NULL, true),
None that I can see. I think GetAvatarNameForProfile came after, and I didn't
update this in all the places. My bad! :(

On 2014/05/21 16:19:55, treib wrote:
> @noms: Is there any reason why this didn't use
profiles::GetAvatarNameForProfile
> before?

[msw, 2014-05-21 17:20:25]

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

https://codereview.chromium.org/287103004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:64: name = label +
base::char16(' ') + name;
Appending the strings like this is incorrect, even though you've attempted to
handle RTL here. Instead, the string resource should be defined with a
placeholder for the profile name like this:
  <ph name="NAME">$1<ex>John Doe</ex></ph> (Supervised)
Then, you can use l10n_util::GetStringFUTF16(IDS_MANAGED_USER_NEW_AVATAR_LABEL,
name) to assemble the string properly.

[Marc Treib, 2014-05-26 09:17:52]

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

https://codereview.chromium.org/287103004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:64: name = label +
base::char16(' ') + name;
On 2014/05/21 17:20:25, msw wrote:
> Appending the strings like this is incorrect, even though you've attempted to
> handle RTL here. Instead, the string resource should be defined with a
> placeholder for the profile name like this:
>   <ph name="NAME">$1<ex>John Doe</ex></ph> (Supervised)
> Then, you can use
l10n_util::GetStringFUTF16(IDS_MANAGED_USER_NEW_AVATAR_LABEL,
> name) to assemble the string properly.

Ah yes, I was kinda expecting a comment here ;-)
So thanks for the hint! Done.

[Marc Treib, 2014-05-26 14:31:00]

Update: I've added the Mac implementation to the same CL, since it depends on
the same new resource string.

@Mike: PTAL at that as well. Thanks!

@Monica: To get the correct elision behavior (i.e. elide the profile name, but
not the "supervised" label), I had to change the way elision is done on Mac.
It's now done manually, similar to the Windows version, as opposed to just
clamping the width. As a result, the width of the button might change by a few
pixels.
Please check if you can see any problems with this. Thanks!

[msw, 2014-05-27 17:16:41]

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

https://codereview.chromium.org/287103004/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:184: int
maxTextWidth = kMaxButtonContentWidth - [[button_ image] size].width;
nit: it seems like you're decreasing the width twice (once by subtracting the
image width, and another time by reducing the const's value by 20px), is that
intended and reasonable, especially in the non-managed case?

https://codereview.chromium.org/287103004/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:185: return
gfx::ElideText(avatarName,
nit: will this fade/truncate and [not] add ellipsis like the old method?
(maybe post before/after screenshots of buttons with long names to the bug?)

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

https://codereview.chromium.org/287103004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:59: base::string16
GetButtonText(Profile* profile) {
nit: could this and GetElidedText be shared cross-platform functions?

[Marc Treib, 2014-05-28 11:04:04]

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

https://codereview.chromium.org/287103004/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:184: int
maxTextWidth = kMaxButtonContentWidth - [[button_ image] size].width;
On 2014/05/27 17:16:41, msw wrote:
> nit: it seems like you're decreasing the width twice (once by subtracting the
> image width, and another time by reducing the const's value by 20px), is that
> intended and reasonable, especially in the non-managed case?

Yes: The old 120 pixels were the total width of the button, the new 100 are the
internal width, i.e. without the borders.

https://codereview.chromium.org/287103004/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/profiles/avatar_button_controller.mm:185: return
gfx::ElideText(avatarName,
On 2014/05/27 17:16:41, msw wrote:
> nit: will this fade/truncate and [not] add ellipsis like the old method?
> (maybe post before/after screenshots of buttons with long names to the bug?)

This adds an ellipsis. The button looks identical to the old one, except for a
few pixels difference in width. Before/after screenshots:
https://drive.google.com/a/google.com/folderview?id=0BxTU2YPRG-lRa1llWWZKVUh6...

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

https://codereview.chromium.org/287103004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:59: base::string16
GetButtonText(Profile* profile) {
On 2014/05/27 17:16:41, msw wrote:
> nit: could this and GetElidedText be shared cross-platform functions? 

At the moment, the GetElidedText implementations behave slightly differently
between platforms (Win/Linux limits (roughly) by number of characters, Mac
limits by number of pixels), and I'm not sure if there's a particular reason for
that. 
noms@: Can you clarify the reasoning here?

Anyway, since this is pretty urgent (must get into M37, which means land by
Monday), can I commit it as is for now, and then do a cleanup CL later if
appropriate?

[noms (inactive), 2014-05-28 13:49:35]

I think the GetElidedText implementations are different because the Mac/Win code
didn't go into the same CL, and different reviewers suggested different things.
I don't have any objections to merging them!

[noms (inactive), 2014-05-28 14:32:31]

I can shave the yak of consolidating the two functions in profiles_state.cc.
There's already GetAvatarNameForProfile, so a new
GetElidedProfileNameForAvatarButton (or the like) would fit right in.

On 2014/05/28 13:49:35, Monica Dinculescu wrote:
> I think the GetElidedText implementations are different because the Mac/Win
code
> didn't go into the same CL, and different reviewers suggested different
things.
> I don't have any objections to merging them!

[msw, 2014-05-28 20:13:23]

lgtm

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

https://codereview.chromium.org/287103004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/profiles/new_avatar_button.cc:59: base::string16
GetButtonText(Profile* profile) {
On 2014/05/28 11:04:05, treib wrote:
> On 2014/05/27 17:16:41, msw wrote:
> > nit: could this and GetElidedText be shared cross-platform functions? 
> 
> At the moment, the GetElidedText implementations behave slightly differently
> between platforms (Win/Linux limits (roughly) by number of characters, Mac
> limits by number of pixels), and I'm not sure if there's a particular reason
for
> that. 
> noms@: Can you clarify the reasoning here?
> 
> Anyway, since this is pretty urgent (must get into M37, which means land by
> Monday), can I commit it as is for now, and then do a cleanup CL later if
> appropriate?

You can land as-is and cleanup later if needed.
Code health is important, please do remember to cleanup.

[Marc Treib, 2014-05-29 17:18:25]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2014-05-29 17:20:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/treib@chromium.org/287103004/60001

[commit-bot: I haz the power, 2014-05-29 18:53:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-29 22:55:43]

Change committed as 273640