First try at a user management screen for the desktop

First try at a user management screen for the desktop. 

This should look and have similar functionality as the CrOS account picker 
screen. The implementation cannot be fully merged, however, as some actions do 
not make sense in a desktop sense (for example shut down), and the server 
code is slightly different. 
More details can be found in this design document:
https://docs.google.com/a/google.com/document/d/1fg47-s19akfqaKNqjggS6Igj_pkXExhgTgdVPl2BZ7Y/edit?usp=sharing

The desktop user chooser can be accessible by enabling the 
--new-profile-management flag and going to chrome://user-chooser. It is not 
available on CrOs/iOS/Android. In a future CL, it will be linked to the avatar 
menu, and will come up when users sign out. 

This code is *not* meant to replace the CrOS account picker, and should not 
be used in CrOS. 

Implementation quirks: 
1) Guest-mode is currently not ready for the desktop, so "Browse as Guest" 
brings up an Incognito browser. 
2) If a profile is signed-out/locked, the corresponding pod shows a 
password field, but this password is not actually validated, and the profile 
browser is launched. This can't be fixed at the moment, but will be 
fixed in a future CL. 
3) There are no high-resolution avatar resources. Until this problem is solved, 
we are displaying a standard "chrome user" avatar.

This will be fixed in a future CL with either higher resolution resources
or the CrOS profile picture chooser dialog.

BUG=257033
TEST=Enable --new-profile-management. Go to chrome://user-chooser. 
This should display a row of user pods, for each existing profile.
Clicking on the pods should either give you a password field, or a
"Sign in" button, both of which should launch a new browser window 
for the selected profile (no password authentication is done at the moment). 

Screenshot: 
https://docs.google.com/a/chromium.org/file/d/0B1B1Up4p2NRMMHRMaHlITGxRdUk/edit?usp=sharing
RTL screenshot:
https://docs.google.com/a/chromium.org/file/d/0B1B1Up4p2NRMMVFrZUdnQjBISkU/edit?usp=sharing

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

