The User Manager needs to use profile paths not display names for profile switching

The User Manager needs to use profile paths not display names for profile switching

Since profiles can have duplicate names now, the profile path is the only thing that
distinguishes profiles.

Also did some cleanup of const refs.

BUG=448347
TEST=Start Chrome with --enable-new-avatar-menu. Create two profiles with the same
name. You should have no problems switching between the two in the "Switch Person"
overlay.

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

[noms (inactive), 2015-01-13 17:53:58]

noms@chromium.org changed reviewers:
+ mlerman@chromium.org, xiyuan@chromium.org

[noms (inactive), 2015-01-13 17:53:59]

Hiya,

Duplicate profile names have never worked correctly in the User Manager, and
this CL fixes this. Please take a look!

mike: all of the things!
xiyuan: chrome/browser/resources/user_manager/user_manager.js


Thanks!

[xiyuan, 2015-01-13 18:05:25]

Would user emails be unique? If so, why not using that instead of profile path?

[Mike Lerman, 2015-01-13 19:55:42]

Xiyuan, you can't the same email address signed in multiple times on the same
machine, so usernames will be unique.

Monica, please update the launchUser called in UserManagerUIBrowserTest.

Other comments below.

https://codereview.chromium.org/840673004/diff/1/chrome/browser/ui/webui/sign...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/840673004/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:115: const
base::string16& email) {
Optional simpliciation: Change the email parameter to type std::string and call
base::UTF8ToUTF16() in this method rather than in multiple locations below.

https://codereview.chromium.org/840673004/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:319: 
nit: Remove empty line.

https://codereview.chromium.org/840673004/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:410: const
base::Value* profile_path_value;
nit: Initialize profile_path_value to NULL (or nullptr).

https://codereview.chromium.org/840673004/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:666:
profile_value->SetString(kKeyProfilePath, profile_path.MaybeAsASCII());
FilePath::MaybeAsASCII() returns std::string() whenever the file path contains
non-ascii characters. If the OS name, for example, contains Unicode characters,
then the Profile Path (usually within C:\Users\<username>) will contain unicode
characters, and this won't work.

You can perhaps use FilePath::BaseName to get only the last part of the file
path (being the profile directory), which should be unique, and then using
LossyDisplayName() instead of MaybeAsASCII()?

[xiyuan, 2015-01-13 20:19:46]

On 2015/01/13 19:55:42, Mike Lerman wrote:
> Xiyuan, you can't the same email address signed in multiple times on the same
> machine, so usernames will be unique.
> 

Right. That is why I propose to use email. Passing profile path feels like
exposing unnecessary details on the JS side.

[noms (inactive), 2015-01-13 20:20:55]

On 2015/01/13 20:19:46, xiyuan wrote:
> On 2015/01/13 19:55:42, Mike Lerman wrote:
> > Xiyuan, you can't the same email address signed in multiple times on the
same
> > machine, so usernames will be unique.
> > 
> 
> Right. That is why I propose to use email. Passing profile path feels like
> exposing unnecessary details on the JS side.

As i responded above, that only works on ChromeOS :) 
On Desktop, local profiles do not have an email. They just have a displayName,
which can be a duplicate (and it is, hence the current bug). The only thing that
differentiates local profiles is the profile path.

[xiyuan, 2015-01-13 20:23:31]

On 2015/01/13 20:20:55, Monica Dinculescu wrote:
> On 2015/01/13 20:19:46, xiyuan wrote:
> > On 2015/01/13 19:55:42, Mike Lerman wrote:
> > > Xiyuan, you can't the same email address signed in multiple times on the
> same
> > > machine, so usernames will be unique.
> > > 
> > 
> > Right. That is why I propose to use email. Passing profile path feels like
> > exposing unnecessary details on the JS side.
> 
> As i responded above, that only works on ChromeOS :) 
> On Desktop, local profiles do not have an email. They just have a displayName,
> which can be a duplicate (and it is, hence the current bug). The only thing
that
> differentiates local profiles is the profile path.

I see. Thanks for the clarification. Totally forgot the unauthenticated case. :p

LGTM then.

[noms (inactive), 2015-01-13 21:36:42]

