Import supervised user dialog

Import supervised user dialog

This dialog is displayed when the user clicks on the 'import supervised users' link from the 'create user' page on the user manager screen. Or when the users attempts to create a supervised user that already exists and is prompted to import the user instead.

browser tests for this patch will follow in another CL...

BUG=563722, 605899, 605481
Committed: https://crrev.com/53539eb613fc973531c9a40f73eee83f4f65ddaa
Cr-Commit-Position: refs/heads/master@{#389187}

[Moe, 2016-04-18 20:06:00]

Description was changed from

==========
Import supervised user dialog

BUG=
==========

to

==========
Import supervised user dialog

This dialog is displayed when the user clicks on the 'import supervised users'
link from the 'create user' page on the user manager screen. Or when the users
attempts to create a supervised user that already exists and is prompted to
import the user instead.

browser tests for this patch will follow in another CL...

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

[Moe, 2016-04-18 20:06:00]

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

[Moe, 2016-04-18 20:08:12]

On 2016/04/18 20:06:00, Moe wrote:
> mailto:mahmadi@chromium.org changed reviewers:
> + mailto:rogerta@chromium.org, mailto:tommycli@chromium.org

Hi Roger and Tommy, Please review these changes:

c/b/browser_resources.grd tommycli@
c/b/resources tommycli@
c/b/ui/webui/signin/ @rogerta
c/t/data/webui tommycli@

[Moe, 2016-04-18 20:08:12]

On 2016/04/18 20:06:00, Moe wrote:
> mailto:mahmadi@chromium.org changed reviewers:
> + mailto:rogerta@chromium.org, mailto:tommycli@chromium.org

Hi Roger and Tommy, Please review these changes:

c/b/browser_resources.grd tommycli@
c/b/resources tommycli@
c/b/ui/webui/signin/ @rogerta
c/t/data/webui tommycli@

[Pam (message me for reviews), 2016-04-18 20:20:42]

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

[tommycli, 2016-04-18 23:27:18]

I didn't look at the C++. Thanks!

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

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:20: <link
rel="import" type="css"
External stylesheets deprecated. But since this already exists, the fix should
probably be in a separate CL.

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

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:102: 'tap': 'onTap_'
This is leftover right?

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:138: onTap_:
function(event) {
This is also a leftover right? I don't see any dynamically created <a> elements.

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:313: * Handler for
the 'import-supervised-user' element's import event.
Maybe update the comment to elucidate more that this is what happens after the
importUserPopup finishes.

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:418: return
createInProgress || !this.signedInUser_(signedInUserIndex);
Interesting... the link is really hidden as soon as the user clicks it? The
mocks specify that?

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

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/import_supervised_user.html:12:
href="chrome://md-user-manager/import_supervised_user.css">
External stylesheets deprecated.

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/import_supervised_user.html:22: <div
id="message">[[message_]]</div>
Instead of a property, we can use a computed binding for message_

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

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/import_supervised_user.js:28: hidden_:
{
Maybe clarify the var name and comment to indicate that this is a dialog element
that hides and shows itself.

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/import_supervised_user.js:39: value:
function() { return null; },
nit: all of the final values have no comma, except this one. Either style is
fine (I personally prefer trailing commas), just consistent is better.

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/import_supervised_user.js:92:
sortSupervisedUsers_: function(supervisedUsers) {
Since this method is only used once and is private, we can just inline the
sorting functionality within 'show'

[Moe, 2016-04-19 21:47:43]

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

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:20: <link
rel="import" type="css"
On 2016/04/18 23:27:18, tommycli wrote:
> External stylesheets deprecated. But since this already exists, the fix should
> probably be in a separate CL.

Sure. will send out a follow-up CL fixing all these. I already had a todo to
address this.

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

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:102: 'tap': 'onTap_'
On 2016/04/18 23:27:18, tommycli wrote:
> This is leftover right?

No it is not. Unfortunately, this is another i18n messages with links issue. We
have at least two error messages with link in them. This handler is meant to
handle tap events on those. 
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Desktop%20S...

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:138: onTap_:
function(event) {
On 2016/04/18 23:27:18, tommycli wrote:
> This is also a leftover right? I don't see any dynamically created <a>
elements.

Please see the previous response.

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:313: * Handler for
the 'import-supervised-user' element's import event.
On 2016/04/18 23:27:18, tommycli wrote:
> Maybe update the comment to elucidate more that this is what happens after the
> importUserPopup finishes.

Done.

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:418: return
createInProgress || !this.signedInUser_(signedInUserIndex);
On 2016/04/18 23:27:18, tommycli wrote:
> Interesting... the link is really hidden as soon as the user clicks it? The
> mocks specify that?

I checked with UX. The same thing as the create scenario should happen here.
Loading the custodian account could take some time. In meantime, we disable the
"Save" button and hide "Import an existing supervised user" link.

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

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/import_supervised_user.html:12:
href="chrome://md-user-manager/import_supervised_user.css">
On 2016/04/18 23:27:18, tommycli wrote:
> External stylesheets deprecated.

Done.

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/import_supervised_user.html:22: <div
id="message">[[message_]]</div>
On 2016/04/18 23:27:18, tommycli wrote:
> Instead of a property, we can use a computed binding for message_

Done.

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

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/import_supervised_user.js:28: hidden_:
{
On 2016/04/18 23:27:18, tommycli wrote:
> Maybe clarify the var name and comment to indicate that this is a dialog
element
> that hides and shows itself.

Done.

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/import_supervised_user.js:39: value:
function() { return null; },
On 2016/04/18 23:27:18, tommycli wrote:
> nit: all of the final values have no comma, except this one. Either style is
> fine (I personally prefer trailing commas), just consistent is better.

Done.

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/import_supervised_user.js:92:
sortSupervisedUsers_: function(supervisedUsers) {
On 2016/04/18 23:27:18, tommycli wrote:
> Since this method is only used once and is private, we can just inline the
> sorting functionality within 'show'

Done.

[tommycli, 2016-04-19 23:33:37]

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

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:138: onTap_:
function(event) {
On 2016/04/19 21:47:42, Moe wrote:
> On 2016/04/18 23:27:18, tommycli wrote:
> > This is also a leftover right? I don't see any dynamically created <a>
> elements.
> 
> Please see the previous response.

This is unfortunate but ... -- this is pretty dirty but I don't have a concrete
alternative suggestion.

Just remember to preventDefault on the event then.

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

https://codereview.chromium.org/1901853002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:103:
'importUserPopup.import': 'onImportUserPopupImport_'
This is pretty awesome. Thanks for the tip!

[tommycli, 2016-04-19 23:39:57]

lgtm with these nits and the above message too

https://codereview.chromium.org/1901853002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/import_supervised_user.js (right):

https://codereview.chromium.org/1901853002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/import_supervised_user.js:89: message_:
function(supervisedUsers) {
getMessage_ ?

https://codereview.chromium.org/1901853002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/import_supervised_user.js:101:
userClassNames_: function(supervisedUser) {
getUserClassNames_ ?

[Dan Beam, 2016-04-19 23:41:58]

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

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:102: 'tap': 'onTap_'
bummer, you can't do supervised-user-import-existing.tap because the element is
dynamically added :(

https://www.polymer-project.org/1.0/docs/devguide/events.html#event-listeners

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:138: onTap_:
function(event) {
On 2016/04/19 23:33:37, tommycli wrote:
> On 2016/04/19 21:47:42, Moe wrote:
> > On 2016/04/18 23:27:18, tommycli wrote:
> > > This is also a leftover right? I don't see any dynamically created <a>
> > elements.
> > 
> > Please see the previous response.
> 
> This is unfortunate but ... -- this is pretty dirty but I don't have a
concrete
> alternative suggestion.
> 
> Just remember to preventDefault on the event then.

this is not dirty, it's event delegation

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:142: if
(element.nodeName == 'A') {
you should never need to filter by nodeName if you have an ID

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:143: if (element.id
== 'supervised-user-import-existing') {
no curlies

[atanasova, 2016-04-20 09:50:23]

lgtm

[Roger Tawa OOO till Jul 10th, 2016-04-20 15:14:37]

lgtm

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

https://codereview.chromium.org/1901853002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/md_user_manager_ui.cc:118: #endif
For my own info, why is this the only place in the CL where supervised user code
is #ifdef'ed?

[Moe, 2016-04-20 15:28:55]

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

[Moe, 2016-04-20 15:28:56]

+dbeam@

Dan, Could you please have a look at the following? Thank you.

c/b/browser_resources.grd
c/b/resources
c/t/data/webui

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

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:102: 'tap': 'onTap_'
On 2016/04/19 23:41:57, Dan Beam wrote:
> bummer, you can't do supervised-user-import-existing.tap because the element
is
> dynamically added :(
> 
> https://www.polymer-project.org/1.0/docs/devguide/events.html#event-listeners

totally!

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:138: onTap_:
function(event) {
On 2016/04/19 23:33:37, tommycli wrote:
> On 2016/04/19 21:47:42, Moe wrote:
> > On 2016/04/18 23:27:18, tommycli wrote:
> > > This is also a leftover right? I don't see any dynamically created <a>
> > elements.
> > 
> > Please see the previous response.
> 
> This is unfortunate but ... -- this is pretty dirty but I don't have a
concrete
> alternative suggestion.
> 
> Just remember to preventDefault on the event then.

Done.

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:142: if
(element.nodeName == 'A') {
On 2016/04/19 23:41:57, Dan Beam wrote:
> you should never need to filter by nodeName if you have an ID

Done.

https://codereview.chromium.org/1901853002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:143: if (element.id
== 'supervised-user-import-existing') {
On 2016/04/19 23:41:57, Dan Beam wrote:
> no curlies

Done.

https://codereview.chromium.org/1901853002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_user_manager/import_supervised_user.js (right):

https://codereview.chromium.org/1901853002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/import_supervised_user.js:89: message_:
function(supervisedUsers) {
On 2016/04/19 23:39:57, tommycli wrote:
> getMessage_ ?

Done.

https://codereview.chromium.org/1901853002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/import_supervised_user.js:101:
userClassNames_: function(supervisedUser) {
On 2016/04/19 23:39:57, tommycli wrote:
> getUserClassNames_ ?

Done.

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

https://codereview.chromium.org/1901853002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/md_user_manager_ui.cc:118: #endif
On 2016/04/20 15:14:36, Roger Tawa wrote:
> For my own info, why is this the only place in the CL where supervised user
code
> is #ifdef'ed?

Here isn't exactly the only place. in signin_create_profile_handler.*,
supervised user related functions and variables are also #ifdef'ed. But since
signin_supervised_user_import_handler_ is entirely supervised user related we
just #ifdef around its usages. And here's the only place where it's used.

[tommycli, 2016-04-20 18:58:56]

one more nit also

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

https://codereview.chromium.org/1901853002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:134: * General
handler for tap event on the host, used to handle tap events for
In the comment, it would be helpful to call out specifically which i18n strings
contain the embedded links (for readers like me)

[Moe, 2016-04-20 20:10:31]

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

https://codereview.chromium.org/1901853002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:134: * General
handler for tap event on the host, used to handle tap events for
On 2016/04/20 18:58:56, tommycli wrote:
> In the comment, it would be helpful to call out specifically which i18n
strings
> contain the embedded links (for readers like me)

Makes sense. Done.

[Dan Beam, 2016-04-20 22:38:56]

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

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:69:
hidden="[[isImportUserLinkHidden_(createInProgress_, signedInUserIndex_)]]">
arguably, now that \n are allowed in computed bindings, you should be doing
something like:

   hidden="[[isImportUserLinkHidden_(createInProgress_,
                                     signedInUserIndex_)]]"

or something

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

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:206:
this.createInProgress_ = false;
it's odd that createInProgress_ is set to true then (possibly) immediately to
false

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:213: function(error)
{ this.handleMessage_(error); }.bind(this));
can we just update handleMessage_ to take an {Error|string} (or {*}) so this
code (and below) could be:

  this.browserProxy_.getExistingSupervisedUsers(signedInUser.profilePath).then(
      this.showImportSupervisedUserPopup_.bind(this),
      this.handleMessage_.bind(this));

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:315: signedInUser:
!SignedInUser}}} event
nit: this row needs a * at the start

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

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/import_supervised_user.html:48:
padding: 20px 16px 16px 16px;
nit: padding: 20px 16px 16px;

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/import_supervised_user.html:66:
padding-left: 10px;
RTL?

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/import_supervised_user.html:73:
padding-left: 10px;
RTL?

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

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/import_supervised_user.js:68:
this.supervisedUsers_ = supervisedUsers || [];
empty arrays are truthy in JS, so the "|| []" will never trigger.

null is not truthy.  can |supervisedUsers| explicitly be |null|?  if so, you
should probably make this type:

  {?Array<!SupervisedUsers>}

alternatively, is this an optional parameter?  i.e.

  * @param {!Array<!SupervisedUser>=} opt_supervisedUsers

which means show could be called with only 1 argument (and |supervisedUsers|
omitted and === undefined)

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/import_supervised_user.js:103: if
(!supervisedUser.onCurrentDevice) {
no curlies

[Moe, 2016-04-21 17:32:30]

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

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:69:
hidden="[[isImportUserLinkHidden_(createInProgress_, signedInUserIndex_)]]">
On 2016/04/20 22:38:55, Dan Beam wrote:
> arguably, now that \n are allowed in computed bindings, you should be doing
> something like:
> 
>    hidden="[[isImportUserLinkHidden_(createInProgress_,
>                                      signedInUserIndex_)]]"
> 
> or something

Done.

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

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:206:
this.createInProgress_ = false;
On 2016/04/20 22:38:55, Dan Beam wrote:
> it's odd that createInProgress_ is set to true then (possibly) immediately to
> false

It is indeed. I moved all instances of "this.createInProgress_ = true;" right
next to the appropriate this.browserProxy_ calls to avoid cases like this.

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:213: function(error)
{ this.handleMessage_(error); }.bind(this));
On 2016/04/20 22:38:55, Dan Beam wrote:
> can we just update handleMessage_ to take an {Error|string} (or {*}) so this
> code (and below) could be:
> 
>  
this.browserProxy_.getExistingSupervisedUsers(signedInUser.profilePath).then(
>       this.showImportSupervisedUserPopup_.bind(this),
>       this.handleMessage_.bind(this));

Done. The catch is there'll be type cast needed here:

this.message_ =  /** @type {string} */ (message);

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:315: signedInUser:
!SignedInUser}}} event
On 2016/04/20 22:38:55, Dan Beam wrote:
> nit: this row needs a * at the start

Done.

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

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/import_supervised_user.html:48:
padding: 20px 16px 16px 16px;
On 2016/04/20 22:38:55, Dan Beam wrote:
> nit: padding: 20px 16px 16px;

I didn't know about the three value shorthand. Thanks for the tip!

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/import_supervised_user.html:66:
padding-left: 10px;
On 2016/04/20 22:38:55, Dan Beam wrote:
> RTL?

Done.

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/import_supervised_user.html:73:
padding-left: 10px;
On 2016/04/20 22:38:55, Dan Beam wrote:
> RTL?

Done.

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

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/import_supervised_user.js:68:
this.supervisedUsers_ = supervisedUsers || [];
On 2016/04/20 22:38:55, Dan Beam wrote:
> empty arrays are truthy in JS, so the "|| []" will never trigger.
> 
> null is not truthy.  can |supervisedUsers| explicitly be |null|?  if so, you
> should probably make this type:
> 
>   {?Array<!SupervisedUsers>}
> 
> alternatively, is this an optional parameter?  i.e.
> 
>   * @param {!Array<!SupervisedUser>=} opt_supervisedUsers
> 
> which means show could be called with only 1 argument (and |supervisedUsers|
> omitted and === undefined)

I was under the assumption that |supervisedUsers| which is returned by c++ could
be null or undefined. In which case this should've been the type:

   * @param {(?Array<!SupervisedUser>|undefined)} supervisedUsers

But, on a second thought, I don't think there is any reason to assume this could
be null or undefined when the return value in C++ clearly isn't. Am I right?

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/import_supervised_user.js:103: if
(!supervisedUser.onCurrentDevice) {
On 2016/04/20 22:38:55, Dan Beam wrote:
> no curlies

Done.

[Dan Beam, 2016-04-22 01:25:31]

lgtm

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

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:213: function(error)
{ this.handleMessage_(error); }.bind(this));
On 2016/04/21 17:32:30, Moe wrote:
> On 2016/04/20 22:38:55, Dan Beam wrote:
> > can we just update handleMessage_ to take an {Error|string} (or {*}) so this
> > code (and below) could be:
> > 
> >  
> this.browserProxy_.getExistingSupervisedUsers(signedInUser.profilePath).then(
> >       this.showImportSupervisedUserPopup_.bind(this),
> >       this.handleMessage_.bind(this));
> 
> Done. The catch is there'll be type cast needed here:
> 
> this.message_ =  /** @type {string} */ (message);
> 

could we just make it a real string?

  this.message_ = '' + message;

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

https://codereview.chromium.org/1901853002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:371: this.message_ = 
/** @type {string} */ (message);
this.message_ = '' + message;

[Moe, 2016-04-22 18:46:04]

Thank y'all!

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

https://codereview.chromium.org/1901853002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:213: function(error)
{ this.handleMessage_(error); }.bind(this));
On 2016/04/22 01:25:30, Dan Beam wrote:
> On 2016/04/21 17:32:30, Moe wrote:
> > On 2016/04/20 22:38:55, Dan Beam wrote:
> > > can we just update handleMessage_ to take an {Error|string} (or {*}) so
this
> > > code (and below) could be:
> > > 
> > >  
> >
this.browserProxy_.getExistingSupervisedUsers(signedInUser.profilePath).then(
> > >       this.showImportSupervisedUserPopup_.bind(this),
> > >       this.handleMessage_.bind(this));
> > 
> > Done. The catch is there'll be type cast needed here:
> > 
> > this.message_ =  /** @type {string} */ (message);
> > 
> 
> could we just make it a real string?
> 
>   this.message_ = '' + message;

Done.

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

https://codereview.chromium.org/1901853002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:371: this.message_ = 
/** @type {string} */ (message);
On 2016/04/22 01:25:31, Dan Beam wrote:
> this.message_ = '' + message;

Done.

[Moe, 2016-04-22 18:46:15]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-04-22 18:46:16]

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

[commit-bot: I haz the power, 2016-04-22 18:47:11]

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

[commit-bot: I haz the power, 2016-04-22 18:55:16]

Description was changed from

==========
Import supervised user dialog

This dialog is displayed when the user clicks on the 'import supervised users'
link from the 'create user' page on the user manager screen. Or when the users
attempts to create a supervised user that already exists and is prompted to
import the user instead.

browser tests for this patch will follow in another CL...

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

to

==========
Import supervised user dialog

This dialog is displayed when the user clicks on the 'import supervised users'
link from the 'create user' page on the user manager screen. Or when the users
attempts to create a supervised user that already exists and is prompted to
import the user instead.

browser tests for this patch will follow in another CL...

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

[commit-bot: I haz the power, 2016-04-22 18:55:17]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-22 19:50:37]

Description was changed from

==========
Import supervised user dialog

This dialog is displayed when the user clicks on the 'import supervised users'
link from the 'create user' page on the user manager screen. Or when the users
attempts to create a supervised user that already exists and is prompted to
import the user instead.

browser tests for this patch will follow in another CL...

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

to

==========
Import supervised user dialog

This dialog is displayed when the user clicks on the 'import supervised users'
link from the 'create user' page on the user manager screen. Or when the users
attempts to create a supervised user that already exists and is prompted to
import the user instead.

browser tests for this patch will follow in another CL...

BUG=563722

Committed: https://crrev.com/53539eb613fc973531c9a40f73eee83f4f65ddaa
Cr-Commit-Position: refs/heads/master@{#389187}
==========

[commit-bot: I haz the power, 2016-04-22 19:50:38]

Patchset 6 (id:??) landed as
https://crrev.com/53539eb613fc973531c9a40f73eee83f4f65ddaa
Cr-Commit-Position: refs/heads/master@{#389187}

[Moe, 2016-05-02 14:07:38]

Description was changed from

==========
Import supervised user dialog

This dialog is displayed when the user clicks on the 'import supervised users'
link from the 'create user' page on the user manager screen. Or when the users
attempts to create a supervised user that already exists and is prompted to
import the user instead.

browser tests for this patch will follow in another CL...

BUG=563722

Committed: https://crrev.com/53539eb613fc973531c9a40f73eee83f4f65ddaa
Cr-Commit-Position: refs/heads/master@{#389187}
==========

to

==========
Import supervised user dialog

This dialog is displayed when the user clicks on the 'import supervised users'
link from the 'create user' page on the user manager screen. Or when the users
attempts to create a supervised user that already exists and is prompted to
import the user instead.

browser tests for this patch will follow in another CL...

BUG=563722, 605899, 605481

Committed: https://crrev.com/53539eb613fc973531c9a40f73eee83f4f65ddaa
Cr-Commit-Position: refs/heads/master@{#389187}
==========