[Roger Tawa OOO till Jul 10th, 2013-06-05 18:53:56]

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/login/user_pod_row.js (right):

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/user_pod_row.js:307: }
Would it be possible to derive something like DesktopUserPod from UserPod so
there is no boolean?

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/user_pod_row.js:589: 
Should probably move this file to a directory that is not chromeos specific.

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc (right):

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc:76:
chrome::ShowSingletonTab(chrome::FindBrowserWithWebContents(
Add a TODO here for what the real behaviour should be.

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc:82:
chrome::NewIncognitoWindow(chrome::FindBrowserWithWebContents(
Add a TODO here for what the real behaviour should be.

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc:102:
avatar_menu_model_->SwitchToProfile(i, true);
This seems to be the only use of the menu model.  Seems a little overkill. 
Maybe factor out the core of SwitchToProfile() into a standalone function that
can be called from here and from AvatarMenuModel::SwitchToProfile().

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc:220: };*/
I guess this commented out code is a reference to some javascript code.  Maybe
put the path of the file where the constants are defined?

Why 2 if |needs_signin| and 0 otherwise?

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc:222:
profile_value->SetBoolean(kKeySignedIn, is_active_user);
Already set at line 216.

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/signin/user_chooser_ui.cc (right):

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_ui.cc:38: // Set up the
chrome://theme/ source
Inside #if defined(ENABLE_THEMES) ?

[Nikita (slow), 2013-06-06 15:05:27]

Please ping when this CL is ready for review.

[noms, 2013-06-06 19:01:30]

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/login/user_pod_row.js (right):

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/user_pod_row.js:307: }
On 2013/06/05 18:53:56, Roger Tawa wrote:
> Would it be possible to derive something like DesktopUserPod from UserPod so
> there is no boolean?

Done.

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/user_pod_row.js:589: 
This file is actually part of pretty large codebase (it's used by the login
page, which is included in the oobe flow for CrOS), so we can't really move just
it out, and I don't think we should move all of the flow out of the directory...

On 2013/06/05 18:53:56, Roger Tawa wrote:
> Should probably move this file to a directory that is not chromeos specific.

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc (right):

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc:76:
chrome::ShowSingletonTab(chrome::FindBrowserWithWebContents(
On 2013/06/05 18:53:56, Roger Tawa wrote:
> Add a TODO here for what the real behaviour should be.

Done.

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc:82:
chrome::NewIncognitoWindow(chrome::FindBrowserWithWebContents(
On 2013/06/05 18:53:56, Roger Tawa wrote:
> Add a TODO here for what the real behaviour should be.

Done.

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc:102:
avatar_menu_model_->SwitchToProfile(i, true);
On 2013/06/05 18:53:56, Roger Tawa wrote:
> This seems to be the only use of the menu model.  Seems a little overkill. 
> Maybe factor out the core of SwitchToProfile() into a standalone function that
> can be called from here and from AvatarMenuModel::SwitchToProfile().

Done.

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc:220: };*/
On 2013/06/05 18:53:56, Roger Tawa wrote:
> I guess this commented out code is a reference to some javascript code.  Maybe
> put the path of the file where the constants are defined?
> 
> Why 2 if |needs_signin| and 0 otherwise?

Done.

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc:222:
profile_value->SetBoolean(kKeySignedIn, is_active_user);
On 2013/06/05 18:53:56, Roger Tawa wrote:
> Already set at line 216.

Done.

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/signin/user_chooser_ui.cc (right):

https://codereview.chromium.org/16104008/diff/21001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_ui.cc:38: // Set up the
chrome://theme/ source
On 2013/06/05 18:53:56, Roger Tawa wrote:
> Inside #if defined(ENABLE_THEMES) ?

Done.

[noms (inactive), 2013-06-10 13:47:44]

Hi Nikita,

I think this is ready for review. Would you mind taking a look at it, please?
Thanks!

[Nikita (slow), 2013-06-11 18:22:15]

first round of comments

https://codereview.chromium.org/16104008/diff/33001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:14961: +      <message
name="IDS_LOGIN_PUBLIC_ACCOUNT_ENTER_ACCESSIBLE_NAME" desc="Text to be spoken
when focus is set to the sign-in button in the public account user pod.">
You won't need public account related strings on desktop.

But before these strings could be removed:
* handler has to be fixed to not rely on them (ifdef)
* make sure that no JS code uses them from loadTimeDate
* HTML templates have to be per OS i.e. <div
id="public-account-user-pod-extras-template" hidden> block is CrOS specific

Please add these as TODO somewhere in a separate cleanup bug.

https://codereview.chromium.org/16104008/diff/33001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:14976: +      <message
name="IDS_LOGIN_POD_OWNER_USER" desc="Login screen user pod menu title for a
user who owns the device.">
Same here - should be a TODO in cleanup.

https://codereview.chromium.org/16104008/diff/33001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:14982: +      <message
name="IDS_CREATE_LOCALLY_MANAGED_USER_FEATURE_NAME" desc="Title close to icon
shown on side of Google sign-in UI to create a locally managed user.">
Same TODO for locally managed users related strings.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:169: chrome::HostDesktopType
desktop_type,
nit: parameters need to be aligned

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:652: 
nit: drop empty line

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:181: static void SwitchToProfile(
Please add method comment.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/login/user_pod_row.js (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/user_pod_row.js:788: var isLockedUser =
this.user.needsSignin;
Will we have concept of locked user on Chrome desktop?

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/user_pod_row.js:822: this.disabled =
true;
Why user pod is disabled?

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/user_pod_row.js:832: 
nit: drop empty line

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/user_pod_row.js:942: if
(user.isDesktopScreen)
nit: isDesktopScreen is property of a user but is named as login UI mode.

Rename it to something like isDesktopUser?

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/header_bar.html (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/header_bar.html:1: <div
id="login-header-bar" class="login-header-bar">
nit: Please rename this file to control_bar.html. You could leave ID's and CSS
rules as is.

Initially this was a header on CrOS login screen but than it was moved below so
become a footer of just control bar.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/header_bar.html:16: <div
id="shutdown-header-bar-item" class="header-bar-item" hidden>
nit: You'll probably won't need shutdown / sign out buttons.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/header_bar.js (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/header_bar.js:39:
$('sign-out-user-button').addEventListener('click',
You probably don't need sign out?

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/header_bar.js:41:
$('add-user-button').hidden = false;
Call updateUI_() instead?

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/user_chooser.html (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:2: <html
i18n-values="dir:textdirection;screen:screenType;lockanimations:lockAnimationsType;build:buildType">
lockanimations:lockAnimationsType - please remove

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:5: <link
rel="stylesheet" href="../chromeos/login/oobe.css">
In this case <link> / <script> elements should not be aligned with spaces.
Should be right below title element.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:39: <div
id="header-sections"></div>
nit: Please fix alignment - 2 spaces for elements, 4 spaces for attributes.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:42: <include
src="../chromeos/login/screen_gaia_signin.html">
You probably don't need screen_gaia_signin.html.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:44: <div
id="progress-dots" class="down"></div>
nit: Remove progress-dots

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:172: 
nit: drop empty line

[Nikita (slow), 2013-06-11 18:43:28]

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/user_chooser.js (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.js:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
nit: 2013

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.js:4: <include
src="user_chooser_common.js"></include>
Please move this include to html.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.js:16: function
setupSelect(select, list, callback) {
Where is setupSelect used? That looks like dead code from Chrome OS out-of-box
code.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.js:44:
login.GaiaSigninScreen.register();
Are you planning on using the same GaiaSigninScreen to authenticate users?

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/user_chooser_common.js (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser_common.js:13: <include
src="../chromeos/login/apps_menu.js"></include>
These are specific to CrOS OOBE screens and should be removed:

user_images_grid.js
apps_menu.js
screen_error_message.js
oobe_screen_user_image.js
screen_gaia_signin.js
screen_locally_managed_user_creation.js
screen_password_changed.js
screen_tpm_error.js
screen_wrong_hwid.js

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser_common.js:22:
src="../chromeos/login/screen_locally_managed_user_creation.js"></include>
nit: move </include> on next line instead?

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser_common.js:27: 
nit: drop empty line

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc:101:
info_cache.GetNameOfProfileAtIndex(i) == displayName) {
Why you have to check both email and displayName?
Email seems to be enough to identify user. Or is it for cases when email is
empty and only displayName could be used to identify such user?

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/signin/user_chooser_screen_handler.h (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.h:8: #include
"base/values.h"
nit: remove this header and forward declare base::ListValue

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.h:9: #include
"chrome/browser/profiles/avatar_menu_model.h"
nit: not used?

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/signin/user_chooser_ui.cc (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_ui.cc:7: 
drop: empty line

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_ui.cc:9: 
nit: drop empty line

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_ui.cc:29: 
nit: drop empty line

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_ui.cc:68: 
nit: drop empty line

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/signin/user_chooser_ui.h (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_ui.h:8: #include "base/values.h"
nit: drop and forward declare instead

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_ui.h:17: // A WebUI dialog to
display available users
nit: Add dot at the end.

[noms (inactive), 2013-06-11 19:39:41]

I've answered some of the comments, while I'm fixing all the rest.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/login/user_pod_row.js (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/user_pod_row.js:788: var isLockedUser =
this.user.needsSignin;
Yes. Signed in users will be able to "sign out" and lock their profile.

On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> Will we have concept of locked user on Chrome desktop?

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/header_bar.html (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/header_bar.html:16: <div
id="shutdown-header-bar-item" class="header-bar-item" hidden>
I only included them because there are some parts of the common code with Chrome
OS that assume they exist. I will track them down and maybe add some ifs to
check for existence.
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> nit: You'll probably won't need shutdown / sign out buttons.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc:101:
info_cache.GetNameOfProfileAtIndex(i) == displayName) {
On the desktop you can have local profiles that aren't signed in, so they aren't
tied to an email address (which is empty in that case). On the other hand,
display name alone might not be unique.
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> Why you have to check both email and displayName?
> Email seems to be enough to identify user. Or is it for cases when email is
> empty and only displayName could be used to identify such user?

[noms, 2013-06-12 20:41:54]

https://codereview.chromium.org/16104008/diff/33001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:14961: +      <message
name="IDS_LOGIN_PUBLIC_ACCOUNT_ENTER_ACCESSIBLE_NAME" desc="Text to be spoken
when focus is set to the sign-in button in the public account user pod.">
I've removed the strings. For now I've created a separate HTML template file to
deal with this, as I think the pod template from the mocks is a little bit
different as well.

On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> You won't need public account related strings on desktop.
> 
> But before these strings could be removed:
> * handler has to be fixed to not rely on them (ifdef)
> * make sure that no JS code uses them from loadTimeDate
> * HTML templates have to be per OS i.e. <div
> id="public-account-user-pod-extras-template" hidden> block is CrOS specific
> 
> Please add these as TODO somewhere in a separate cleanup bug.

https://codereview.chromium.org/16104008/diff/33001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:14976: +      <message
name="IDS_LOGIN_POD_OWNER_USER" desc="Login screen user pod menu title for a
user who owns the device.">
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> Same here - should be a TODO in cleanup.

Removed strings.

https://codereview.chromium.org/16104008/diff/33001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:14982: +      <message
name="IDS_CREATE_LOCALLY_MANAGED_USER_FEATURE_NAME" desc="Title close to icon
shown on side of Google sign-in UI to create a locally managed user.">
Removed strings.
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> Same TODO for locally managed users related strings.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:169: chrome::HostDesktopType
desktop_type,
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> nit: parameters need to be aligned

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.cc:652: 
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> nit: drop empty line

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:181: static void SwitchToProfile(
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> Please add method comment.

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
File chrome/browser/resources/chromeos/login/user_pod_row.js (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/user_pod_row.js:822: this.disabled =
true;
I copied the code from the standard user pod, but you're right. It doesn't make
sense. fixed. 
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> Why user pod is disabled?

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/user_pod_row.js:832: 
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> nit: drop empty line

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/chromeos/login/user_pod_row.js:942: if
(user.isDesktopScreen)
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> nit: isDesktopScreen is property of a user but is named as login UI mode.
> 
> Rename it to something like isDesktopUser?

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/header_bar.html (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/header_bar.html:1: <div
id="login-header-bar" class="login-header-bar">
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> nit: Please rename this file to control_bar.html. You could leave ID's and CSS
> rules as is.
> 
> Initially this was a header on CrOS login screen but than it was moved below
so
> become a footer of just control bar.

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/header_bar.js (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/header_bar.js:39:
$('sign-out-user-button').addEventListener('click',
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> You probably don't need sign out?

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/header_bar.js:41:
$('add-user-button').hidden = false;
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> Call updateUI_() instead?

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/user_chooser.html (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:2: <html
i18n-values="dir:textdirection;screen:screenType;lockanimations:lockAnimationsType;build:buildType">
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> lockanimations:lockAnimationsType - please remove

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:5: <link
rel="stylesheet" href="../chromeos/login/oobe.css">
Oops. Sorry! Done.
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> In this case <link> / <script> elements should not be aligned with spaces.
> Should be right below title element.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:39: <div
id="header-sections"></div>
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> nit: Please fix alignment - 2 spaces for elements, 4 spaces for attributes.

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:42: <include
src="../chromeos/login/screen_gaia_signin.html">
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> You probably don't need screen_gaia_signin.html.

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:44: <div
id="progress-dots" class="down"></div>
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> nit: Remove progress-dots

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/user_chooser.js (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.js:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> nit: 2013

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.js:16: function
setupSelect(select, list, callback) {
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> Where is setupSelect used? That looks like dead code from Chrome OS out-of-box
> code.

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.js:44:
login.GaiaSigninScreen.register();
Probably not. There's still discussions on what we'll do about it, so I've
removed it for now.
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> Are you planning on using the same GaiaSigninScreen to authenticate users?

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/user_chooser_common.js (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser_common.js:13: <include
src="../chromeos/login/apps_menu.js"></include>
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> These are specific to CrOS OOBE screens and should be removed:
> 
> user_images_grid.js
> apps_menu.js
> screen_error_message.js
> oobe_screen_user_image.js
> screen_gaia_signin.js
> screen_locally_managed_user_creation.js
> screen_password_changed.js
> screen_tpm_error.js
> screen_wrong_hwid.js

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser_common.js:22:
src="../chromeos/login/screen_locally_managed_user_creation.js"></include>
Removed the include altogether :)
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> nit: move </include> on next line instead?

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser_common.js:27: 
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> nit: drop empty line

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:172: 
On 2013/06/11 18:22:16, Nikita Kostylev wrote:
> nit: drop empty line

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/signin/user_chooser_screen_handler.h (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.h:8: #include
"base/values.h"
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> nit: remove this header and forward declare base::ListValue

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.h:9: #include
"chrome/browser/profiles/avatar_menu_model.h"
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> nit: not used?

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/signin/user_chooser_ui.cc (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_ui.cc:7: 
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> drop: empty line

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_ui.cc:9: 
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> nit: drop empty line

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_ui.cc:29: 
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> nit: drop empty line

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_ui.cc:68: 
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> nit: drop empty line

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/signin/user_chooser_ui.h (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_ui.h:8: #include "base/values.h"
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> nit: drop and forward declare instead

Done.

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_ui.h:17: // A WebUI dialog to
display available users
On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> nit: Add dot at the end.

Done.

[Nikita (slow), 2013-06-13 08:12:38]

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
File chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc (right):

https://codereview.chromium.org/16104008/diff/33001/chrome/browser/ui/webui/s...
chrome/browser/ui/webui/signin/user_chooser_screen_handler.cc:101:
info_cache.GetNameOfProfileAtIndex(i) == displayName) {
On 2013/06/11 19:39:41, Monica Dinculescu wrote:
> On the desktop you can have local profiles that aren't signed in, so they
aren't
> tied to an email address (which is empty in that case). On the other hand,
> display name alone might not be unique.
> On 2013/06/11 18:43:28, Nikita Kostylev wrote:
> > Why you have to check both email and displayName?
> > Email seems to be enough to identify user. Or is it for cases when email is
> > empty and only displayName could be used to identify such user?
> 

I've realized that there's probably no code that prevents you to add 2 different
"Chrome" users with the same email and display name. That's not an issue on
Chrome multi-profiles menu right now as they are identified by index.

Will this be an issue here as you imagine you duplicate users like have
user A: user1@gmail.com, "user A"
...
user B: user1@gmail.com, "user A"

When you select second (user B) user it will select user A in the backend
(here).
Does this make sense?

[Nikita (slow), 2013-06-14 16:51:35]

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/user_chooser.html (right):

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:37: <div
id="inner-container" class="down">
nit: 2 space indentation

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:37: <div
id="inner-container" class="down">
nit: drop class="down" attribute, only needed on CrOS boot.

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:40: </div>
nit: align with opening div tag

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:42: </div>
nit: align with opening div tag

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:45: <div id="bubble"
class="bubble faded" hidden></div>
nit: 2 space indentation

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/user_chooser.js (right):

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.js:4: <include
src="user_chooser_common.js"></include>
Move this include to html file.

[Nikita (slow), 2013-06-14 17:23:15]

Overall looks okay. I'll do a final check once all existing comments are
addressed.

https://codereview.chromium.org/16104008/diff/74001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/16104008/diff/74001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:181: // Opens a Browser with the
specified profile given by |path|.
nit: given > identified

https://codereview.chromium.org/16104008/diff/74001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/desktop_user_pod_template.html
(left):

https://codereview.chromium.org/16104008/diff/74001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/desktop_user_pod_template.html:8: <img
class="capslock-hint"
Why caps lock hint is dropped?

[noms (inactive), 2013-06-18 20:40:33]

This can't actually happen on the desktop. If UserA is signed in with
user1@gmail.com, then we're not allowing any other profile to sign in with the
same email address. So the emails themselves will never match. Similarly, if you
have a local user named "UserA", not signed in (so email address is ""), the UI
won't let you create a second local user with the same name. However, you could
have a signed in user that has the same name, but in this case the email
addresses will be different.

> I've realized that there's probably no code that prevents you to add 2
different
> "Chrome" users with the same email and display name. That's not an issue on
> Chrome multi-profiles menu right now as they are identified by index.
> 
> Will this be an issue here as you imagine you duplicate users like have
> user A: mailto:user1@gmail.com, "user A"
> ...
> user B: mailto:user1@gmail.com, "user A"
> 
> When you select second (user B) user it will select user A in the backend
> (here).
> Does this make sense?

[Nikita (slow), 2013-06-19 13:33:19]

On 2013/06/18 20:40:33, Monica Dinculescu wrote:
> This can't actually happen on the desktop. If UserA is signed in with
> mailto:user1@gmail.com, then we're not allowing any other profile to sign in
with the
> same email address. So the emails themselves will never match. Similarly, if
you
> have a local user named "UserA", not signed in (so email address is ""), the
UI
> won't let you create a second local user with the same name. However, you
could
> have a signed in user that has the same name, but in this case the email
> addresses will be different.
> 
> > I've realized that there's probably no code that prevents you to add 2
> different
> > "Chrome" users with the same email and display name. That's not an issue on
> > Chrome multi-profiles menu right now as they are identified by index.
> > 
> > Will this be an issue here as you imagine you duplicate users like have
> > user A: mailto:user1@gmail.com, "user A"
> > ...
> > user B: mailto:user1@gmail.com, "user A"
> > 
> > When you select second (user B) user it will select user A in the backend
> > (here).
> > Does this make sense?

Thanks for clarification. It is useful to have such restriction.

[noms (inactive), 2013-06-28 17:31:04]

There was a bit of a rebase disaster; please ignore Patch #18 which accidentally
deletes some files.

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/user_chooser.html (right):

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:37: <div
id="inner-container" class="down">
On 2013/06/14 16:51:35, Nikita Kostylev wrote:
> nit: drop class="down" attribute, only needed on CrOS boot.

Done.

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:37: <div
id="inner-container" class="down">
On 2013/06/14 16:51:35, Nikita Kostylev wrote:
> nit: 2 space indentation

Done.

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:40: </div>
On 2013/06/14 16:51:35, Nikita Kostylev wrote:
> nit: align with opening div tag

Done.

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:42: </div>
On 2013/06/14 16:51:35, Nikita Kostylev wrote:
> nit: align with opening div tag

Done.

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:45: <div id="bubble"
class="bubble faded" hidden></div>
Hmm, does this need to be indented, given that it's just under the body, at the
same level as login-display? 
On 2013/06/14 16:51:35, Nikita Kostylev wrote:
> nit: 2 space indentation

https://codereview.chromium.org/16104008/diff/74001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/16104008/diff/74001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_manager.h:181: // Opens a Browser with the
specified profile given by |path|.
On 2013/06/14 17:23:15, Nikita Kostylev wrote:
> nit: given > identified

Done.

https://codereview.chromium.org/16104008/diff/74001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/desktop_user_pod_template.html
(left):

https://codereview.chromium.org/16104008/diff/74001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/desktop_user_pod_template.html:8: <img
class="capslock-hint"
At the moment the password box is completely non functional, and there's still
some discussion about what the password will actually do. I was going to add
caps-lock back in a further CL that actually does something with the password.

On 2013/06/14 17:23:15, Nikita Kostylev wrote:
> Why caps lock hint is dropped?

[Nikita (slow), 2013-07-02 14:20:11]

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
File chrome/browser/resources/user_chooser/user_chooser.html (right):

https://codereview.chromium.org/16104008/diff/59001/chrome/browser/resources/...
chrome/browser/resources/user_chooser/user_chooser.html:45: <div id="bubble"
class="bubble faded" hidden></div>
On 2013/06/28 17:31:04, Monica Dinculescu wrote:
> Hmm, does this need to be indented, given that it's just under the body, at
the
> same level as login-display? 
> On 2013/06/14 16:51:35, Nikita Kostylev wrote:
> > nit: 2 space indentation
> 

Yes, it should. In fact everything inside <body> (like <div
id="outer-container">) should also be indented.

[Nikita (slow), 2013-07-02 14:26:09]

lgtm

https://codereview.chromium.org/16104008/diff/110001/chrome/browser/resources...
File chrome/browser/resources/user_chooser/desktop_user_pod_template.html
(right):

https://codereview.chromium.org/16104008/diff/110001/chrome/browser/resources...
chrome/browser/resources/user_chooser/desktop_user_pod_template.html:30: </div>
nit: It seems that you have extra closing <div> tag.

https://codereview.chromium.org/16104008/diff/110001/chrome/browser/resources...
File chrome/browser/resources/user_chooser/user_chooser.html (right):

https://codereview.chromium.org/16104008/diff/110001/chrome/browser/resources...
chrome/browser/resources/user_chooser/user_chooser.html:46: <include
src="control_bar.html">
nit: All tags inside <body> should have 2 spaces indent.

[noms, 2013-07-02 21:14:53]

+ Sailesh for owners stamp on:
chrome/app/theme/theme_resources.grd
chrome/browser/profiles/* 

Thanks!

https://codereview.chromium.org/16104008/diff/110001/chrome/browser/resources...
File chrome/browser/resources/user_chooser/desktop_user_pod_template.html
(right):

https://codereview.chromium.org/16104008/diff/110001/chrome/browser/resources...
chrome/browser/resources/user_chooser/desktop_user_pod_template.html:30: </div>
And some testing code. Eek. Fixed.
On 2013/07/02 14:26:09, Nikita Kostylev wrote:
> nit: It seems that you have extra closing <div> tag.

https://codereview.chromium.org/16104008/diff/110001/chrome/browser/resources...
File chrome/browser/resources/user_chooser/user_chooser.html (right):

https://codereview.chromium.org/16104008/diff/110001/chrome/browser/resources...
chrome/browser/resources/user_chooser/user_chooser.html:46: <include
src="control_bar.html">
On 2013/07/02 14:26:09, Nikita Kostylev wrote:
> nit: All tags inside <body> should have 2 spaces indent.

Done.

[sail, 2013-07-02 21:46:39]

Some high level comments:
  - CL description needs better formatting:
      - First line should be short summary (just copy the subject field of the
code review)
       - you should manually break lines at 72 characters
  - need to add links to screenshots to your CL description. Ideally you should
include links to screenshots for Views, Cocoa, and one in RTL

You mention that avatar resources look pixelated. What size do you need them to
be? The Chrome desktop avatar resources were provided by a 3rd party. Do you
have a plan for getting them rendered at a higher resolution?

https://codereview.chromium.org/16104008/diff/138001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/16104008/diff/138001/chrome/app/generated_res...
chrome/app/generated_resources.grd:15150: +        Add user
You need a title case version for Mac. See use_titlecase

https://codereview.chromium.org/16104008/diff/138001/chrome/app/theme/theme_r...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/16104008/diff/138001/chrome/app/theme/theme_r...
chrome/app/theme/theme_resources.grd:351: <structure type="chrome_scaled_image"
name="IDR_ICON_POWER_WHITE" file="icon_power_white.png" />
Did you intend to remove this from the ifdef?

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

https://codereview.chromium.org/16104008/diff/138001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:648: void
ProfileManager::SwitchToProfile(
I don't think there's any value in having this in ProfileManager. Can we keep
all this in AvatarMenuModel instead?

[sail, 2013-07-02 23:11:46]

Also, this needs a bug number.

[noms, 2013-07-03 17:11:34]

I've updated the CL description. For the pixelated avatars, the plan is to move
towards the CrOS profile image chooser (in which the images are a bigger
resolution, and could also allow the user to upload their own), but that code is
still in the planning stages and isn't ready yet.

https://codereview.chromium.org/16104008/diff/138001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/16104008/diff/138001/chrome/app/generated_res...
chrome/app/generated_resources.grd:15150: +        Add user
I've titlecased it for all platforms for now, as the new mocks show the buttons
title cased always. The mocks aren't final yet, so the text might change again
in the future.

On 2013/07/02 21:46:40, sail wrote:
> You need a title case version for Mac. See use_titlecase

https://codereview.chromium.org/16104008/diff/138001/chrome/app/theme/theme_r...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/16104008/diff/138001/chrome/app/theme/theme_r...
chrome/app/theme/theme_resources.grd:351: <structure type="chrome_scaled_image"
name="IDR_ICON_POWER_WHITE" file="icon_power_white.png" />
I intended to remove some of them (to reuse them on both chrome os and the
desktop platforms). I've made the ifdef tighter rather than removing it
altogether (as the user chooser won't be used on mobile)
On 2013/07/02 21:46:40, sail wrote:
> Did you intend to remove this from the ifdef?

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

https://codereview.chromium.org/16104008/diff/138001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:648: void
ProfileManager::SwitchToProfile(
The reason I moved it here is that the user chooser page also needs to open a
window for a selected profile, and as it is, it would need to instantiate a
mostly-unused AvatarMenuModel. Roger had suggested in a previous comment to
factor it out to a standalone function. Given that it calls CreateProfileAsync
on the profile manager (and doesn't really do anything about the avatars), I
thought the ProfileManager would be a better place for it. Do you think it
should be a static function in the AvatarMenuModel instead? 

On 2013/07/02 21:46:40, sail wrote:
> I don't think there's any value in having this in ProfileManager. Can we keep
> all this in AvatarMenuModel instead?

[sail, 2013-07-03 18:18:04]

> For the pixelated avatars, the plan is to move
> towards the CrOS profile image chooser

I'm not sure what this means. Could you write a doc about the exact plan?

For example, are existing profile icons going to be removed? If so, what happens
to existing profiles. If not then what do you show in your UI?

Also, just referencing the bug number for screenshots is not idea. For example,
as you iterate it's hard to find the latest version. Find a good image hosting
site and link directly to the image. Something like:
  Screenshots: Mac: example.com/1.png
                       Windows: example.com/2.png
                       Windows RTL: example.com/3.png

Also, is the screenshot on the bug up to date? You mentioned that you switched
everything to title case but the "sign in" button is not title case. Also, the
sign in button needs to be styled to match buttons in the rest of the web ui.

Finally, the avatar icons are not sized correctly . They have the wrong aspect
ratio.

https://codereview.chromium.org/16104008/diff/138001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/16104008/diff/138001/chrome/app/generated_res...
chrome/app/generated_resources.grd:15150: +        Add user
On 2013/07/03 17:11:34, noms wrote:
> I've titlecased it for all platforms for now, as the new mocks show the
buttons
> title cased always. The mocks aren't final yet, so the text might change again
> in the future.
> 
> On 2013/07/02 21:46:40, sail wrote:
> > You need a title case version for Mac. See use_titlecase
> 

This is not correct. Windows does not use title case buttons or menus.

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

https://codereview.chromium.org/16104008/diff/138001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:648: void
ProfileManager::SwitchToProfile(
On 2013/07/03 17:11:35, noms wrote:
> The reason I moved it here is that the user chooser page also needs to open a
> window for a selected profile, and as it is, it would need to instantiate a
> mostly-unused AvatarMenuModel. Roger had suggested in a previous comment to
> factor it out to a standalone function. Given that it calls CreateProfileAsync
> on the profile manager (and doesn't really do anything about the avatars), I
> thought the ProfileManager would be a better place for it. Do you think it
> should be a static function in the AvatarMenuModel instead? 
> 
> On 2013/07/02 21:46:40, sail wrote:
> > I don't think there's any value in having this in ProfileManager. Can we
keep
> > all this in AvatarMenuModel instead?
> 

Adding this to profile manager is a bad idea.
The profile manager API is already more complicated than it needs to be.

[noms, 2013-07-05 17:39:27]

I've put together a small design document that explains the motivation behind
this feature, and also addresses the avatar images situation in a bit more
detail:
https://docs.google.com/a/google.com/document/d/1fg47-s19akfqaKNqjggS6Igj_pkX...

To answer Sailesh's other comments:
- the reason why the avatar images look stretched in the screenshot is that the
original image is not square (it's 31x38 px), and it's being re-sized to a
square
- the "Sign in" button currently has the default styling from the CrOS code. The
mocks keep changing and have not been finalized (for example the bottom control
bar buttons used to be dark grey but now no longer are), so what this button
should look like is currently unknown. This CL is just the first iteration of
this screen (it's lacking a ton of functionality as well), so I've put it behind
a flag until it's ready. I will definitely have a future CL addressing all the
styling issues, once the design team has agreed on the mocks.

https://codereview.chromium.org/16104008/diff/138001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/16104008/diff/138001/chrome/app/generated_res...
chrome/app/generated_resources.grd:15150: +        Add user
On 2013/07/03 18:18:04, sail wrote:
> On 2013/07/03 17:11:34, noms wrote:
> > I've titlecased it for all platforms for now, as the new mocks show the
> buttons
> > title cased always. The mocks aren't final yet, so the text might change
again
> > in the future.
> > 
> > On 2013/07/02 21:46:40, sail wrote:
> > > You need a title case version for Mac. See use_titlecase
> > 
> 
> This is not correct. Windows does not use title case buttons or menus.

Done.

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

https://codereview.chromium.org/16104008/diff/138001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:648: void
ProfileManager::SwitchToProfile(
As discussed offline, this is ok because it's only adding one static function,
that needs to be called from placed that aren't just the AvatarMenuModel, and
whose functionality is only related to the ProfileManager.

On 2013/07/03 18:18:04, sail wrote:
> On 2013/07/03 17:11:35, noms wrote:
> > The reason I moved it here is that the user chooser page also needs to open
a
> > window for a selected profile, and as it is, it would need to instantiate a
> > mostly-unused AvatarMenuModel. Roger had suggested in a previous comment to
> > factor it out to a standalone function. Given that it calls
CreateProfileAsync
> > on the profile manager (and doesn't really do anything about the avatars), I
> > thought the ProfileManager would be a better place for it. Do you think it
> > should be a static function in the AvatarMenuModel instead? 
> > 
> > On 2013/07/02 21:46:40, sail wrote:
> > > I don't think there's any value in having this in ProfileManager. Can we
> keep
> > > all this in AvatarMenuModel instead?
> > 
> 
> Adding this to profile manager is a bad idea.
> The profile manager API is already more complicated than it needs to be.

[sail, 2013-07-05 17:49:28]

It seems like the current approach is to implement the feature and design it
later. This is backwards.

This isn't iteration. If you want to iterate then what you should do is:
  - get a proper design from the UX folks
  - implement that design (across multiple CLs is fine)
  - find problems with the design and iterate on improvements

For example, your docs doesn't address my question about existing avatars. It
just restates the question.

Lets first figure out what we're going to do. Then implement it.

> As discussed offline, this is ok because it's only adding one static function,
> that needs to be called from placed that aren't just the AvatarMenuModel, and
> whose functionality is only related to the ProfileManager.

That's not what I meant. If you don't have time to clarify the ProfileManager
API then add your new api to a new profile_manager_my_util_file.h or something.

[noms, 2013-07-09 20:41:57]

I have uploaded a new patch that has:
- consistent button styles
- RTL support
- no pixelated avatars -- we will display a default "chrome user" avatar until
better resources exist
- title case for the Mac. "Guest" is capitalized on purpose on all platforms
(like it is in CrOS), as it's a proper noun.

I will move the profile manager function once this CL lands:
https://codereview.chromium.org/18615010/

On 2013/07/05 17:49:28, sail wrote:
> It seems like the current approach is to implement the feature and design it
> later. This is backwards.
> 
> This isn't iteration. If you want to iterate then what you should do is:
>   - get a proper design from the UX folks
>   - implement that design (across multiple CLs is fine)
>   - find problems with the design and iterate on improvements
> 
> For example, your docs doesn't address my question about existing avatars. It
> just restates the question.
> 
> Lets first figure out what we're going to do. Then implement it.
> 
> > As discussed offline, this is ok because it's only adding one static
function,
> > that needs to be called from placed that aren't just the AvatarMenuModel,
and
> > whose functionality is only related to the ProfileManager.
> 
> That's not what I meant. If you don't have time to clarify the ProfileManager
> API then add your new api to a new profile_manager_my_util_file.h or
something.

[sail, 2013-07-11 07:10:15]

LGTM
Looks great. Thanks so much for sticking through this.

[noms, 2013-07-18 15:43:28]

Roger: I have moved the utility function out of the ProfileManager and into
profile_window.cc, as requested by Sailesh. Could you please give it a
sanity-check look over before I commit the CL? Thanks!

[Roger Tawa OOO till Jul 10th, 2013-07-18 16:04:37]

still lgtm

https://codereview.chromium.org/16104008/diff/186001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/16104008/diff/186001/chrome/browser/profiles/...
chrome/browser/profiles/profile_window.cc:30: Profile::CreateStatus status) {
Align args to first one.

[Nikita (slow), 2013-07-18 17:24:20]

CC: xiyuan@

[noms, 2013-07-18 18:04:54]

https://codereview.chromium.org/16104008/diff/186001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_window.cc (right):

https://codereview.chromium.org/16104008/diff/186001/chrome/browser/profiles/...
chrome/browser/profiles/profile_window.cc:30: Profile::CreateStatus status) {
On 2013/07/18 16:04:37, Roger Tawa wrote:
> Align args to first one.

Done.

[commit-bot: I haz the power, 2013-07-18 18:05:44]

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

[commit-bot: I haz the power, 2013-07-18 19:20:33]

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

[commit-bot: I haz the power, 2013-07-18 19:22:40]

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

[commit-bot: I haz the power, 2013-07-18 19:43:14]

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

[noms, 2013-07-18 20:16:56]

Oops! Missed an owner.
+Scott for owner stamp on chrome/browser/browser_resources.grd

[sky, 2013-07-19 14:56:16]

LGTM

[commit-bot: I haz the power, 2013-07-19 14:57:35]

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

[commit-bot: I haz the power, 2013-07-19 18:55:29]

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

[commit-bot: I haz the power, 2013-07-19 19:21:30]

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

[commit-bot: I haz the power, 2013-07-19 23:35:45]

Change committed as 212688