updated UI, default profile name, check for existing supervised user before create

A follow up CL for MD Use Manager. It reflects the latest mocks, the default profile name, and checking for existing supervised users before creating one with the same name.

BUG=563722
Committed: https://crrev.com/1d0bb14e8121201dd393eef0f43834327702815c
Cr-Commit-Position: refs/heads/master@{#385787}

[Moe, 2016-03-23 20:18:21]

Description was changed from

==========
updated UI, default profile name, check for existing supervised user before
create

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

to

==========
A follow up CL for MD Use Manager. It reflects the latest mocks, the default
profile name, and checking for existing supervised users before creating one
with the same name.

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

[Moe, 2016-03-23 20:18:22]

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

[Moe, 2016-03-23 20:18:53]

Hi Roger and Tommy, Please review my changes for the MD User Manager.

[Roger Tawa OOO till Jul 10th, 2016-03-24 14:39:05]

I looked mostly at the C++ code.  A few questions below.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:183: * user. If yes,
the user is prompted to import the existing supervised user.
Remove "user."

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_create_profile_handler.cc (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:549: state ==
GoogleServiceAuthError::ACCOUNT_DISABLED;
Is there a specific reason to handle these 4 states?  Is it possible to use
GoogleServiceAuthError::IsPersistentError() instead?

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_supervised_user_import_handler.cc
(right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_supervised_user_import_handler.cc:69:
&supervisor_profile_path);
Indent 3 more.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_supervised_user_import_handler.cc:123:
break;
It seems like some code paths will not invoke "cr.webUIResponse".  Like these
cases here and CREATE_STATUS_LOCAL_FAIL.  Maybe these errors are handled
separately?

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_supervised_user_import_handler.cc:134:
Profile* profile, const base::DictionaryValue* dict) {
Nit: put |dict| on next line.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_supervised_user_import_handler.cc:218:
state == GoogleServiceAuthError::ACCOUNT_DISABLED;
Same comment as earlier.

[tommycli, 2016-03-24 19:28:51]

I mostly looked at the JS/HTML/CSS

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.css (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.css:18: position:
absolute;
Does this have to be absolute? Is it floating above the other contents?

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.html (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:53:
selected="{{signedInUserIndex_}}" attr-for-selected="index">
Can we change 'index' to 'data-user-index' so no one confuses it with a Polymer
attribute?

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:54: <paper-item
index="-1">Select an account</paper-item>
I hate sentinel values, but in this case, I think this makes sense.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:87:
signedInUserIndex_: {
Just add a comment here that -1 means no user is selected

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:165: var signedInUser
= this.signedInUsers_[this.signedInUserIndex_];
Is there anything to prevent Save from occuring if the -1 sentinel value is
selected? Also there should be an assert here

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:227: var signedInUser
= this.signedInUsers_[this.signedInUserIndex_];
Same here. How do we prevent a -1 signedInUserIndex_? Can we add an assert?

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:311: isSaveDisabled_:
function(createInProgress, profileName) {
How is this disabled if the selectedUserIndex = -1?

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_create_profile_handler.h (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_create_profile_handler.h:28: class
SigninCreateProfileHandler : public content::WebUIMessageHandler {
This should have a unit test. (Can be added in a separate CL)

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

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_supervised_user_import_handler.h
(right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_supervised_user_import_handler.h:24: class
SigninSupervisedUserImportHandler : public content::WebUIMessageHandler {
This should also have a unit test. It should be added in this CL (since this is
new code):
Example:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[Pam (message me for reviews), 2016-03-24 19:53:13]

pam@chromium.org changed reviewers:
+ atanasova@chromium.org, pam@chromium.org

[Pam (message me for reviews), 2016-03-24 19:53:14]

+atanasova, who dealt with some SU name-duplication issues recently

Can you point me to screenshots or mocks, especially of what the Supervised User
creation and import UI will look like?

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_create_profile_handler.cc (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:98:
IDS_PROFILES_CREATE_SUPERVISOR_ACCOUNT_DETAILS_OUT_OF_DATE_ERROR));
Existing bugs and docs and other code refer to the person that creates a
supervised user as the "custodian" (rather than "supervisor") -- so this CL
should too, for the sake of consistency.

[Moe, 2016-03-24 22:07:57]

Hi, Pam here are the Mocks:

https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.css (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.css:18: position:
absolute;
On 2016/03/24 19:28:50, tommycli wrote:
> Does this have to be absolute? Is it floating above the other contents?

message-container should be fixed to the top of the window. and it should not
push the content down when it is displayed. I can use 'position: fixed' instead.
Are there any disadvantages in using absolute positioning here?

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.html (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:53:
selected="{{signedInUserIndex_}}" attr-for-selected="index">
On 2016/03/24 19:28:50, tommycli wrote:
> Can we change 'index' to 'data-user-index' so no one confuses it with a
Polymer
> attribute?

Done.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:54: <paper-item
index="-1">Select an account</paper-item>
On 2016/03/24 19:28:50, tommycli wrote:
> I hate sentinel values, but in this case, I think this makes sense.

yes I agree. I couldn't go with a Label here. This is a menu item in the mocks.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:87:
signedInUserIndex_: {
On 2016/03/24 19:28:51, tommycli wrote:
> Just add a comment here that -1 means no user is selected

Done.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:165: var signedInUser
= this.signedInUsers_[this.signedInUserIndex_];
On 2016/03/24 19:28:50, tommycli wrote:
> Is there anything to prevent Save from occuring if the -1 sentinel value is
> selected? Also there should be an assert here

If the new profile is not marked to be supervised, the -1 sentinel value could
be selected. if the checkbox is checked though, here we make sure a signed-in
user is in fact selected (it is not undefined)

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:183: * user. If yes,
the user is prompted to import the existing supervised user.
On 2016/03/24 14:39:05, Roger Tawa wrote:
> Remove "user."

Done.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:227: var signedInUser
= this.signedInUsers_[this.signedInUserIndex_];
On 2016/03/24 19:28:50, tommycli wrote:
> Same here. How do we prevent a -1 signedInUserIndex_? Can we add an assert?

Same here. We don't need to prevent it when the new profile is not supervised
(we check for it not to be undefined here). If it is supervised though, we've
already made sure a singedin user is selected in onSaveTap_().

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:311: isSaveDisabled_:
function(createInProgress, profileName) {
On 2016/03/24 19:28:50, tommycli wrote:
> How is this disabled if the selectedUserIndex = -1?

The button is not disabled in that case by design. instead user receives a an
error message asking them to select a user.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_create_profile_handler.cc (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:98:
IDS_PROFILES_CREATE_SUPERVISOR_ACCOUNT_DETAILS_OUT_OF_DATE_ERROR));
On 2016/03/24 19:53:14, Pam (also PM for reviews) wrote:
> Existing bugs and docs and other code refer to the person that creates a
> supervised user as the "custodian" (rather than "supervisor") -- so this CL
> should too, for the sake of consistency.

Done.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_create_profile_handler.cc:549: state ==
GoogleServiceAuthError::ACCOUNT_DISABLED;
On 2016/03/24 14:39:05, Roger Tawa wrote:
> Is there a specific reason to handle these 4 states?  Is it possible to use
> GoogleServiceAuthError::IsPersistentError() instead?

No specific reason. These 4 states were being checked for in
c/b/ui/webui/options/supervised_user_import_handler.cc

As per our discussion offline, I changed this to check for any error state.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_create_profile_handler.h (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_create_profile_handler.h:28: class
SigninCreateProfileHandler : public content::WebUIMessageHandler {
On 2016/03/24 19:28:51, tommycli wrote:
> This should have a unit test. (Can be added in a separate CL)
> 
> See
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> for an example.

Absolutely. I will be working on those in the meantime.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_supervised_user_import_handler.cc
(right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_supervised_user_import_handler.cc:69:
&supervisor_profile_path);
On 2016/03/24 14:39:05, Roger Tawa wrote:
> Indent 3 more.

Done.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_supervised_user_import_handler.cc:123:
break;
On 2016/03/24 14:39:05, Roger Tawa wrote:
> It seems like some code paths will not invoke "cr.webUIResponse".  Like these
> cases here and CREATE_STATUS_LOCAL_FAIL.  Maybe these errors are handled
> separately?

Added error handling for the CREATE_STATUS_LOCAL_FAIL state. As for the three
last states, I don't believe we should get there. Other instances in the code
base also don't seems to handle these states.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_supervised_user_import_handler.cc:134:
Profile* profile, const base::DictionaryValue* dict) {
On 2016/03/24 14:39:05, Roger Tawa wrote:
> Nit: put |dict| on next line.

Done.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_supervised_user_import_handler.cc:218:
state == GoogleServiceAuthError::ACCOUNT_DISABLED;
On 2016/03/24 14:39:05, Roger Tawa wrote:
> Same comment as earlier.

Done.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
File chrome/browser/ui/webui/signin/signin_supervised_user_import_handler.h
(right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/ui/webui/sig...
chrome/browser/ui/webui/signin/signin_supervised_user_import_handler.h:24: class
SigninSupervisedUserImportHandler : public content::WebUIMessageHandler {
On 2016/03/24 19:28:51, tommycli wrote:
> This should also have a unit test. It should be added in this CL (since this
is
> new code):
> Example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Will work on tests for this and signin_create_profile_handler next week.

[tommycli, 2016-03-24 22:18:54]

cool the HTML and CSS are pretty much LG by me assuming we resolve the issue
regarding the sentinel value (detailed in the below comments)

See the code here for an example of how you can bind to constants:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.css (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.css:18: position:
absolute;
On 2016/03/24 22:07:56, Moe wrote:
> On 2016/03/24 19:28:50, tommycli wrote:
> > Does this have to be absolute? Is it floating above the other contents?
> 
> message-container should be fixed to the top of the window. and it should not
> push the content down when it is displayed. I can use 'position: fixed'
instead.
> Are there any disadvantages in using absolute positioning here?

In that case absolute is fine. (Don't use fixed)

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:87:
signedInUserIndex_: {
On 2016/03/24 22:07:56, Moe wrote:
> On 2016/03/24 19:28:51, tommycli wrote:
> > Just add a comment here that -1 means no user is selected
> 
> Done.

Can we make this a constant? NO_USER_SELECTED or something?

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:165: var signedInUser
= this.signedInUsers_[this.signedInUserIndex_];
On 2016/03/24 22:07:57, Moe wrote:
> On 2016/03/24 19:28:50, tommycli wrote:
> > Is there anything to prevent Save from occuring if the -1 sentinel value is
> > selected? Also there should be an assert here
> 
> If the new profile is not marked to be supervised, the -1 sentinel value could
> be selected. if the checkbox is checked though, here we make sure a signed-in
> user is in fact selected (it is not undefined)

Relying on undefined was too subtle for me to understand on first read. Can we
specifically check against -1? And can we make -1 a constant? NO_USER_SELECTED
or something?

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:227: var signedInUser
= this.signedInUsers_[this.signedInUserIndex_];
On 2016/03/24 22:07:56, Moe wrote:
> On 2016/03/24 19:28:50, tommycli wrote:
> > Same here. How do we prevent a -1 signedInUserIndex_? Can we add an assert?
> 
> Same here. We don't need to prevent it when the new profile is not supervised
> (we check for it not to be undefined here). If it is supervised though, we've
> already made sure a singedin user is selected in onSaveTap_().

Same comment as above, explicitly checking vs. the sentinal value (that's in a
constant)

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

https://codereview.chromium.org/1826903002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:55: <paper-item
data-user-index="-1">
This can be bound to [[Constants.NO_USER_SELECTED]] if you add 

created: function() {
  this.Constants = Constants;
}

(and define Constants above the polymer element)

[atanasova, 2016-03-29 07:53:17]

I looked at the mocks and don't see a few user cases related to Supervised Users
(SUs), but I see some logic in the code, so can you please provide some
screenshots for these:

 - If the name you entered is already a SU, where would the error appear? Also
how does the link look like (the import this user link)
 - In case of an existing name, is the "Save" button disabled?
 - In the SUs list dialog, is there a reason why the accounts that are already
on the device are not disabled (at least that is how it looks in the mocks). In
the current version those on the device are still shown, but are grayed out and
are not clickable.

[Moe, 2016-03-29 19:27:05]

On 2016/03/29 07:53:17, atanasova wrote:
> I looked at the mocks and don't see a few user cases related to Supervised
Users
> (SUs), but I see some logic in the code, so can you please provide some
> screenshots for these:
> 
>  - If the name you entered is already a SU, where would the error appear? Also
> how does the link look like (the import this user link)
>  - In case of an existing name, is the "Save" button disabled?
>  - In the SUs list dialog, is there a reason why the accounts that are already
> on the device are not disabled (at least that is how it looks in the mocks).
In
> the current version those on the device are still shown, but are grayed out
and
> are not clickable.

Hi Maria, the mocks are broken down into multiple folders. You should be able to
find the ones you are referring to under spec/

https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...

Not having the accounts that are already on the device greyed out was a design
decision by the designers. However those should still not be selectable.
In case of an existing name (or any other error for that matter) the "Save"
button is not disabled by design. The error message, unlike the behavior today
which checks for an existing name proactively, appears once the user clicks
"Save". Clicking "Save" again essentially does nothing. Let me know if you need
more details.

[Moe, 2016-03-29 19:27:53]

On 2016/03/29 19:27:05, Moe wrote:
> On 2016/03/29 07:53:17, atanasova wrote:
> > I looked at the mocks and don't see a few user cases related to Supervised
> Users
> > (SUs), but I see some logic in the code, so can you please provide some
> > screenshots for these:
> > 
> >  - If the name you entered is already a SU, where would the error appear?
Also
> > how does the link look like (the import this user link)
> >  - In case of an existing name, is the "Save" button disabled?
> >  - In the SUs list dialog, is there a reason why the accounts that are
already
> > on the device are not disabled (at least that is how it looks in the mocks).
> In
> > the current version those on the device are still shown, but are grayed out
> and
> > are not clickable.
> 
> Hi Maria, the mocks are broken down into multiple folders. You should be able
to
> find the ones you are referring to under spec/
> 
>
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
> 
> Not having the accounts that are already on the device greyed out is by
design. However those should still not be selectable.
> In case of an existing name (or any other error for that matter) the "Save"
> button is not disabled by design. The error message, unlike the behavior today
> which checks for an existing name proactively, appears once the user clicks
> "Save". Clicking "Save" again essentially does nothing. Let me know if you
need
> more details.

[Moe, 2016-03-29 20:51:16]

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
File chrome/browser/resources/md_user_manager/create_profile.js (right):

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:87:
signedInUserIndex_: {
On 2016/03/24 22:18:54, tommycli (OOO Mar 28 - Apr 1) wrote:
> On 2016/03/24 22:07:56, Moe wrote:
> > On 2016/03/24 19:28:51, tommycli wrote:
> > > Just add a comment here that -1 means no user is selected
> > 
> > Done.
> 
> Can we make this a constant? NO_USER_SELECTED or something?

Done.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:165: var signedInUser
= this.signedInUsers_[this.signedInUserIndex_];
On 2016/03/24 22:18:54, tommycli (OOO Mar 28 - Apr 1) wrote:
> On 2016/03/24 22:07:57, Moe wrote:
> > On 2016/03/24 19:28:50, tommycli wrote:
> > > Is there anything to prevent Save from occuring if the -1 sentinel value
is
> > > selected? Also there should be an assert here
> > 
> > If the new profile is not marked to be supervised, the -1 sentinel value
could
> > be selected. if the checkbox is checked though, here we make sure a
signed-in
> > user is in fact selected (it is not undefined)
> 
> Relying on undefined was too subtle for me to understand on first read. Can we
> specifically check against -1? And can we make -1 a constant? NO_USER_SELECTED
> or something?

Done.

https://codereview.chromium.org/1826903002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:227: var signedInUser
= this.signedInUsers_[this.signedInUserIndex_];
On 2016/03/24 22:18:54, tommycli (OOO Mar 28 - Apr 1) wrote:
> On 2016/03/24 22:07:56, Moe wrote:
> > On 2016/03/24 19:28:50, tommycli wrote:
> > > Same here. How do we prevent a -1 signedInUserIndex_? Can we add an
assert?
> > 
> > Same here. We don't need to prevent it when the new profile is not
supervised
> > (we check for it not to be undefined here). If it is supervised though,
we've
> > already made sure a singedin user is selected in onSaveTap_().
> 
> Same comment as above, explicitly checking vs. the sentinal value (that's in a
> constant)

Done.

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

https://codereview.chromium.org/1826903002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:55: <paper-item
data-user-index="-1">
On 2016/03/24 22:18:54, tommycli (OOO Mar 28 - Apr 1) wrote:
> This can be bound to [[Constants.NO_USER_SELECTED]] if you add 
> 
> created: function() {
>   this.Constants = Constants;
> }
> 
> (and define Constants above the polymer element)

Done.

[Roger Tawa OOO till Jul 10th, 2016-04-04 13:46:44]

lgtm

[tommycli, 2016-04-04 16:48:45]

lgtm except that signin_supervised_user_import_handler still needs a unit test.
If you're going to add that in the very next CL I guess that's fine too.

[Moe, 2016-04-04 23:00:15]

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

[Moe, 2016-04-04 23:00:16]

Thank you Tommy!

I updated the browser tests in the last patch.

Next CL will be unittests for signin_supervised_user_import_handler and
signin_create_profile_handler

Also, +dbeam

[Dan Beam, 2016-04-05 23:38:40]

couldn't get to all of this today. write smaller patches ;)

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

https://codereview.chromium.org/1826903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.css:40: padding: 104px 0
16px 0;
nit: padding: 104px 0 16px;

https://codereview.chromium.org/1826903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.css:40: padding: 104px 0
16px 0;
why 104px?

[Dan Beam, 2016-04-06 06:45:19]

lgtm

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

https://codereview.chromium.org/1826903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:162: return
this.signedInUsers_[signedInUserIndex];
nit: can't this technically return undefined as well?

https://codereview.chromium.org/1826903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:342: * @param
{!Array<SignedInUser>} signedInUsers signed-in users.
Nit: !Array<!SignedInUser>

[atanasova, 2016-04-07 14:18:51]

lgtm

[Moe, 2016-04-07 14:36:40]

Thank you!

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

https://codereview.chromium.org/1826903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.css:40: padding: 104px 0
16px 0;
On 2016/04/05 23:38:40, Dan Beam wrote:
> nit: padding: 104px 0 16px;

Done.

https://codereview.chromium.org/1826903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.css:40: padding: 104px 0
16px 0;
On 2016/04/05 23:38:40, Dan Beam wrote:
> why 104px?

The spec asks for it:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...

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

https://codereview.chromium.org/1826903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:162: return
this.signedInUsers_[signedInUserIndex];
On 2016/04/06 06:45:19, Dan Beam wrote:
> nit: can't this technically return undefined as well?

Done.

https://codereview.chromium.org/1826903002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:342: * @param
{!Array<SignedInUser>} signedInUsers signed-in users.
On 2016/04/06 06:45:19, Dan Beam wrote:
> Nit: !Array<!SignedInUser>

Done.

[Moe, 2016-04-07 16:45:21]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-04-07 16:45:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org, rogerta@chromium.org, atanasova@chromium.org,
dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1826903002/#ps80001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-04-07 16:45:41]

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

[commit-bot: I haz the power, 2016-04-07 16:52:01]

Description was changed from

==========
A follow up CL for MD Use Manager. It reflects the latest mocks, the default
profile name, and checking for existing supervised users before creating one
with the same name.

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

to

==========
A follow up CL for MD Use Manager. It reflects the latest mocks, the default
profile name, and checking for existing supervised users before creating one
with the same name.

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

[commit-bot: I haz the power, 2016-04-07 16:52:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-07 16:53:17]

Description was changed from

==========
A follow up CL for MD Use Manager. It reflects the latest mocks, the default
profile name, and checking for existing supervised users before creating one
with the same name.

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

to

==========
A follow up CL for MD Use Manager. It reflects the latest mocks, the default
profile name, and checking for existing supervised users before creating one
with the same name.

BUG=563722

Committed: https://crrev.com/1d0bb14e8121201dd393eef0f43834327702815c
Cr-Commit-Position: refs/heads/master@{#385787}
==========

[commit-bot: I haz the power, 2016-04-07 16:53:18]

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

[lwchkg, 2016-04-07 18:04:50]

On 2016/04/07 16:53:18, commit-bot: I haz the power wrote:
> Patchset 5 (id:??) landed as
> https://crrev.com/1d0bb14e8121201dd393eef0f43834327702815c
> Cr-Commit-Position: refs/heads/master@{#385787}

Sorry to step in after a closed review.

Just to notify that ProfileInfoCache is in the process of being refactored out.
Future uses of the profile info cache (or it's new name, profile attributes
storage) should be done in something like below:

ProfileAttributesStorage& storage =
g_browser_process->profile_manager()->GetProfileAttributesStorage();
base::DictionaryValue profile_info;
profile_info.SetString("name", storage.ChooseNameForNewProfile(0));

If you have any enquiries about the refactoring, please feel free to contact me.
(Or you can contact anthonyvd@.)

Anyway, I'll do the refactoring of signin_create_profile_handler.cc for you this
time, because one of my CQs needs the file refactored to compile correctly.
(FYI, that CQ has a side effect of removing implicit (i.e. non-IWYU) references
to profile_info_cache.h)

Regards,
WC Leung.