Stop using the old-style profile names when using --new-profile-management

Stop using the old-style profile names when using --new-profile-management

Instead use:
- "You" for a single, local profile
- The custom name chosen by the user, if the user renames the profile
- The user's GAIA first name for signed in profiles
- Person %n in all other cases.

This will be used consistently in both by the avatar menu button, and the avatar bubble.

Bonus changes to make this work:
- changed the name and signature of profiles::GetActiveProfileDisplayName to
GetAvatarNameForProfile, to make sure it's not accidentally used
outside of the avatar button related stuff, with baffling results ("Why is the profile name 'You'?)
- the Win avatar button now listens to ProfileInfoCache changes (just like the Mac one does), so that if a new profile is added, the avatar button changes from displaying 'You' to the actual profile name.

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

[noms (inactive), 2014-02-20 19:32:41]

Hi all,

As part of the profile management redesign, we're changing a little what the
avatar button displays (see CL description). Please take a look. Thanks!

rlp: c/b/profiles/*

sky: c/app/generated_resources.grd and c/b/ui/views/* (an api change + changed
the avatar button model listener)

groby: c/b/ui/cocoa/* (just an api change)

[groby-ooo-7-16, 2014-02-20 20:17:27]

Cocoa LGTM, 2 nits

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_unittest.cc:137: ProfileInfoCache& cache
= profile_manager->GetProfileInfoCache();
Indent seems off

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.h (right):

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.h:32: base::string16
GetAvatarButtonDisplayNameForProfile(Profile* profile);
GetProfileAvatarName?

I'm bikeshedding probably, but I really don't like having UI element types in
the function name. And based on the old naming - Get ActiveProfile DisplayName -
it seems Get Profile AvatarName. I.e. verb, object, noun. 

Feel free to ignore :)

[rpetterson, 2014-02-20 21:41:32]

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_unittest.cc:1036: // We should display
custom names for local profiles.
How does the profile info cache know that it was a custom name vs. a given
default name?

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_unittest.cc:1080:
cache.SetUserNameOfProfileAtIndex(0, ASCIIToUTF16("user@gmail.com"));
Does username take precedence over name?

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

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:63: if (index == std::string::npos)
shouldn't this be at least 0?

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.h (right):

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.h:32: base::string16
GetAvatarButtonDisplayNameForProfile(Profile* profile);
On 2014/02/20 20:17:27, groby wrote:
> GetProfileAvatarName?
> 
> I'm bikeshedding probably, but I really don't like having UI element types in
> the function name. And based on the old naming - Get ActiveProfile DisplayName
-
> it seems Get Profile AvatarName. I.e. verb, object, noun. 
> 
> Feel free to ignore :)

I agree with groby on this one. I think GetAvatarNameForProfile would be better.

[sky, 2014-02-20 22:14:07]

LGTM

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/new_avatar_button.h (right):

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/new_avatar_button.h:38: 
nit: no newline between overrides for same class (see style guide).

[noms (inactive), 2014-02-24 22:35:05]

I've addressed the comments, and added in two tiny changes. 
Unfortunately, I also accidentally rebased, it seems. To make up for this, here
is a panda: http://giphy.com/gifs/rDMGBIqjAJIys. I'm very sorry!

The changes are, and whom it affects:

sky, groby: After talking to Travis, we decided we should be consistent in the
avatar bubble and display the same thing as the avatar button. So if the button
displays "You", the avatar bubble should as well. 

rlp: The logic for deciding whether this is a "default profile name" is better,
because it checks whether the profile is of the form "Person %d", not just
"contains the word Person. However, it also has a sscanf now, so that makes me
sad.

Please take a look!

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_unittest.cc:137: ProfileInfoCache& cache
= profile_manager->GetProfileInfoCache();
On 2014/02/20 20:17:27, groby wrote:
> Indent seems off

Done.

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_unittest.cc:1036: // We should display
custom names for local profiles.
The ProfileInfoCache doesn't. The profiles::GetAvatarButtonDisplayNameForProfile
does a check whether the name is of the form "Person %d", which is what default
names look like. That function is the only one that cares whether the name is
the default one the profile was created with. I thought about adding a
preference to the ProfileInfoCache -- the problem is that it wouldn't be set for
all existing profiles, so we might display "you" for a lot of single, local
profiles where the user has changed the name.
 
On 2014/02/20 21:41:32, rpetterson wrote:
> How does the profile info cache know that it was a custom name vs. a given
> default name?

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager_unittest.cc:1080:
cache.SetUserNameOfProfileAtIndex(0, ASCIIToUTF16("user@gmail.com"));
Username just means that the user is signed in, and therefore we are probably
using the user's gaia name, if it exists (in cache.GetNameOfProfile). It's
essentially the same as having a "custom" name, and they're both treated in the
same way (whether the name is custom, or whether the profile is signed in, we do
the same thing, which is display the actual profile name)

On 2014/02/20 21:41:32, rpetterson wrote:
> Does username take precedence over name?

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

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.cc:63: if (index == std::string::npos)
Not always. GetIndexOfProfile returns std::string::npos if it can't find a
profile with that path in the ProfileInfoCache. :(

On 2014/02/20 21:41:32, rpetterson wrote:
> shouldn't this be at least 0?

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
File chrome/browser/profiles/profiles_state.h (right):

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/profiles...
chrome/browser/profiles/profiles_state.h:32: base::string16
GetAvatarButtonDisplayNameForProfile(Profile* profile);
Hmm, doesn't avatar name imply "the name of the avatar we are displaying"? Let's
go with it for now though, and see if it causes confusion in the future.

On 2014/02/20 21:41:32, rpetterson wrote:
> On 2014/02/20 20:17:27, groby wrote:
> > GetProfileAvatarName?
> > 
> > I'm bikeshedding probably, but I really don't like having UI element types
in
> > the function name. And based on the old naming - Get ActiveProfile
DisplayName
> -
> > it seems Get Profile AvatarName. I.e. verb, object, noun. 
> > 
> > Feel free to ignore :)
> 
> I agree with groby on this one. I think GetAvatarNameForProfile would be
better.

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/new_avatar_button.h (right):

https://codereview.chromium.org/171523004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/new_avatar_button.h:38: 
On 2014/02/20 22:14:08, sky wrote:
> nit: no newline between overrides for same class (see style guide).

Done.

[rpetterson, 2014-02-24 23:50:18]

lgtm

[rpetterson, 2014-02-24 23:50:43]

Oh, one additional thought (but doesn't change my lgtm).

https://codereview.chromium.org/171523004/diff/260001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/171523004/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1118: // the new state and away from
the previous "forced" state.
Perhaps open a bug for this and reference the bug here? Just for a little extra
insurance that it might get done.

[sky, 2014-02-25 00:36:45]

SLGTM

[noms (inactive), 2014-02-25 01:08:26]

Thanks everyone! :)

https://codereview.chromium.org/171523004/diff/260001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/171523004/diff/260001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1118: // the new state and away from
the previous "forced" state.
Sure. I'll add the TODO tomorrow in a separate CL, since this is code already in
master.

On 2014/02/24 23:50:47, rpetterson wrote:
> Perhaps open a bug for this and reference the bug here? Just for a little
extra
> insurance that it might get done.

[noms (inactive), 2014-02-25 01:08:52]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-02-25 01:56:25]

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

[commit-bot: I haz the power, 2014-02-25 02:52:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-25 02:52:53]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, google_apis_unittests,
gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[noms (inactive), 2014-02-25 20:47:30]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-02-25 20:50:28]

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

[commit-bot: I haz the power, 2014-02-25 21:59:16]

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

[commit-bot: I haz the power, 2014-02-25 22:25:54]

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

[commit-bot: I haz the power, 2014-02-25 23:33:38]

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

[commit-bot: I haz the power, 2014-02-26 00:17:38]

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

[commit-bot: I haz the power, 2014-02-26 05:21:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-26 05:21:11]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[noms (inactive), 2014-02-26 14:47:40]

The CQ bit was checked by noms@chromium.org

[noms (inactive), 2014-02-26 14:47:50]

The CQ bit was unchecked by noms@chromium.org

[commit-bot: I haz the power, 2014-02-26 14:48:10]

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

[noms (inactive), 2014-02-26 16:57:37]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2014-02-26 16:59:17]

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

[commit-bot: I haz the power, 2014-02-26 20:39:30]

Change committed as 253588