MD user manager (html/js/css for create profile flow)

MD user manager (html/js/css for create profile flow)

Part 2 of https://codereview.chromium.org/1630903002/

based on https://codereview.chromium.org/1716163005/

BUG=563722
Committed: https://crrev.com/4777abff1049063566fe011d6ee41097a6ab5814
Cr-Commit-Position: refs/heads/master@{#380242}

[Moe, 2016-02-23 00:04:28]

dbeam@ here's part 2 of https://codereview.chromium.org/1630903002/

[Moe, 2016-02-23 00:05:59]

Description was changed from

==========
MD user manager (html/js/css for create profile flow)

Part 2 of https://codereview.chromium.org/1630903002/

based on https://codereview.chromium.org/1716163005/

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

to

==========
MD user manager (html/js/css for create profile flow)

Part 2 of https://codereview.chromium.org/1630903002/

based on https://codereview.chromium.org/1716163005/

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

[Moe, 2016-02-23 00:05:59]

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

[Dan Beam, 2016-02-25 16:44:39]

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/control_bar.css:17: font-size: 13px;
this should be a relative unit (i.e. em or %)

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.css:6: font-size: 13px;
em or %

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.css:13: font-size: 16px;
em or %

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.css:19: color: #333;
is there a --google-grey-* or --paper-grey-* equivalent you could for any rgb()
or #hex color in this file?

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:34: <span
hidden$="{{!signedIn_}}">
can you move this to a <template is="dom-if" if="[[signedIn_]]"> instead?

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:41: <template
is="dom-repeat" items="[[signedInUsers_]]">
are you sure this list works?

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:54: <div
id="messageBubble" inner-h-t-m-l="{{message_}}"></div>
hacks++
security--

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:84: signedInUsers_n:
{
typo

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:106:
this.handleMessage_.bind(this));
can you use cr.addWebUIListener() instead?

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:129:
this.handleAvailableIcons_.bind(this));
it seems like these are request specific, could the signin.ProfileApi return a
Promise or take a callback so that you don't need to do this global callback
dance?

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:266: message != '';
nit:

  return createInProgress || !profileName || message ||
      !this.$.nameInput.validate();

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/profile_api.js:5: /** @typedef
{{username:(string), profilePath:(string)}} */
/** @typedef {{username: string, profilePath: string}} */

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/profile_api.js:8: /** @typedef
{{name:(string), filePath:(string), isSupervised:(boolean)}} */
drop ()s, they're only need for type unions i.e. (boolean|undefined)

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/profile_api.js:32:
ProfileApi.profileCreateSuccessCallback_ = null;
can you use cr.sendWithCallback() instead of these ^ globals?

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/profile_api.js:102:
cchrome.send('cancelCreateProfile');
i recommend you manually or automatedly test your code, and/or compile it

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager.css:7: }
please use text_defaults_md.css

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager.css:159: color: rgb(54,
54, 54);
this is kind of between --paper-grey-800 and --paper-grey-900 :(

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager.js:33: * @param {boolean}
showGuest True if |Browse as Guest| button should be
|pipes| are for variables or code-y font (think ` in github or <code> tags)

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager.js:119: });
i still don't understand how Oobe is used here

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager_pages.js:8: * screen.
don't wrap after @fileoverview

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager_pages.js:17: * @element
user-manager-pages
just remove Example: to the end of this comment

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager_pages.js:24: * id of the
currently selected page.
ID

[Moe, 2016-03-02 18:03:19]

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/control_bar.css:17: font-size: 13px;
On 2016/02/25 16:44:38, Dan Beam wrote:
> this should be a relative unit (i.e. em or %)

As per our discussion offline, fontsize and zoom level settings don't apply to
the user manager window, b/c the user manager uses a different profile from that
of regular chrome tabs.

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.css:6: font-size: 13px;
On 2016/02/25 16:44:39, Dan Beam wrote:
> em or %

For the same reason as in control_bar.css, I think we can keep these fixed.

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.css:13: font-size: 16px;
On 2016/02/25 16:44:38, Dan Beam wrote:
> em or %

ditto

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.css:19: color: #333;
On 2016/02/25 16:44:38, Dan Beam wrote:
> is there a --google-grey-* or --paper-grey-* equivalent you could for any
rgb()
> or #hex color in this file?

I went through all of them. The ones that are remained have no equivalent.

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:34: <span
hidden$="{{!signedIn_}}">
On 2016/02/25 16:44:39, Dan Beam wrote:
> can you move this to a <template is="dom-if" if="[[signedIn_]]"> instead?

Done.

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:41: <template
is="dom-repeat" items="[[signedInUsers_]]">
On 2016/02/25 16:44:39, Dan Beam wrote:
> are you sure this list works?

I just tested and it works.

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.html:54: <div
id="messageBubble" inner-h-t-m-l="{{message_}}"></div>
On 2016/02/25 16:44:39, Dan Beam wrote:
> hacks++
> security--

Correct. But as I mentioned in the old CL, I've gotten rid of this hack in the
upcoming CLs.

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:84: signedInUsers_n:
{
On 2016/02/25 16:44:39, Dan Beam wrote:
> typo

Done.

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:106:
this.handleMessage_.bind(this));
On 2016/02/25 16:44:39, Dan Beam wrote:
> can you use cr.addWebUIListener() instead?

Please see my comments below and changes to profile_api.js and see whether that
addresses your concern. Wouldn't using "cr.addWebUIListener()" go against
encapsulating things in the profile_api.js?

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:129:
this.handleAvailableIcons_.bind(this));
On 2016/02/25 16:44:39, Dan Beam wrote:
> it seems like these are request specific, could the signin.ProfileApi return a
> Promise or take a callback so that you don't need to do this global callback
> dance?

ProfileApi methods do receive callback. In future CLs I've experimented with
Promises too. What I'm doing wrong here I believe is that I'm assigning the
callbacks to the global ProfileApi function instead of "signin.ProfileApi"
object. I was simply following what was being done in
settings/people_page/sync_private_api.js.

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:266: message != '';
On 2016/02/25 16:44:39, Dan Beam wrote:
> nit:
> 
>   return createInProgress || !profileName || message ||
>       !this.$.nameInput.validate();

I can't check if message is empty like that b/c the method will be returning
string (instead of boolean) if createInProgress is false and profileName is a
non-empty string. Also for this.$.nameInput I'm assigning it to a variable to
stop the closure compiler from complaining that this.$.nameInput doesn't have a
validate() property.
/** @type {{validate:(function())}} */
var nameInput = this.$.nameInput;

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/profile_api.js:5: /** @typedef
{{username:(string), profilePath:(string)}} */
On 2016/02/25 16:44:39, Dan Beam wrote:
> /** @typedef {{username: string, profilePath: string}} */

Done.

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/profile_api.js:8: /** @typedef
{{name:(string), filePath:(string), isSupervised:(boolean)}} */
On 2016/02/25 16:44:39, Dan Beam wrote:
> drop ()s, they're only need for type unions i.e. (boolean|undefined)

Done.

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/profile_api.js:32:
ProfileApi.profileCreateSuccessCallback_ = null;
On 2016/02/25 16:44:39, Dan Beam wrote:
> can you use cr.sendWithCallback() instead of these ^ globals?

Is it also considered a bad practice to assign these callbacks to the instance
(vs globals)? In future iterations I can look into replacing most of these with
"cr.sendWithPromise"

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/profile_api.js:102:
cchrome.send('cancelCreateProfile');
On 2016/02/25 16:44:39, Dan Beam wrote:
> i recommend you manually or automatedly test your code, and/or compile it

Sorry about this. This must have snuck in while breaking up the old CL. I had
caught and fixed it in patch #2.

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager.css:7: }
On 2016/02/25 16:44:39, Dan Beam wrote:
> please use text_defaults_md.css

Done.

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager.css:159: color: rgb(54,
54, 54);
On 2016/02/25 16:44:39, Dan Beam wrote:
> this is kind of between --paper-grey-800 and --paper-grey-900 :(

These are old user pod styles. the designers didn't make any comments on this. I
think it's safe to go with --paper-grey-800 since it's closer to it.

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager.js:33: * @param {boolean}
showGuest True if |Browse as Guest| button should be
On 2016/02/25 16:44:39, Dan Beam wrote:
> |pipes| are for variables or code-y font (think ` in github or <code> tags)

Done.

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager.js:119: });
On 2016/02/25 16:44:39, Dan Beam wrote:
> i still don't understand how Oobe is used here