https://codereview.chromium.org/840673004/diff/1/chrome/browser/ui/webui/sign...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/840673004/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:115: const
base::string16& email) {
On 2015/01/13 19:55:41, Mike Lerman wrote:
> Optional simpliciation: Change the email parameter to type std::string and
call
> base::UTF8ToUTF16() in this method rather than in multiple locations below.

Done.

https://codereview.chromium.org/840673004/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:319: 
On 2015/01/13 19:55:41, Mike Lerman wrote:
> nit: Remove empty line.

Done.

https://codereview.chromium.org/840673004/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:410: const
base::Value* profile_path_value;
On 2015/01/13 19:55:41, Mike Lerman wrote:
> nit: Initialize profile_path_value to NULL (or nullptr).

Done.

https://codereview.chromium.org/840673004/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:666:
profile_value->SetString(kKeyProfilePath, profile_path.MaybeAsASCII());
On 2015/01/13 19:55:41, Mike Lerman wrote:
> FilePath::MaybeAsASCII() returns std::string() whenever the file path contains
> non-ascii characters. If the OS name, for example, contains Unicode
characters,
> then the Profile Path (usually within C:\Users\<username>) will contain
unicode
> characters, and this won't work.
> 
> You can perhaps use FilePath::BaseName to get only the last part of the file
> path (being the profile directory), which should be unique, and then using
> LossyDisplayName() instead of MaybeAsASCII()?

I used base::CreateFilePathValue(), which is what chrome://settings uses. It's
also paired with a base::GetValueAsFilePath() so everything should be ok.

[Mike Lerman, 2015-01-14 01:53:17]

https://codereview.chromium.org/840673004/diff/1/chrome/browser/ui/webui/sign...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/840673004/diff/1/chrome/browser/ui/webui/sign...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:666:
profile_value->SetString(kKeyProfilePath, profile_path.MaybeAsASCII());
On 2015/01/13 21:36:42, Monica Dinculescu wrote:
> On 2015/01/13 19:55:41, Mike Lerman wrote:
> > FilePath::MaybeAsASCII() returns std::string() whenever the file path
contains
> > non-ascii characters. If the OS name, for example, contains Unicode
> characters,
> > then the Profile Path (usually within C:\Users\<username>) will contain
> unicode
> > characters, and this won't work.
> > 
> > You can perhaps use FilePath::BaseName to get only the last part of the file
> > path (being the profile directory), which should be unique, and then using
> > LossyDisplayName() instead of MaybeAsASCII()?
> 
> I used base::CreateFilePathValue(), which is what chrome://settings uses. It's
> also paired with a base::GetValueAsFilePath() so everything should be ok. 

Chrome://settings doesn't seem to do anything with the value it gets from
CreateFilePathValue(). I do think this solution should work, but it should be
tested at least manually.

https://codereview.chromium.org/840673004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc (right):

https://codereview.chromium.org/840673004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc:78: base::string16
profile_name =
You can remove this variable - it's not being used anymore.

[noms (inactive), 2015-01-14 15:17:55]

https://codereview.chromium.org/840673004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc (right):

https://codereview.chromium.org/840673004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/signin/user_manager_ui_browsertest.cc:78: base::string16
profile_name =
On 2015/01/14 01:53:17, Mike Lerman wrote:
> You can remove this variable - it's not being used anymore.

Done.

[Mike Lerman, 2015-01-14 15:25:34]

lgtm

Please do verify it works with Unicode characters, though, please :)

[noms (inactive), 2015-01-14 15:30:02]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2015-01-14 15:30:18]

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

[commit-bot: I haz the power, 2015-01-14 17:20:43]

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

[commit-bot: I haz the power, 2015-01-14 17:20:44]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[noms (inactive), 2015-01-14 20:36:56]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2015-01-14 20:38:32]

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

[commit-bot: I haz the power, 2015-01-14 23:25:55]

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

[commit-bot: I haz the power, 2015-01-14 23:25:56]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[noms (inactive), 2015-01-15 23:17:43]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2015-01-15 23:19:26]

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

[commit-bot: I haz the power, 2015-01-15 23:20:47]

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

[commit-bot: I haz the power, 2015-01-15 23:20:48]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[noms (inactive), 2015-01-19 19:19:17]

The CQ bit was checked by noms@chromium.org

[commit-bot: I haz the power, 2015-01-19 19:20:01]

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

[commit-bot: I haz the power, 2015-01-19 20:26:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-19 20:27:28]

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