[MD settings][cros accounts] Updates the accounts page according to the specs

[MD settings][cros accounts] Updates the accounts page according to the specs

BUG=598042, 563928
1. checkbox unchecked state:
https://screenshot.googleplex.com/dkZsoRT2TFx
2. checkbox checked state:
https://screenshot.googleplex.com/Qt4JzHLWQnN

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/af59c5177d3a4012d39e3ea1471ab865b309f9fe
Cr-Commit-Position: refs/heads/master@{#432287}

[Moe, 2016-11-11 23:53:00]

Description was changed from

==========
[MD settings][cros accounts] Updates the accounts page according to the specs

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

to

==========
[MD settings][cros accounts] Updates the accounts page according to the specs

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Moe, 2016-11-11 23:53:16]

The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 23:53:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-12 01:57:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-12 01:57:56]

Dry run: This issue passed the CQ dry run.

[Moe, 2016-11-14 15:00:03]

Description was changed from

==========
[MD settings][cros accounts] Updates the accounts page according to the specs

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings][cros accounts] Updates the accounts page according to the specs

BUG=598042, 563928
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Moe, 2016-11-14 15:03:42]

Description was changed from

==========
[MD settings][cros accounts] Updates the accounts page according to the specs

BUG=598042, 563928
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings][cros accounts] Updates the accounts page according to the specs

BUG=598042, 563928

1. checkbox unchecked state: https://screenshot.googleplex.com/dkZsoRT2TFx
2. checkbox checked state:
https://screenshot.googleplex.com/Qt4JzHLWQnN

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Moe, 2016-11-14 16:51:17]

The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run

[Moe, 2016-11-14 16:51:33]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-11-14 16:51:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Moe, 2016-11-14 16:52:00]

Description was changed from

==========
[MD settings][cros accounts] Updates the accounts page according to the specs

BUG=598042, 563928

1. checkbox unchecked state: https://screenshot.googleplex.com/dkZsoRT2TFx
2. checkbox checked state:
https://screenshot.googleplex.com/Qt4JzHLWQnN

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings][cros accounts] Updates the accounts page according to the specs

BUG=598042, 563928

1. checkbox unchecked state:
https://screenshot.googleplex.com/dkZsoRT2TFx
2. checkbox checked state:
https://screenshot.googleplex.com/Qt4JzHLWQnN

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Moe, 2016-11-14 17:04:23]

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

[Moe, 2016-11-14 17:04:24]

Hi Tommy,

Please take a look at this CL. This should fix the two bugs linked in the
description. As for crbug.com/565887, I assigned it back to you to keep an eye
on.

There is a remaining issue here though, the profile name in cros
chrome.usersPrivate.User is the same as the profile email. We have the same
issue in the people card as well:
https://screenshot.googleplex.com/26S3GV6OoN9

Perhaps you can fix ProfileInfoHandler::GetAccountNameAndIcon() to return the
correct profile name and call ProfileInfoBrowserProxy.prototype.getProfileInfo
for every user in chrome.usersPrivate.User in settings-user-list.

PS. I found it very difficult to test cros as device owner. the best way I found
was to use --stub-cros-settings command line flag.

[commit-bot: I haz the power, 2016-11-14 17:52:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-14 17:52:37]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-11-14 20:11:58]

lgtm except:

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/user_list.html (right):

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/user_list.html:41: [[item.name]]
Odd that you say the display name isn't shown correctly. I think I specifically
addressed that with this patch for supervised users:
https://codereview.chromium.org/2259943003