some of Oobe functions are called from signin/user_manager_screen_handler.cc. I
don't know why it was necessary to use it here. I wouldn't however touch it
until everything else goes in and we decide to fork the user pods from cros.

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager_pages.js:8: * screen.
On 2016/02/25 16:44:39, Dan Beam wrote:
> don't wrap after @fileoverview

Done.

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager_pages.js:17: * @element
user-manager-pages
On 2016/02/25 16:44:39, Dan Beam wrote:
> just remove Example: to the end of this comment

Done.

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager_pages.js:24: * id of the
currently selected page.
On 2016/02/25 16:44:39, Dan Beam wrote:
> ID

Done.

[Dan Beam, 2016-03-02 23:19:24]

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

https://codereview.chromium.org/1722843002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/user_manager.js:119: });
On 2016/03/02 18:03:18, Moe wrote:
> On 2016/02/25 16:44:39, Dan Beam wrote:
> > i still don't understand how Oobe is used here
> 
> some of Oobe functions are called from signin/user_manager_screen_handler.cc.
I
> don't know why it was necessary to use it here. I wouldn't however touch it
> until everything else goes in and we decide to fork the user pods from cros.

you're propagating code that no longer makes sense.  please don't use the name
"Oobe" (out of box experience) unless it's actually relevant.

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.css:17: font-size: 13px;
can this just inherit from body instead of copying the constant?

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.css:18: line-height: 32px;
why does this differ from polymer?

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.html:8: "shared styles"
format. -->
nit:

<!-- TODO(...): ...
     wrap second line at same indent as TODO(). -->

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.css:6: font-size: 13px;
is this the same as body?

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.css:29: };
#nameInput,
paper-dropdown-menu {
  --paper-input-container-input: {
    color: #333;
    font-size: inherit;
  };
}

#nameInput {
  width: 300px;
}

paper-dropdown-menu {
  position: relative;
  top: -1px;
  width: 200px;
}

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:47:
on-tap="onLearnMore_"></a>
using an action-link likely requires including the css within this <dom-module>

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:57: <paper-button
id="cancel-button" on-tap="onCancel_"
nit: onCancelTap_

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:60: <paper-button
id="ok-button" on-tap="onOk_"
nit: onOkTap_

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/profile_api.js:16: return {
why is this better than the previous code?

the code in earlier patchsets more closely mirrors the typical webui convention

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.html:11: <link
rel="stylesheet" href="chrome://resources/css/chrome_shared.css">
why are you using chrome_shared.css?  this applies to a previous design style
and probably shouldn't be used in Material Design style pages

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.html:24: <link
rel="import" href="chrome://resources/html/action_link.html">
there seem to be transitive dependencies in this list.  please only list direct
dependencies as imports at each level

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.html:36: <body
class="oobe-display" unresolved>
why is oobe again mentioned here?

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:138: var Oobe =
cr.ui.Oobe;
this doesn't make much sense, we define cr.ui.Oobe just to alias it at Oobe
again

[Moe, 2016-03-03 18:05:46]

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.css:17: font-size: 13px;
On 2016/03/02 23:19:23, Dan Beam wrote:
> can this just inherit from body instead of copying the constant?

Done.

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.css:18: line-height: 32px;
On 2016/03/02 23:19:23, Dan Beam wrote:
> why does this differ from polymer?

using padding-top(-bottom) set to zero and line-height set to 32px I'm trying to
achieve a height of 32px for the button.

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.html:8: "shared styles"
format. -->
On 2016/03/02 23:19:23, Dan Beam wrote:
> nit:
> 
> <!-- TODO(...): ...
>      wrap second line at same indent as TODO(). -->

Done.

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.css:6: font-size: 13px;
On 2016/03/02 23:19:24, Dan Beam wrote:
> is this the same as body?

Done. Yes I can set the font-size on body to 13px and inherit these from that.

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.css:29: };
On 2016/03/02 23:19:24, Dan Beam wrote:
> #nameInput,
> paper-dropdown-menu {
>   --paper-input-container-input: {
>     color: #333;
>     font-size: inherit;
>   };
> }
> 
> #nameInput {
>   width: 300px;
> }
> 
> paper-dropdown-menu {
>   position: relative;
>   top: -1px;
>   width: 200px;
> }

Done.

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:47:
on-tap="onLearnMore_"></a>
On 2016/03/02 23:19:24, Dan Beam wrote:
> using an action-link likely requires including the css within this
<dom-module>

Done.

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:57: <paper-button
id="cancel-button" on-tap="onCancel_"
On 2016/03/02 23:19:24, Dan Beam wrote:
> nit: onCancelTap_

