material design user manager with create profile flow

material design user manager with create profile flow

it excludes changes to the user pods as well profile import flow

BUG=563722

[Moe, 2016-01-25 19:46:23]

mahmadi@chromium.org changed reviewers:
+ anthonyvd@chromium.org, rogerta@chromium.org

[Roger Tawa OOO till Jul 10th, 2016-01-27 19:13:26]

lgtm

I mostly looked at the C++ code.  Maybe you could upload some screenshots to the
bug?

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

https://codereview.chromium.org/1630903002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:476: void
SigninCreateProfileHandler::IsValidExistingSupervisedUser(
I would expect this method to return a bool given its name.  Maybe call it
DoCreateProfileIfPossible ?

[Moe, 2016-01-29 14:42:57]

mahmadi@chromium.org changed reviewers:
+ estade@chromium.org

[Moe, 2016-01-29 14:42:58]

@rogerta, addressed your comment.

@estade, This is the first patch for the new material design user manager
screen. I appreciate it if you reviewed my changes.

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

https://codereview.chromium.org/1630903002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:476: void
SigninCreateProfileHandler::IsValidExistingSupervisedUser(
On 2016/01/27 19:13:26, Roger Tawa wrote:
> I would expect this method to return a bool given its name.  Maybe call it
> DoCreateProfileIfPossible ?

Done.

[anthonyvd, 2016-01-29 15:50:17]

anthonyvd@chromium.org changed reviewers:
+ mlerman@chromium.org

[anthonyvd, 2016-01-29 15:50:18]

Great work! Just a few minor comments.

+mlerman. Mike, could you please take a look specifically at the profile
creation code in signin_create_profile_handler.cc in case there's some caveat
I'm missing? Basically we're creating Supervised Users in the Profile Manager
now so there's some code to load the Profile that does the supervising if
needed.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/user_manager_tutorial.js (right):

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager_tutorial.js:18: 'friends',
nit: extra leading space.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/md_user_manager_ui.cc (right):

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/md_user_manager_ui.cc:97:
localized_strings->SetString("buildType", "chrome");
I'm fairly confident there's a string defined as the "branded name" already.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_create_profile_handler.cc (right):

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:59:
l10n_util::GetStringUTF16(
nit: need 2 extra leading spaces

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:62:
l10n_util::GetStringUTF16(
nit: need 2 extra leading spaces

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:67:
l10n_util::GetStringUTF16(IDS_LEARN_MORE));
nit: need 2 extra leading spaces

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:125:
ProfileManager* pm = g_browser_process->profile_manager();
nit: Chrome code tends to use full names over abreviations, so profile_manager
or even just manager here.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:126: const
ProfileInfoCache& cache = pm->GetProfileInfoCache();
The ProfileInfoCache is deprecated, so you should use ProfileAttributesStorage
instead. Keep in mind the latter isn't sorted so you'll have to do that if you
need it.

[Mike Lerman, 2016-01-29 19:48:22]

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_create_profile_handler.cc (right):

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:113: std::string
url = profiles::GetDefaultAvatarIconUrl(i);
This is going to include the grey silhouette, kPlaceholderAvatarIcon, which we
probably want to skip.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:122: void
SigninCreateProfileHandler::RequestCreateProfileUpdate(
This method name doesn't seem to reflect what the function does. The function's
all about finding all the signed in profiles, that should be in the method name.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:128: if
(!cache.ProfileIsAuthErrorAtIndex(i)) {
I think skipping profiles where there's an auth error will lead to unexpected
behaviour - especially if the profile the user actively uses is in error! (this
happens more often then you'd think)

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:141:
web_ui()->CallJavascriptFunction("cr.ui.ProfileApi.updateSignedInUsers",
thought: what if the profile's locked?

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:473: }
If this was returned to with some error code, then the user's left hanging; we
never create the profile nor return any message to the user via the UI.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:492: return;
Shouldn't there be some feedback to the user that the SupervisedUser already
exists on the client?

[Evan Stade, 2016-01-30 01:52:12]

estade@chromium.org changed reviewers:
+ dbeam@chromium.org

[Evan Stade, 2016-01-30 01:52:12]

These days +dbeam is a better reviewer for webui stuff and particularly MD webui
stuff. Dan, can you review or delegate?

[Moe, 2016-02-01 01:11:14]

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/user_manager_tutorial.js (right):

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager_tutorial.js:18: 'friends',
On 2016/01/29 15:50:17, anthonyvd wrote:
> nit: extra leading space.

Done.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/md_user_manager_ui.cc (right):

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/md_user_manager_ui.cc:97:
localized_strings->SetString("buildType", "chrome");
On 2016/01/29 15:50:17, anthonyvd wrote:
> I'm fairly confident there's a string defined as the "branded name" already.

I couldn't find the string you're referring to. Could you please point me to it?

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/signin_create_profile_handler.cc (right):

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:59:
l10n_util::GetStringUTF16(
On 2016/01/29 15:50:17, anthonyvd wrote:
> nit: need 2 extra leading spaces

Done.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:62:
l10n_util::GetStringUTF16(
On 2016/01/29 15:50:17, anthonyvd wrote:
> nit: need 2 extra leading spaces

Done.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:67:
l10n_util::GetStringUTF16(IDS_LEARN_MORE));
On 2016/01/29 15:50:17, anthonyvd wrote:
> nit: need 2 extra leading spaces

Done.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:113: std::string
url = profiles::GetDefaultAvatarIconUrl(i);
On 2016/01/29 19:48:22, Mike Lerman wrote:
> This is going to include the grey silhouette, kPlaceholderAvatarIcon, which we
> probably want to skip.

Done.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:122: void
SigninCreateProfileHandler::RequestCreateProfileUpdate(
On 2016/01/29 19:48:22, Mike Lerman wrote:
> This method name doesn't seem to reflect what the function does. The
function's
> all about finding all the signed in profiles, that should be in the method
name.

Done.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:125:
ProfileManager* pm = g_browser_process->profile_manager();
On 2016/01/29 15:50:17, anthonyvd wrote:
> nit: Chrome code tends to use full names over abreviations, so profile_manager
> or even just manager here.

Done.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:126: const
ProfileInfoCache& cache = pm->GetProfileInfoCache();
On 2016/01/29 15:50:18, anthonyvd wrote:
> The ProfileInfoCache is deprecated, so you should use ProfileAttributesStorage
> instead. Keep in mind the latter isn't sorted so you'll have to do that if you
> need it.

Done.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:128: if
(!cache.ProfileIsAuthErrorAtIndex(i)) {
On 2016/01/29 19:48:22, Mike Lerman wrote:
> I think skipping profiles where there's an auth error will lead to unexpected
> behaviour - especially if the profile the user actively uses is in error!
(this
> happens more often then you'd think)

Done.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:141:
web_ui()->CallJavascriptFunction("cr.ui.ProfileApi.updateSignedInUsers",
On 2016/01/29 19:48:22, Mike Lerman wrote:
> thought: what if the profile's locked?

The decision was to allow any user add a supervised user to any signed in
account. I don't know yet if re-authentication will be  required in case of a
locked profile, but the UI should allow that.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:473: }
On 2016/01/29 19:48:22, Mike Lerman wrote:
> If this was returned to with some error code, then the user's left hanging; we
> never create the profile nor return any message to the user via the UI.

Done.

https://codereview.chromium.org/1630903002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:492: return;
On 2016/01/29 19:48:22, Mike Lerman wrote:
> Shouldn't there be some feedback to the user that the SupervisedUser already
> exists on the client?

I agree. For now I'm going to use the generic local error message for supervised
user creation. I'll discuss this with the PM to see if it'll need a more
specific message.

[Mike Lerman, 2016-02-01 16:11:20]

Cool, thanks! SigninCreateProfileHandler lgtm

[Dan Beam, 2016-02-01 20:00:57]

Description was changed from

==========
material design user manager with create profile flow

it excludes changes to the user pods as well profile import flow

BUG=563722
==========

to

==========
material design user manager with create profile flow

it excludes changes to the user pods as well profile import flow

BUG=563722
==========

[Dan Beam, 2016-02-01 20:00:57]

dbeam@chromium.org changed reviewers:
+ tommycli@chromium.org
- dbeam@chromium.org

[Dan Beam, 2016-02-01 20:03:33]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-02-01 20:03:34]

tommycli@ is probably a better initial reviewer as he's been following the
profile/user management with ewald more closely

[tommycli, 2016-02-01 20:13:07]

On 2016/02/01 20:03:34, Dan Beam wrote:
> tommycli@ is probably a better initial reviewer as he's been following the
> profile/user management with ewald more closely

That sounds good. I'll take a look at this.

[tommycli, 2016-02-03 01:13:22]

moe: I took a first pass, but didn't look through all the HTML and JS and CSS
files.

The C++ changes have already been reviewed and approved by the other reviewers
right? If that's the case, I'm mostly going to focus on the HTML/JS/CSS.

The below comments apply to all the HTML/JS/CSS files. Thanks!

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/control_bar.css (right):

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.css:39: right: 24px;
In this case, doesn't left need to be set to the default value also?

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/control_bar.js (right):

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.js:6: Polymer({
It would be good to add a @fileoverview describing the control. ideally
describing the purpose of the control.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.js:12: * If true then
|Browse as Guest| button is displayed.
nit: formatting. The stars are aligned and the top line has two stars.

Here for example.
https://developers.google.com/closure/compiler/docs/js-for-compiler

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.css (right):

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.css:47: background:
#DEDEDE;
I believe the preferred style is the rgb style when you cannot fit it into a
three character hex notation.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.html (right):

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:15: <link
rel="stylesheet" type="css" href="create_profile.css">
indentation seems off. Everything needs to be indented over. Also try "git cl
format".

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:6: Polymer({
Also needs the fileoverview.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:10: 
extra line

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:24: ready: function()
{
This should be on "attached" with the override annotation

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:25:
cr.ui.ProfileApi.setProfileCreateSuccessCallback(
All of these callbacks should be unset in the corresponding detached section. (I
know the sync code doesn't do that. It's a bug)

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:38: * ProfileApi
every time the page becomes visible.
annotate the param. Here and in all the below functions.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:49: /*
Two stars to begin these block comments.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:76:
handleUpdateSignedInUsers_: function(signedIn_users) {
camelCase preferred

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/profile_api.js (right):

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/profile_api.js:6: cr.define('cr.ui',
function() {
This and all of the JS should be compiled. See the examples set in
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....

You can run the closure compiler with:
https://www.chromium.org/developers/how-tos/closure-compilation

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/profile_api.js:12:
ProfileApi.getAvailableIconsCallback_ = null;
Need annotations

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/md_user_manager_ui.cc (right):

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/md_user_manager_ui.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
Try using --similarity=40 or lower so git cl upload picks up that this is a copy
instead of a brand new file. Allows reviewers to see the delta earlier.

[Moe, 2016-02-05 17:58:39]

Addressed tommy's comments

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/control_bar.css (right):

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.css:39: right: 24px;
On 2016/02/03 01:13:21, tommycli wrote:
> In this case, doesn't left need to be set to the default value also?

Done.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/control_bar.js (right):

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.js:6: Polymer({
On 2016/02/03 01:13:21, tommycli wrote:
> It would be good to add a @fileoverview describing the control. ideally
> describing the purpose of the control.

Done. Also added to other JS files.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.js:12: * If true then
|Browse as Guest| button is displayed.
On 2016/02/03 01:13:21, tommycli wrote:
> nit: formatting. The stars are aligned and the top line has two stars.
> 
> Here for example.
> https://developers.google.com/closure/compiler/docs/js-for-compiler

Done. Also fixed in other JS files.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.css (right):

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.css:47: background:
#DEDEDE;
On 2016/02/03 01:13:21, tommycli wrote:
> I believe the preferred style is the rgb style when you cannot fit it into a
> three character hex notation.

Done.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.html (right):

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:15: <link
rel="stylesheet" type="css" href="create_profile.css">
On 2016/02/03 01:13:21, tommycli wrote:
> indentation seems off. Everything needs to be indented over. Also try "git cl
> format".

Done. Also fixed other html files.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:6: Polymer({
On 2016/02/03 01:13:21, tommycli wrote:
> Also needs the fileoverview.

Done.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:10: 
On 2016/02/03 01:13:21, tommycli wrote:
> extra line

Done.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:24: ready: function()
{
On 2016/02/03 01:13:22, tommycli wrote:
> This should be on "attached" with the override annotation

Done.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:25:
cr.ui.ProfileApi.setProfileCreateSuccessCallback(
On 2016/02/03 01:13:21, tommycli wrote:
> All of these callbacks should be unset in the corresponding detached section.
(I
> know the sync code doesn't do that. It's a bug)

Done.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:38: * ProfileApi
every time the page becomes visible.
On 2016/02/03 01:13:21, tommycli wrote:
> annotate the param. Here and in all the below functions.

Done.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:49: /*
On 2016/02/03 01:13:22, tommycli wrote:
> Two stars to begin these block comments.

Done.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:76:
handleUpdateSignedInUsers_: function(signedIn_users) {
On 2016/02/03 01:13:21, tommycli wrote:
> camelCase preferred

Done.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/profile_api.js (right):

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/profile_api.js:6: cr.define('cr.ui',
function() {
On 2016/02/03 01:13:22, tommycli wrote:
> This and all of the JS should be compiled. See the examples set in
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....
> 
> You can run the closure compiler with:
> https://www.chromium.org/developers/how-tos/closure-compilation

Done.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/profile_api.js:12:
ProfileApi.getAvailableIconsCallback_ = null;
On 2016/02/03 01:13:22, tommycli wrote:
> Need annotations

Done.

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/md_user_manager_ui.cc (right):

https://codereview.chromium.org/1630903002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/md_user_manager_ui.cc:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2016/02/03 01:13:22, tommycli wrote:
> Try using --similarity=40 or lower so git cl upload picks up that this is a
copy
> instead of a brand new file. Allows reviewers to see the delta earlier.

Thanks for the tip!

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/user_manager_tutorial.js (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager_tutorial.js:6: /**
@tommy, not sure if this is the right way to quiet the closure compiler warning
about $(pod-row) not having "pods" and "focusPod()" properties defined.

[tommycli, 2016-02-08 18:45:19]

Moe: we are getting closer. thanks!

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/control_bar.html (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.html:8: <div
id="login-header-bar" class="login-header-bar layout horizontal \
I have not seen the '\' used for line continuation in chrome html. Does it fit
if you just put the class 'attr' on a second line? If not, it can exceed 80
char, that seems better than breaking up the attr body.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/control_bar.js (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.js:18: * Controls whether
that |Browse as Guest| button is displayed.
Actually I thought the "True if ..." is better since "Controls..." is more
vague.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.css (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.css:47: background:
rgb(222,222,222);
I think most of the rgb notations do have spaces between commas:
https://code.google.com/p/chromium/codesearch#search/&q=rgb%5C(%20file:%5Esrc...

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.html (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:27:
active$="{{isActiveIcon_(item, profileIconUrl)}}">
I think in most of these cases where you use the '$=' attribute binding, you
only need the '=' property binding. 

See
https://www.polymer-project.org/1.0/docs/devguide/data-binding.html#attribute...

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:50:
i18n-values=".innerHTML:manageProfilesSupervisedNotSignedIn"
I think you can do i18n-content instead of innerHTML.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:54: <div
id="messageBubble" hidden$="{{!errorMessage_}}"></div>
See the js page on comments on how to improve message bubble.

Also I think you can hide this with pure CSS, should be faster:
https://css-tricks.com/almanac/selectors/e/empty/

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:63:
disabled$="{{isOkDisabled_(createInProgress_, profileName, \
No line continuations. If it doesn't fit, it's okay to be over 80 characters.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:18: * Indicates
whether the supervised user checkbox should be disabled.
True if the supervised user checkbox is disabled.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:88:
this.availableIconUrls = [];
Hi, this needs to be a property if it's going to be referenced from the HTML via
data binding. The same applies for all these other member variables.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:200: handleWarning_:
function(warningHTML) {
Since this is the same as Error (just displays html in a message bubble, can we
combine this with handleError_? handleMessage_?

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:217: * @param
{!string} newValue New value of the |errorMessage_|
string is primitive and thus implied to be non-nullable.

You don't need {!string}, {string} implies non-nullable.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:220:
errorMessageChanged_: function(newValue) {
Can we accomplish this goal without this observer? Does the newValue contain
HTML? Or just plain text?

http://stackoverflow.com/questions/32595018/binding-html-rather-than-plain-text

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:229: * @return
{boolean}
I think i've usually seen @return above @private.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/supervised_user_learn_more.js
(right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/supervised_user_learn_more.js:8: *
'supervised-user-learn-more' is a page that contains information about
maybe more specifically: "contains an explanation of what supervised users" or
something.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/user_manager.css (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.css:21: box-shadow: 0 1px
2px rgba(0,0,0,0.1);
Why did the spaces go away?

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/user_manager.html (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.html:7: <meta
charset="utf-8">
I know you didn't create this file, but can you fix the indentation while you're
here?

See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
for an example.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/user_manager.js (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:34: * @param {boolean}
showGuest whether |Browse as Guest| button is displayed.
"True if the... should be displayed."

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:35: * @param {boolean}
showAddPerson wether |Add Person| button is displayed.
misspelled

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:55: * @param {!string}
profilePath The profile's path.
I believe string is implicitly non-nullable.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:70: * @param {!string}
optEmail An optional email for signin UI.
Is it blank if no email? or null?

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/user_manager_tutorial.js (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager_tutorial.js:6: /**
On 2016/02/05 17:58:39, Moe wrote:
> @tommy, not sure if this is the right way to quiet the closure compiler
warning
> about $(pod-row) not having "pods" and "focusPod()" properties defined.

I think when you create podRow variable, down on line 100, you typecast it to an
object with these properties.

https://developers.google.com/closure/compiler/docs/js-for-compiler#instantia...
https://groups.google.com/forum/#!topic/closure-compiler-discuss/cf_r0oet4y4

Try that and let me know if it doesn't work. Thanks.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager_tutorial.js:41: 'friends',
alignment

[Moe, 2016-02-09 00:15:34]

Thanks Tommy! Addressed your comments.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/control_bar.html (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.html:8: <div
id="login-header-bar" class="login-header-bar layout horizontal \
On 2016/02/08 18:45:18, tommycli wrote:
> I have not seen the '\' used for line continuation in chrome html. Does it fit
> if you just put the class 'attr' on a second line? If not, it can exceed 80
> char, that seems better than breaking up the attr body.

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/control_bar.js (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.js:18: * Controls whether
that |Browse as Guest| button is displayed.
On 2016/02/08 18:45:18, tommycli wrote:
> Actually I thought the "True if ..." is better since "Controls..." is more
> vague.

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.css (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.css:47: background:
rgb(222,222,222);
On 2016/02/08 18:45:18, tommycli wrote:
> I think most of the rgb notations do have spaces between commas:
>
https://code.google.com/p/chromium/codesearch#search/&q=rgb%5C(%20file:%5Esrc...

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.html (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:27:
active$="{{isActiveIcon_(item, profileIconUrl)}}">
On 2016/02/08 18:45:18, tommycli wrote:
> I think in most of these cases where you use the '$=' attribute binding, you
> only need the '=' property binding. 
> 
> See
>
https://www.polymer-project.org/1.0/docs/devguide/data-binding.html#attribute...

you're right. except for "hidden" the rest are properties.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:50:
i18n-values=".innerHTML:manageProfilesSupervisedNotSignedIn"
On 2016/02/08 18:45:18, tommycli wrote:
> I think you can do i18n-content instead of innerHTML.

it seems like i18n-content sets the text content of the node. the error message
here can be HTML.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:54: <div
id="messageBubble" hidden$="{{!errorMessage_}}"></div>
On 2016/02/08 18:45:18, tommycli wrote:
> See the js page on comments on how to improve message bubble.
> 
> Also I think you can hide this with pure CSS, should be faster:
> https://css-tricks.com/almanac/selectors/e/empty/

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:63:
disabled$="{{isOkDisabled_(createInProgress_, profileName, \
On 2016/02/08 18:45:18, tommycli wrote:
> No line continuations. If it doesn't fit, it's okay to be over 80 characters.

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:18: * Indicates
whether the supervised user checkbox should be disabled.
On 2016/02/08 18:45:19, tommycli wrote:
> True if the supervised user checkbox is disabled.

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:88:
this.availableIconUrls = [];
On 2016/02/08 18:45:19, tommycli wrote:
> Hi, this needs to be a property if it's going to be referenced from the HTML
via
> data binding. The same applies for all these other member variables.

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:200: handleWarning_:
function(warningHTML) {
On 2016/02/08 18:45:19, tommycli wrote:
> Since this is the same as Error (just displays html in a message bubble, can
we
> combine this with handleError_? handleMessage_?

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:217: * @param
{!string} newValue New value of the |errorMessage_|
On 2016/02/08 18:45:19, tommycli wrote:
> string is primitive and thus implied to be non-nullable.
> 
> You don't need {!string}, {string} implies non-nullable.

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:220:
errorMessageChanged_: function(newValue) {
On 2016/02/08 18:45:19, tommycli wrote:
> Can we accomplish this goal without this observer? Does the newValue contain
> HTML? Or just plain text?
> 
>
http://stackoverflow.com/questions/32595018/binding-html-rather-than-plain-text

newValue can be HTML. I came across the above link originally. nice hack but I
didn't see any usage in the codebase so went ahead with the observer. If you
think it's not too hacky it's fine with me.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:229: * @return
{boolean}
On 2016/02/08 18:45:19, tommycli wrote:
> I think i've usually seen @return above @private.

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/supervised_user_learn_more.js
(right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/supervised_user_learn_more.js:8: *
'supervised-user-learn-more' is a page that contains information about
On 2016/02/08 18:45:19, tommycli wrote:
> maybe more specifically: "contains an explanation of what supervised users" or
> something.

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/user_manager.css (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.css:21: box-shadow: 0 1px
2px rgba(0,0,0,0.1);
On 2016/02/08 18:45:19, tommycli wrote:
> Why did the spaces go away?

used an external css cleanup tool...

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/user_manager.html (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.html:7: <meta
charset="utf-8">
On 2016/02/08 18:45:19, tommycli wrote:
> I know you didn't create this file, but can you fix the indentation while
you're
> here?
> 
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> for an example.

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/user_manager.js (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:34: * @param {boolean}
showGuest whether |Browse as Guest| button is displayed.
On 2016/02/08 18:45:19, tommycli wrote:
> "True if the... should be displayed."

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:35: * @param {boolean}
showAddPerson wether |Add Person| button is displayed.
On 2016/02/08 18:45:19, tommycli wrote:
> misspelled

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:55: * @param {!string}
profilePath The profile's path.
On 2016/02/08 18:45:19, tommycli wrote:
> I believe string is implicitly non-nullable.

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:70: * @param {!string}
optEmail An optional email for signin UI.
On 2016/02/08 18:45:19, tommycli wrote:
> Is it blank if no email? or null?

seems like either blank or an email. updated it accordingly.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/user_manager_tutorial.js (right):

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager_tutorial.js:6: /**
On 2016/02/08 18:45:19, tommycli wrote:
> On 2016/02/05 17:58:39, Moe wrote:
> > @tommy, not sure if this is the right way to quiet the closure compiler
> warning
> > about $(pod-row) not having "pods" and "focusPod()" properties defined.
> 
> I think when you create podRow variable, down on line 100, you typecast it to
an
> object with these properties.
> 
>
https://developers.google.com/closure/compiler/docs/js-for-compiler#instantia...
> https://groups.google.com/forum/#!topic/closure-compiler-discuss/cf_r0oet4y4
> 
> Try that and let me know if it doesn't work. Thanks.

It worked. Thank you!

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager_tutorial.js:6: /**
On 2016/02/05 17:58:39, Moe wrote:
> @tommy, not sure if this is the right way to quiet the closure compiler
warning
> about $(pod-row) not having "pods" and "focusPod()" properties defined.

Done.

https://codereview.chromium.org/1630903002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager_tutorial.js:41: 'friends',
On 2016/02/08 18:45:19, tommycli wrote:
> alignment

Done.

[tommycli, 2016-02-09 18:33:20]

lgtm sans below, assuming the other reviewers have reviewed the C++ parts.

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager.js (right):

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:57: * @param {string=}
profilePath The profile's path.
Why string=? is that a typo?

[Dan Beam, 2016-02-09 18:55:33]

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager.js (right):

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:57: * @param {string=}
profilePath The profile's path.
On 2016/02/09 18:33:20, tommycli wrote:
> Why string=? is that a typo?

that means optional

this variable should be prefixed with opt_

[Dan Beam, 2016-02-09 18:56:05]

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/compiled_resources.gyp (right):

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/compiled_resources.gyp:3: # found in
the LICENSE file.
don't add this, add a compiled_resources2.gyp instead

[Dan Beam, 2016-02-10 01:00:37]

not lgtm

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/signin_create_profile_handler.cc (right):

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:416:
DCHECK(base::GetValueAsFilePath(*path_value, supervisor_profile_path));
all of this gets compiled out in release mode

don't do this, none of the code will run in the Chrome users see

[Moe, 2016-02-10 22:28:09]

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/compiled_resources.gyp (right):

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/compiled_resources.gyp:3: # found in
the LICENSE file.
On 2016/02/09 18:56:05, Dan Beam wrote:
> don't add this, add a compiled_resources2.gyp instead

Done.

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager.js (right):

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:57: * @param {string=}
profilePath The profile's path.
On 2016/02/09 18:33:20, tommycli wrote:
> Why string=? is that a typo?

Done.

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:57: * @param {string=}
profilePath The profile's path.
On 2016/02/09 18:55:33, Dan Beam wrote:
> On 2016/02/09 18:33:20, tommycli wrote:
> > Why string=? is that a typo?
> 
> that means optional
> 
> this variable should be prefixed with opt_

This parameter isn't optional after all. I removed the "=".

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/signin_create_profile_handler.cc (right):

https://codereview.chromium.org/1630903002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:416:
DCHECK(base::GetValueAsFilePath(*path_value, supervisor_profile_path));
On 2016/02/10 01:00:37, Dan Beam wrote:
> all of this gets compiled out in release mode
> 
> don't do this, none of the code will run in the Chrome users see

Thanks for catching this Dan.

[Dan Beam, 2016-02-12 23:42:25]

i think this CL is too big

you should split it up

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/control_bar.css (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/control_bar.css:4: */
this should be on the previous line:

 * found in the LICENSE file. */

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/control_bar.html (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/control_bar.html:6: <link
rel="stylesheet" type="css" href="control_bar.css">
this format is deprecated, see "shared styles"
https://blog.polymer-project.org/announcements/2015/08/13/1.1-release/

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/control_bar.js (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/control_bar.js:16: 
nit: remove \n

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/control_bar.js:42:
chrome.send('launchGuest');
i would recommend abstracting this away so you can test this code more easily
without global, browser side-effects

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/create_profile.css (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.css:20: color: #333;
should most of these colors be using a --google or --paper var()?

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.css:21: font-family:
Roboto;
nit: font: inherit; for both of these just makes this inner scope inherit the
outer scope's rules

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.css:51: padding: 6px 4px
6px 4px;
padding: 6px 4px;

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/create_profile.html (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.html:15: <link
rel="stylesheet" type="css" href="create_profile.css">
nit: use type=css consistently between both of these lines (either both have or
both remove)

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.html:54: <div
id="messageBubble" inner-h-t-m-l="{{message_}}"></div>
w-a-t

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.js:96: detached:
function() {
do you expect detached to ever be called?

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.js:168:
[this.profileName_, this.profileIconUrl_, false,
indent off

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/profile_api.js (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_api.js:7: var signedInUser;
SignedInUser

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_api.js:10: var profileInfo;
ProfileInfo

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_api.js:33:
ProfileApi.profileCreateSuccessCallback_ = null;
why are you using globals?  this means that async tests can cause flakiness

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager.css (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.css:118: background-color:
rgb(117, 117, 117);
why are you doing this? grey hex are OK

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager.html (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.html:5: lang:language">
nit: alphabetize

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.html:67: <script
src="chrome://resources/js/cr/ui/grid.js"></script>
you should be making .html files so these can be imported

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager.js (left):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:76: * @param {string}
opt_email An optional email for signin UI.
this was correct

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager.js (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:4: */
revert to single-line comments

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:24: function Oobe() {}
why is this file named user_manager.js but the class defined in it is named
Oobe?

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:44:
document.querySelector('control-bar').showAddPerson = showAddPerson;
re-use the result of this querySelector

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:72: * @param {string=}
optEmail An optional email for signin UI.
opt_paramsLikeThis

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager_pages.js (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager_pages.js:32: 
nit:

listeners: {
  'change-page': 'changePage_',
},

changePage_: function(e) {
  this.selectedPage = e.detail.page,
},

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager_tutorial.js (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager_tutorial.js:107: 
nit: remove \n

[michaelpg, 2016-02-18 03:45:06]

Sorry to only be bringing this up now, but I reeeeally think you should
s/User/Profile everything. This is the Profile Manager, not the User Manager.
(Only Chrome OS has a concept of Users.)

Correct me if I'm wrong or if we're consciously switching from the term
"Profile" to "User", but IMO this should agree with the current codebase's
Profile/User distinction to avoid confusion.

[Moe, 2016-02-20 00:21:00]

dbeam@ I addressed your comments and asked questions on the ones I wasn't sure
about. I will follow up by breaking this down into smaller CLs.

michaelpg@ Thanks for bringing this up. In the future CLs I will go ahead and
replace instances of "user" with "profile". I think profile and user are used
interchangeably in the the "user manager" a lot b/c mlerman@ originally forked
this feature from cros.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/control_bar.css (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/control_bar.css:4: */
On 2016/02/12 23:42:24, Dan Beam wrote:
> this should be on the previous line:
> 
>  * found in the LICENSE file. */

Done.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/control_bar.html (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/control_bar.html:6: <link
rel="stylesheet" type="css" href="control_bar.css">
On 2016/02/12 23:42:24, Dan Beam wrote:
> this format is deprecated, see "shared styles"
> https://blog.polymer-project.org/announcements/2015/08/13/1.1-release/

if that's ok, I will change the format as a follow up CL. I have a few branches
based off of this at the moment. changing it will make rebasing those very
difficult and will probably introduce bugs. I will leave a comment here.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/control_bar.js (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/control_bar.js:16: 
On 2016/02/12 23:42:24, Dan Beam wrote:
> nit: remove \n

Done.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/control_bar.js:42:
chrome.send('launchGuest');
On 2016/02/12 23:42:24, Dan Beam wrote:
> i would recommend abstracting this away so you can test this code more easily
> without global, browser side-effects

Makes sense. Done!

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/create_profile.css (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.css:20: color: #333;
On 2016/02/12 23:42:24, Dan Beam wrote:
> should most of these colors be using a --google or --paper var()?

Ah! I didn't know those variables existed. Will replace those I can with
variables. Thank you.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.css:21: font-family:
Roboto;
On 2016/02/12 23:42:24, Dan Beam wrote:
> nit: font: inherit; for both of these just makes this inner scope inherit the
> outer scope's rules

Done.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.css:51: padding: 6px 4px
6px 4px;
On 2016/02/12 23:42:24, Dan Beam wrote:
> padding: 6px 4px;

Done.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/create_profile.html (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.html:15: <link
rel="stylesheet" type="css" href="create_profile.css">
On 2016/02/12 23:42:24, Dan Beam wrote:
> nit: use type=css consistently between both of these lines (either both have
or
> both remove)

Done.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.html:54: <div
id="messageBubble" inner-h-t-m-l="{{message_}}"></div>
On 2016/02/12 23:42:24, Dan Beam wrote:
> w-a-t

used to set the element's innerHTML inside an observer for |message_|. @tommycli
suggested avoiding observers if possible with this syntax being an alternative.
However I've gotten rid of this in future iterations I have pipelined based on
this CL.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.js:96: detached:
function() {
On 2016/02/12 23:42:24, Dan Beam wrote:
> do you expect detached to ever be called?

Not at the moment. "detached" seemed to be the logical place to reset the
callbacks (question: is it important to reset them?). I am aware it is not being
called b/c the element is not being removed from the DOM. Once we fork the user
pods page and make it a polymer element, I'm planning to move all pages to the
user-manager-pages and use "dom-if" to attach and detach the page elements just
like settings page. If that's an acceptable approach by you i'll leave a TODO
for that.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/create_profile.js:168:
[this.profileName_, this.profileIconUrl_, false,
On 2016/02/12 23:42:24, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/profile_api.js (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_api.js:7: var signedInUser;
On 2016/02/12 23:42:24, Dan Beam wrote:
> SignedInUser

Done.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_api.js:10: var profileInfo;
On 2016/02/12 23:42:24, Dan Beam wrote:
> ProfileInfo

Done.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_api.js:33:
ProfileApi.profileCreateSuccessCallback_ = null;
On 2016/02/12 23:42:24, Dan Beam wrote:
> why are you using globals?  this means that async tests can cause flakiness

I followed the pattern used in sync_private_api.js. What would you recommend to
use instead?

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager.css (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.css:118: background-color:
rgb(117, 117, 117);
On 2016/02/12 23:42:24, Dan Beam wrote:
> why are you doing this? grey hex are OK
tommycli@ suggested colors that need more than 3 digits should use rgb. I'll
keep your comment in mind for the future.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager.html (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.html:5: lang:language">
On 2016/02/12 23:42:24, Dan Beam wrote:
> nit: alphabetize

Done.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.html:67: <script
src="chrome://resources/js/cr/ui/grid.js"></script>
On 2016/02/12 23:42:24, Dan Beam wrote:
> you should be making .html files so these can be imported

I haven't created this file (i modified it heavily though). I've been trying to
keep the changes to this file minimal to avoid breaking things and not making
the CL larger than this. Most of these resources will likely go away once we
fork the user pods from cros and redo them in Polymer.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager.js (left):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:76: * @param {string}
opt_email An optional email for signin UI.
On 2016/02/12 23:42:24, Dan Beam wrote:
> this was correct

Done.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager.js (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:4: */
On 2016/02/12 23:42:24, Dan Beam wrote:
> revert to single-line comments

Done.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:24: function Oobe() {}
On 2016/02/12 23:42:24, Dan Beam wrote:
> why is this file named user_manager.js but the class defined in it is named
> Oobe?

It looks like "user manager" was originally implemented taking advantage of a
lot of cros resources. I have been trying to keep the changes to this file
minimal to avoid breaking things. There will be follow ups in future to
re-implement the user pods and this page and divert from the cros.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:44:
document.querySelector('control-bar').showAddPerson = showAddPerson;
On 2016/02/12 23:42:24, Dan Beam wrote:
> re-use the result of this querySelector

Done.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:72: * @param {string=}
optEmail An optional email for signin UI.
On 2016/02/12 23:42:24, Dan Beam wrote:
> opt_paramsLikeThis

Done.

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager_pages.js (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager_pages.js:32: 
On 2016/02/12 23:42:24, Dan Beam wrote:
> nit:
> 
> listeners: {
>   'change-page': 'changePage_',
> },
> 
> changePage_: function(e) {
>   this.selectedPage = e.detail.page,
> },

I struggled a lot with this syntax originally. it didn't worked. am I missing
something? Could it be that the event is not coming from the local DOM of the
element?

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/user_manager_tutorial.js (right):

https://codereview.chromium.org/1630903002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager_tutorial.js:107: 
On 2016/02/12 23:42:24, Dan Beam wrote:
> nit: remove \n

Done.