Does the behavior differ from Old Options?

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/user_list.js (left):

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/user_list.js:42:
chrome.settingsPrivate.onPrefsChanged.addListener(function(prefs) {
What happened so that we no longer need the pref change listener?

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/user_list.js (right):

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/user_list.js:46: if
(this.isAttached &&
Can this ever fail? i.e., can this callback be called before the element is
attached?

[Moe, 2016-11-15 16:46:48]

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/user_list.html (right):

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/user_list.html:41: [[item.name]]
On 2016/11/14 20:11:57, tommycli wrote:
> Odd that you say the display name isn't shown correctly. I think I
specifically
> addressed that with this patch for supervised users:
> https://codereview.chromium.org/2259943003
> 
> Does the behavior differ from Old Options?

Your patch clearly returns the display email (the original email with dots) as
the "name" and not the display name. The behavior in the old settings page was
the same. The difference was that only display email and profile picture were
being displayed whereas in the MD settings both name and email must be
displayed. There even seems to be a TODO regarding fetching the name:
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/chromeos...

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/user_list.js (left):

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/user_list.js:42:
chrome.settingsPrivate.onPrefsChanged.addListener(function(prefs) {
On 2016/11/14 20:11:57, tommycli wrote:
> What happened so that we no longer need the pref change listener?

I was under the impression that, in order to add another user, we have to
navigate away from the page and since we fetch the user list on navigating to
the page, we don't need to listen to the pref change. Giving it more though, I
think we should keep this around.

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/user_list.js (right):

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/user_list.js:46: if
(this.isAttached &&
On 2016/11/14 20:11:58, tommycli wrote:
> Can this ever fail? i.e., can this callback be called before the element is
> attached?

From what I see in route.js no. But I saw this
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people...
and thought you must've have run into this issue before and hence the check.

[Moe, 2016-11-15 16:47:10]

The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 16:47:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommycli, 2016-11-15 17:50:35]

great thanks!

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/user_list.html (right):

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/user_list.html:41: [[item.name]]
On 2016/11/15 16:46:48, moe wrote:
> On 2016/11/14 20:11:57, tommycli wrote:
> > Odd that you say the display name isn't shown correctly. I think I
> specifically
> > addressed that with this patch for supervised users:
> > https://codereview.chromium.org/2259943003
> > 
> > Does the behavior differ from Old Options?
> 
> Your patch clearly returns the display email (the original email with dots) as
> the "name" and not the display name. The behavior in the old settings page was
> the same. The difference was that only display email and profile picture were
> being displayed whereas in the MD settings both name and email must be
> displayed. There even seems to be a TODO regarding fetching the name:
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/chromeos...

Okay cool. So long as the behavior is consistent with Old Options I'm fine with
it.

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/user_list.js (right):

https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/user_list.js:46: if
(this.isAttached &&
On 2016/11/15 16:46:48, moe wrote:
> On 2016/11/14 20:11:58, tommycli wrote:
> > Can this ever fail? i.e., can this callback be called before the element is
> > attached?
> 
> From what I see in route.js no. But I saw this
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people...
> and thought you must've have run into this issue before and hence the check.

Honestly I'm not sure why it's in sync_page either. Maybe just remove it until
we can be sure we actually need it.

[commit-bot: I haz the power, 2016-11-15 18:19:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 18:19:12]

Dry run: This issue passed the CQ dry run.

[Moe, 2016-11-15 18:20:34]

On 2016/11/15 17:50:35, tommycli wrote:
> great thanks!
> 
>
https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/people_page/user_list.html (right):
> 
>
https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/people_page/user_list.html:41: [[item.name]]
> On 2016/11/15 16:46:48, moe wrote:
> > On 2016/11/14 20:11:57, tommycli wrote:
> > > Odd that you say the display name isn't shown correctly. I think I
> > specifically
> > > addressed that with this patch for supervised users:
> > > https://codereview.chromium.org/2259943003
> > > 
> > > Does the behavior differ from Old Options?
> > 
> > Your patch clearly returns the display email (the original email with dots)
as
> > the "name" and not the display name. The behavior in the old settings page
was
> > the same. The difference was that only display email and profile picture
were
> > being displayed whereas in the MD settings both name and email must be
> > displayed. There even seems to be a TODO regarding fetching the name:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/chromeos...
> 
> Okay cool. So long as the behavior is consistent with Old Options I'm fine
with
> it.
> 
>
https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/people_page/user_list.js (right):
> 
>
https://codereview.chromium.org/2495933002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/people_page/user_list.js:46: if
> (this.isAttached &&
> On 2016/11/15 16:46:48, moe wrote:
> > On 2016/11/14 20:11:58, tommycli wrote:
> > > Can this ever fail? i.e., can this callback be called before the element
is
> > > attached?
> > 
> > From what I see in route.js no. But I saw this
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people...
> > and thought you must've have run into this issue before and hence the check.
> 
> Honestly I'm not sure why it's in sync_page either. Maybe just remove it until
> we can be sure we actually need it.

Sure. I removed both instances. Thank you.

[Moe, 2016-11-15 18:21:17]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2016-11-15 18:21:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
tommycli@chromium.org
Link to the patchset: https://codereview.chromium.org/2495933002/#ps60001
(title: "Addressed comment")

[commit-bot: I haz the power, 2016-11-15 18:21:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 20:06:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 20:06:51]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Moe, 2016-11-15 22:56:52]

The CQ bit was checked by mahmadi@chromium.org

[commit-bot: I haz the power, 2016-11-15 22:57:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 23:28:07]

Description was changed from

==========
[MD settings][cros accounts] Updates the accounts page according to the specs

BUG=598042, 563928

1. checkbox unchecked state:
https://screenshot.googleplex.com/dkZsoRT2TFx
2. checkbox checked state:
https://screenshot.googleplex.com/Qt4JzHLWQnN

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings][cros accounts] Updates the accounts page according to the specs

BUG=598042, 563928

1. checkbox unchecked state:
https://screenshot.googleplex.com/dkZsoRT2TFx
2. checkbox checked state:
https://screenshot.googleplex.com/Qt4JzHLWQnN

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-11-15 23:28:08]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-11-15 23:32:25]

Description was changed from

==========
[MD settings][cros accounts] Updates the accounts page according to the specs

BUG=598042, 563928

1. checkbox unchecked state:
https://screenshot.googleplex.com/dkZsoRT2TFx
2. checkbox checked state:
https://screenshot.googleplex.com/Qt4JzHLWQnN

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[MD settings][cros accounts] Updates the accounts page according to the specs

BUG=598042, 563928

1. checkbox unchecked state:
https://screenshot.googleplex.com/dkZsoRT2TFx
2. checkbox checked state:
https://screenshot.googleplex.com/Qt4JzHLWQnN

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/af59c5177d3a4012d39e3ea1471ab865b309f9fe
Cr-Commit-Position: refs/heads/master@{#432287}
==========

[commit-bot: I haz the power, 2016-11-15 23:32:27]

Patchset 3 (id:??) landed as
https://crrev.com/af59c5177d3a4012d39e3ea1471ab865b309f9fe
Cr-Commit-Position: refs/heads/master@{#432287}