Changed button IDs and handler names in the other active CL.

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.html:60: <paper-button
id="ok-button" on-tap="onOk_"
On 2016/03/02 23:19:24, Dan Beam wrote:
> nit: onOkTap_

Changed button IDs and handler names in the other active CL.

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/profile_api.js:16: return {
On 2016/03/02 23:19:24, Dan Beam wrote:
> why is this better than the previous code?
> 
> the code in earlier patchsets more closely mirrors the typical webui
convention

IMO this is cleaner and less verbose. 'cr.define' expects a callback function
which returns an object containing names and values of the fields. This is
closer to it that than returning a function object with properties defined on
it. Also in the current convention (as in e.g.,
settings/people_page/sync_private_api.js) callbacks are being set on the
function object not the resulting object instance which seems more intuitive.

PS. I still don't quite understand what I need to do with the callbacks. From
what I understand callbacks need to be stored somewhere. Is storing them at all
what's wrong with this code?

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.html:11: <link
rel="stylesheet" href="chrome://resources/css/chrome_shared.css">
On 2016/03/02 23:19:24, Dan Beam wrote:
> why are you using chrome_shared.css?  this applies to a previous design style
> and probably shouldn't be used in Material Design style pages

Done.

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.html:24: <link
rel="import" href="chrome://resources/html/action_link.html">
On 2016/03/02 23:19:24, Dan Beam wrote:
> there seem to be transitive dependencies in this list.  please only list
direct
> dependencies as imports at each level

From the list, I removed load_time_data.html and action_link.html which were not
needed. The rest are dependencies of what's included in the user_manager.js. 
These used to be included as external JS scripts and I converted as many as I
could to html dependencies to avoid having them included multiple times.

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.html:36: <body
class="oobe-display" unresolved>
On 2016/03/02 23:19:24, Dan Beam wrote:
> why is oobe again mentioned here?

a lot of the rules in src/ui/login/screen_container.css assume '.oobe-display'
on the body as well things like '#outer-container', '#inner-container', and
'#oobe' IDs which you will find in this html. Until we fork and redo the
userpods there really is no escaping having these here. I understand this page
is a weird hybrid of Polymer and traditional webui page at its current state,
but redoing the user pods right now is hard to justify, given that this needs go
out before the new settings page.

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:138: var Oobe =
cr.ui.Oobe;
On 2016/03/02 23:19:24, Dan Beam wrote:
> this doesn't make much sense, we define cr.ui.Oobe just to alias it at Oobe
> again

src/ui/login/account_picker/user_pod_row.js (which is shared with cros) calls
methods on Oobe, hence the alias. I left a comment that explains this.

I also combined UserManager with cr.ui.Oobe getting rid of the latter. So 'Oobe'
is referenced only once here and once in the html/css which is hard avoid for
the reasons I explain in the user_manager.html

[Dan Beam, 2016-03-04 02:45:49]

looking better to me

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

https://codereview.chromium.org/1722843002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/profile_api.js:16: return {
On 2016/03/03 18:05:46, Moe wrote:
> On 2016/03/02 23:19:24, Dan Beam wrote:
> > why is this better than the previous code?
> > 
> > the code in earlier patchsets more closely mirrors the typical webui
> convention
> 
> IMO this is cleaner and less verbose. 'cr.define' expects a callback function
> which returns an object containing names and values of the fields. This is
> closer to it that than returning a function object with properties defined on
> it.

if you really want "cleanest" and don't want to do

  cr.define('signin', function() {
    ... defined ProfileApi ...
    return {ProfileApi: ProfileApi};
  });

you can do

  cr.exportPath('signin');

  signin.ProfileApi = function() {};

  signin.ProfileApi.prototype = {
    ...
  };

BUT we have specific logic in our closure compiler pass that looks for the
cr.define() pattern (and I don't see a utility to supporting another format if
your new code doesn't compile)

> Also in the current convention (as in e.g.,
> settings/people_page/sync_private_api.js) callbacks are being set on the
> function object not the resulting object instance which seems more intuitive.

I would recommend not using globals, whether or not they're bound to namespaced,
static context or not.  The old code is was a bummer like that, but not bad
enough to immediately rewrite.  We will likely rewrite it while porting to
Material Design settings.

I'm sorry you've hit so many landmines.

> 
> PS. I still don't quite understand what I need to do with the callbacks. From
> what I understand callbacks need to be stored somewhere. Is storing them at
all
> what's wrong with this code? 

yes, it's very hard to test (especially asynchronously) and probably acts
unexpectedly when called from multiple sources

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

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.html:15:
i18n-content="browseAsGuest" hidden$="[[!showGuest]]"></paper-button>
hidden= works and is faster (the polymer dev guide's explanation as to why one
might always use $= does not really apply to chrome-specific code)

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

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:112: detached:
function() {
do you expect detached to ever actually happen?

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:125: if
(event.target.selected == 'create-user-page') {
nit:

if (event.target.selected != 'create-user-page')
  return;

... less indented ...

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:262: /** @type
{{validate:(function())}} */
this should be a PaperInputElement

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

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/profile_api.js:22:
this.profileCreateSuccessCallback_ = callback;
the closure compiler might say "dangerous use of this" for these

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

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.css:6: font-size: 13px;
this will probably be the default by the time you read this comment
https://codereview.chromium.org/1754803005

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

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.html:60: <script
src="chrome://resources/js/cr/ui/grid.js"></script>
it's not clear that all of these are required.  when are you going to audit
this?

we'd like to remove unused UI components from cr/ui when we've moved over to all
Polymer

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

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:44: * displayed.
@param ...
    ... extra likes wrap at 4\s indent ...
^^^^

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:50: data:
{disableAddUser: false}});
indent off

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:111: var Oobe =
cr.ui.UserManager;
this is unfortunate but at least not in code i own

[Moe, 2016-03-08 00:57:05]

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

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.html:15:
i18n-content="browseAsGuest" hidden$="[[!showGuest]]"></paper-button>
On 2016/03/04 02:45:48, Dan Beam wrote:
> hidden= works and is faster (the polymer dev guide's explanation as to why one
> might always use $= does not really apply to chrome-specific code)

Done.

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

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:112: detached:
function() {
On 2016/03/04 02:45:49, Dan Beam wrote:
> do you expect detached to ever actually happen?

It should now. I put create-profile under a dom-if template with restamp
attribute set to true.

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:125: if
(event.target.selected == 'create-user-page') {
On 2016/03/04 02:45:48, Dan Beam wrote:
> nit:
> 
> if (event.target.selected != 'create-user-page')
>   return;
> 
> ... less indented ...

Done.

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:262: /** @type
{{validate:(function())}} */
On 2016/03/04 02:45:49, Dan Beam wrote:
> this should be a PaperInputElement

Changing this to PaperInputElement and adding the following dependency to my
compiled_resources2.gyp, I receive an error:

'<(DEPTH)/third_party/polymer/v1_0/components-chromium/paper-input/compiled_resources2.gyp:paper-input-extracted',

(ERROR) Error in: paper-input-char-counter-extracted.js
##
/usr/local/google/home/mahmadi/chromium/src/third_party/polymer/v1_0/components-chromium/paper-input/paper-input-char-counter-extracted.js:16:
ERROR - Property inputElement never defined on state
##       if (!state.inputElement) {
##                  ^

I looked at the dependencies of 'paper-input-char-counter-extracted' and it
seems correct. Could this be a bug or am I missing something here?

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

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/profile_api.js:22:
this.profileCreateSuccessCallback_ = callback;
On 2016/03/04 02:45:49, Dan Beam wrote:
> the closure compiler might say "dangerous use of this" for these

Reverted this to the old format.

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

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.css:6: font-size: 13px;
On 2016/03/04 02:45:49, Dan Beam wrote:
> this will probably be the default by the time you read this comment
> https://codereview.chromium.org/1754803005

Sweet!

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

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.html:60: <script
src="chrome://resources/js/cr/ui/grid.js"></script>
On 2016/03/04 02:45:49, Dan Beam wrote:
> it's not clear that all of these are required.  when are you going to audit
> this?
> 
> we'd like to remove unused UI components from cr/ui when we've moved over to
all
> Polymer

Removed the resources, including the html components, that were unused.

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

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:44: * displayed.
On 2016/03/04 02:45:49, Dan Beam wrote:
> @param ...
>     ... extra likes wrap at 4\s indent ...
> ^^^^

Done.

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:50: data:
{disableAddUser: false}});
On 2016/03/04 02:45:49, Dan Beam wrote:
> indent off

Done.

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:111: var Oobe =
cr.ui.UserManager;
On 2016/03/04 02:45:49, Dan Beam wrote:
> this is unfortunate but at least not in code i own

I agree. The code is easier to understand now. We see 'Oobe' once and we know
why.

[Dan Beam, 2016-03-08 01:18:16]

the source looks pretty good to me, but I don't see any tests for the JS or C++.

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

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:262: /** @type
{{validate:(function())}} */
On 2016/03/08 00:57:04, Moe wrote:
> On 2016/03/04 02:45:49, Dan Beam wrote:
> > this should be a PaperInputElement
> 
> Changing this to PaperInputElement and adding the following dependency to my
> compiled_resources2.gyp, I receive an error:
> 
>
'<(DEPTH)/third_party/polymer/v1_0/components-chromium/paper-input/compiled_resources2.gyp:paper-input-extracted',
> 
> (ERROR) Error in: paper-input-char-counter-extracted.js
> ##
>
/usr/local/google/home/mahmadi/chromium/src/third_party/polymer/v1_0/components-chromium/paper-input/paper-input-char-counter-extracted.js:16:
> ERROR - Property inputElement never defined on state
> ##       if (!state.inputElement) {
> ##                  ^
> 
> I looked at the dependencies of 'paper-input-char-counter-extracted' and it
> seems correct. Could this be a bug or am I missing something here?

this is a problem with compiling Polymer.  how about putting a TODO() so it's
clear that this shouldn't be the final state of this code

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:262: /** @type
{{validate:(function())}} */
nit: /** @type {{validate: function():boolean}} */

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

https://codereview.chromium.org/1722843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.html:14: <paper-button
id="guest-user-button" on-click="handleGuestClick_"
we generally use on-tap instead

https://codereview.chromium.org/1722843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.html:14: <paper-button
id="guest-user-button" on-click="handleGuestClick_"
nit: onGuestUserButtonClick_ (or Tap_)

https://codereview.chromium.org/1722843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.html:16: <paper-button
id="add-user-button" on-click="handleAddUserClick_"
nit: onAddUserButtonClick_ (or Tap_)

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

https://codereview.chromium.org/1722843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.html:27: <div id="oobe"
class="faded">
does this still need to be oobe?

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

https://codereview.chromium.org/1722843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:49: data:
{disableAddUser: false}});
indent still off ;)

UserManager.getInstance().showScreen({id: 'account-picker',
                                      data: {disableAddUser: false}});

or

UserManager.getInstance().showScreen({
  id: 'account-picker',
  data: {disableAddUser: false},
});

or something along those lines

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

https://codereview.chromium.org/1722843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager_pages.html:15: <template
is="dom-if" if="[[isPageVisible_(selectedPage_, 'create-user-page')]]" restamp>
nit:

<template is="dom-if"
    if="... long conditional ...">

why [restamp]?  so it actually gets unrendered?

[Moe, 2016-03-08 15:35:29]

Patchset #6 (id:100001) has been deleted

[Moe, 2016-03-08 15:47:34]

This CL is quite big. I'll talk with tommycli@ to get direction on adding tests.
I'll send a follow-up CL for the tests if that's ok.

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

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:262: /** @type
{{validate:(function())}} */
On 2016/03/08 01:18:16, Dan Beam wrote:
> nit: /** @type {{validate: function():boolean}} */

Done.

https://codereview.chromium.org/1722843002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/create_profile.js:262: /** @type
{{validate:(function())}} */
On 2016/03/08 01:18:16, Dan Beam wrote:
> On 2016/03/08 00:57:04, Moe wrote:
> > On 2016/03/04 02:45:49, Dan Beam wrote:
> > > this should be a PaperInputElement
> > 
> > Changing this to PaperInputElement and adding the following dependency to my
> > compiled_resources2.gyp, I receive an error:
> > 
> >
>
'<(DEPTH)/third_party/polymer/v1_0/components-chromium/paper-input/compiled_resources2.gyp:paper-input-extracted',
> > 
> > (ERROR) Error in: paper-input-char-counter-extracted.js
> > ##
> >
>
/usr/local/google/home/mahmadi/chromium/src/third_party/polymer/v1_0/components-chromium/paper-input/paper-input-char-counter-extracted.js:16:
> > ERROR - Property inputElement never defined on state
> > ##       if (!state.inputElement) {
> > ##                  ^
> > 
> > I looked at the dependencies of 'paper-input-char-counter-extracted' and it
> > seems correct. Could this be a bug or am I missing something here?
> 
> this is a problem with compiling Polymer.  how about putting a TODO() so it's
> clear that this shouldn't be the final state of this code

Done.

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

https://codereview.chromium.org/1722843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.html:14: <paper-button
id="guest-user-button" on-click="handleGuestClick_"
On 2016/03/08 01:18:16, Dan Beam wrote:
> nit: onGuestUserButtonClick_ (or Tap_)

I have the events and their handler names fixed in the other CL that's based on
this. Will go ahead and fix them here as well.

https://codereview.chromium.org/1722843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.html:14: <paper-button
id="guest-user-button" on-click="handleGuestClick_"
On 2016/03/08 01:18:16, Dan Beam wrote:
> we generally use on-tap instead

Done.

https://codereview.chromium.org/1722843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/control_bar.html:16: <paper-button
id="add-user-button" on-click="handleAddUserClick_"
On 2016/03/08 01:18:16, Dan Beam wrote:
> nit: onAddUserButtonClick_ (or Tap_)

Done.

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

https://codereview.chromium.org/1722843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.html:27: <div id="oobe"
class="faded">
On 2016/03/08 01:18:16, Dan Beam wrote:
> does this still need to be oobe?

Unfortunately this one has to stay. It's expected to be there in user_pod_row.js
when the pods are created. I got rid of ".oobe-display" on the body however.

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

https://codereview.chromium.org/1722843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager.js:49: data:
{disableAddUser: false}});
On 2016/03/08 01:18:16, Dan Beam wrote:
> indent still off ;)
> 
> UserManager.getInstance().showScreen({id: 'account-picker',
>                                       data: {disableAddUser: false}});
> 
> or
> 
> UserManager.getInstance().showScreen({
>   id: 'account-picker',
>   data: {disableAddUser: false},
> });
> 
> or something along those lines

My bad. Got it now.

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

https://codereview.chromium.org/1722843002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_user_manager/user_manager_pages.html:15: <template
is="dom-if" if="[[isPageVisible_(selectedPage_, 'create-user-page')]]" restamp>
On 2016/03/08 01:18:16, Dan Beam wrote:
> nit:
> 
> <template is="dom-if"
>     if="... long conditional ...">
> 
> why [restamp]?  so it actually gets unrendered?

Without 'restamp' create-profile won't get detached from the DOM and webui
listeners won't get unregistered in WebUIListenerBehavior. I followed
settings/basic_page/basic_page.html as an example.

[Dan Beam, 2016-03-08 19:22:07]

On 2016/03/08 15:47:34, Moe wrote:
> This CL is quite big. I'll talk with tommycli@ to get direction on adding
tests.
> I'll send a follow-up CL for the tests if that's ok.

that's why you write small CLs ;)

[Dan Beam, 2016-03-08 21:10:30]

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

https://codereview.chromium.org/1722843002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:37:
chrome.send('userManagerInitialize', [window.location.hash]);
it's preferable to not interact with the browser directly so that we can isolate
just the JS for testing

https://codereview.chromium.org/1722843002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:101: };
this class looks mainly like a wrapper?  what's the point of it?

[Moe, 2016-03-09 15:29:03]

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

https://codereview.chromium.org/1722843002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:37:
chrome.send('userManagerInitialize', [window.location.hash]);
On 2016/03/08 21:10:30, Dan Beam wrote:
> it's preferable to not interact with the browser directly so that we can
isolate
> just the JS for testing

Done.

https://codereview.chromium.org/1722843002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/user_manager.js:101: };
On 2016/03/08 21:10:30, Dan Beam wrote:
> this class looks mainly like a wrapper?  what's the point of it?

That's correct. src/chrome/browser/resources/chromeos/login/login_common.js
creates a wrapper around cr.ui.login.DisplayManager called Oobe. Oobe is what
being used in src/ui/login/account_picker/user_pod_row.js (which is shared
between cros and desktop). Creating UserManager and then aliasing it to Oobe is
to make user_pod_row.js work in desktop as well without having to fork and
modify it. This is at least why i think this was done this way initially.

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/profile_browser_proxy.js (right):

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:116:
ProfileBrowserProxy: ProfileBrowserProxy,
I noticed in the existing examples under settings/ the interface is not being
returned as a property here on the output object. This makes for example
"signin.ProfileBrowserProxy" a bad type according to closure compiler.

[Dan Beam, 2016-03-09 18:21:37]

ok, i see we're taking steps toward making this class testable (awesome).

lgtm with the promise to add tests in the near future

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/profile_browser_proxy.js (right):

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:24:
getAvailableIcons: function() {},
nit: can this be assertNotReached or something else to attempt to show/enforce
that this class is abstract

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:39: * @param
{string} profileIconUrl URL of the selected icon of the new profile.
80 col wrap

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:116:
ProfileBrowserProxy: ProfileBrowserProxy,
On 2016/03/09 15:29:03, Moe wrote:
> I noticed in the existing examples under settings/ the interface is not being
> returned as a property here on the output object. This makes for example
> "signin.ProfileBrowserProxy" a bad type according to closure compiler.

yeah, this is because of 2 things:
1) technically the type isn't exported unless it's returned
2) our compiler pass[1] handles this fancy export mechanism fragily

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur...

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:120: 
nit: remove extra \n

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:126: /**
@private {!signin.ProfileBrowserProxy} */
this should not be @private if it's externally used

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:132:
this.browserProxy_ = signin.ProfileBrowserProxyImpl.getInstance();
i think it's probably better to set/inject this or just call getInstance()
directly from your classes

maybe just drop this behavior?

[Moe, 2016-03-09 19:30:08]

Thanks! Tests will follow.

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/md_user_manager/profile_browser_proxy.js (right):

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:24:
getAvailableIcons: function() {},
On 2016/03/09 18:21:37, Dan Beam wrote:
> nit: can this be assertNotReached or something else to attempt to show/enforce
> that this class is abstract

Done.

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:39: * @param
{string} profileIconUrl URL of the selected icon of the new profile.
On 2016/03/09 18:21:37, Dan Beam wrote:
> 80 col wrap

Done.

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:116:
ProfileBrowserProxy: ProfileBrowserProxy,
On 2016/03/09 18:21:37, Dan Beam wrote:
> On 2016/03/09 15:29:03, Moe wrote:
> > I noticed in the existing examples under settings/ the interface is not
being
> > returned as a property here on the output object. This makes for example
> > "signin.ProfileBrowserProxy" a bad type according to closure compiler.
> 
> yeah, this is because of 2 things:
> 1) technically the type isn't exported unless it's returned
> 2) our compiler pass[1] handles this fancy export mechanism fragily
> 
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/closur...

Acknowledged.

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:120: 
On 2016/03/09 18:21:37, Dan Beam wrote:
> nit: remove extra \n

Done.

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:126: /**
@private {!signin.ProfileBrowserProxy} */
On 2016/03/09 18:21:37, Dan Beam wrote:
> this should not be @private if it's externally used

Done.

https://codereview.chromium.org/1722843002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/md_user_manager/profile_browser_proxy.js:132:
this.browserProxy_ = signin.ProfileBrowserProxyImpl.getInstance();
On 2016/03/09 18:21:37, Dan Beam wrote:
> i think it's probably better to set/inject this or just call getInstance()
> directly from your classes
> 
> maybe just drop this behavior?

Done.

[Moe, 2016-03-09 21:00:45]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-03-09 21:00:45]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1722843002/#ps160001
(title: "Addressed comments #6")

[commit-bot: I haz the power, 2016-03-09 21:01:40]

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

[commit-bot: I haz the power, 2016-03-09 22:27:22]

Description was changed from

==========
MD user manager (html/js/css for create profile flow)

Part 2 of https://codereview.chromium.org/1630903002/

based on https://codereview.chromium.org/1716163005/

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

to

==========
MD user manager (html/js/css for create profile flow)

Part 2 of https://codereview.chromium.org/1630903002/

based on https://codereview.chromium.org/1716163005/

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

[commit-bot: I haz the power, 2016-03-09 22:27:23]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-03-09 22:47:58]

Description was changed from

==========
MD user manager (html/js/css for create profile flow)

Part 2 of https://codereview.chromium.org/1630903002/

based on https://codereview.chromium.org/1716163005/

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

to

==========
MD user manager (html/js/css for create profile flow)

Part 2 of https://codereview.chromium.org/1630903002/

based on https://codereview.chromium.org/1716163005/

BUG=563722

Committed: https://crrev.com/4777abff1049063566fe011d6ee41097a6ab5814
Cr-Commit-Position: refs/heads/master@{#380242}
==========

[commit-bot: I haz the power, 2016-03-09 22:48:00]

Patchset 8 (id:??) landed as
https://crrev.com/4777abff1049063566fe011d6ee41097a6ab5814
Cr-Commit-Position: refs/heads/master@{#380242}