[MD Settings][MD User Manager] create/manage profile desktop shortcut (Windows only)

[MD Settings][MD User Manager] create/manage profile desktop shortcut (Windows only)

1. Adds a "Create a desktop shortcut for this user" checkbox to the "Add person" flow in the new MD user manager.
https://screenshot.googleplex.com/m0DJQ7EYaFq

2. Adds a toggle in the "Edit person" subpage in MD settings to allow users to either remove an existing desktop shortcut or add a new desktop shortcut for an existing profile.
https://screenshot.googleplex.com/3EMny94D7z0

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

Committed: https://crrev.com/528aca2484f31bcf04808c18ab7147eceaf6cc99
Cr-Commit-Position: refs/heads/master@{#432218}

[Moe, 2016-11-14 23:33:15]

Description was changed from

==========
[MD Settings][MD User Manager] create/manage desktop shortcut (window only)

BUG=597984
==========

to

==========
[MD Settings][MD User Manager] create/manage desktop shortcut (window only)

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

[Moe, 2016-11-14 23:33:33]

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

[commit-bot: I haz the power, 2016-11-14 23:34:23]

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 23:35:07]

Description was changed from

==========
[MD Settings][MD User Manager] create/manage desktop shortcut (window only)

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

to

==========
[MD Settings][MD User Manager] create/manage desktop shortcut (window only)

BUG=597984

screenshots:
user manager: https://screenshot.googleplex.com/m0DJQ7EYaFq
settings: https://screenshot.googleplex.com/3EMny94D7z0

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

[Moe, 2016-11-14 23:35:28]

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

[Moe, 2016-11-14 23:35:28]

Hi Tommy, Please review this CL.

[commit-bot: I haz the power, 2016-11-15 00:43:10]

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

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

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[tommycli, 2016-11-15 01:59:50]

lgtm except below comments.

Would also be good to expand CL description and change title to (Windows only)
thanks!

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

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:98: value: false
Can you double check on Windows whether it is indeed false by default? I thought
I remembered it being true by default:

https://www.youtube.com/watch?v=QHRVHL_Ts9Y&t=1m40s

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:396:
supervisedUser.name, supervisedUser.iconURL, false, true,
Maybe inline comment the param names...

false /* createShortcut */, true /* isSupervised */

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/manage_profile.html (right):

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/manage_profile.html:25: <div
class="settings-box first" hidden="[[!isProfileShortcutsEnabled_]]">
consider just dom-ifing this out instead of hiding.

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/manage_profile.js (right):

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/manage_profile.js:90: }
It looks like the UI state won't be synchronized across multiple tabs, if you
open this subpage in multiple tabs.

But it looks like it wasn't synchronized in the old version too. Can you confirm
on your windows box? If it's no worse than before, it's fine.

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/people_page/manage_profile_browser_proxy.js
(right):

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/manage_profile_browser_proxy.js:31:
hasProfileShortcut: function() {},
Since this is an async getter, maybe change this to getHasProfileShortcut.

https://codereview.chromium.org/2498153002/diff/1/chrome/test/data/webui/md_u...
File chrome/test/data/webui/md_user_manager/create_profile_tests.js (right):

https://codereview.chromium.org/2498153002/diff/1/chrome/test/data/webui/md_u...
chrome/test/data/webui/md_user_manager/create_profile_tests.js:457:
teardown(function(done) {
Somewhat beyond the scope of this patch: But why is it important to remove the
element and wait for async tasks?

In settings tests, the whole body is cleared and the element is appended on
setup:
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/people_p...

Not a deal breaker, just curious

https://codereview.chromium.org/2498153002/diff/1/chrome/test/data/webui/md_u...
chrome/test/data/webui/md_user_manager/create_profile_tests.js:470:
assertFalse(createShortcutCheckbox.checked);
I am surprised it's unchecked by default, as stated in prev. comment

https://codereview.chromium.org/2498153002/diff/1/chrome/test/data/webui/md_u...
chrome/test/data/webui/md_user_manager/create_profile_tests.js:509: });
Nice test!

[Moe, 2016-11-15 15:17:23]

Description was changed from

==========
[MD Settings][MD User Manager] create/manage desktop shortcut (window only)

BUG=597984

screenshots:
user manager: https://screenshot.googleplex.com/m0DJQ7EYaFq
settings: https://screenshot.googleplex.com/3EMny94D7z0

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

to

==========
[MD Settings][MD User Manager] create/manage profile desktop shortcut (Windows
only)

BUG=597984

screenshots:
user manager: https://screenshot.googleplex.com/m0DJQ7EYaFq
settings: https://screenshot.googleplex.com/3EMny94D7z0

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

[Moe, 2016-11-15 15:59:47]

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:00:10]

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-15 16:01:25]

Description was changed from

==========
[MD Settings][MD User Manager] create/manage profile desktop shortcut (Windows
only)

BUG=597984

screenshots:
user manager: https://screenshot.googleplex.com/m0DJQ7EYaFq
settings: https://screenshot.googleplex.com/3EMny94D7z0

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

to

==========
[MD Settings][MD User Manager] create/manage profile desktop shortcut (Windows
only)

1. Adds a "Create a desktop shortcut for this user" checkbox to the "Add person"
flow in the new MD user manager.
2. Adds a toggle in the "Edit person" subpage in MD settings to allow users to
either remove an existing desktop shortcut or add a new desktop shortcut for an
existing profile.


BUG=597984

screenshots:
user manager: https://screenshot.googleplex.com/m0DJQ7EYaFq
settings: https://screenshot.googleplex.com/3EMny94D7z0

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

[Moe, 2016-11-15 16:01:58]

Description was changed from

==========
[MD Settings][MD User Manager] create/manage profile desktop shortcut (Windows
only)

1. Adds a "Create a desktop shortcut for this user" checkbox to the "Add person"
flow in the new MD user manager.
2. Adds a toggle in the "Edit person" subpage in MD settings to allow users to
either remove an existing desktop shortcut or add a new desktop shortcut for an
existing profile.


BUG=597984

screenshots:
user manager: https://screenshot.googleplex.com/m0DJQ7EYaFq
settings: https://screenshot.googleplex.com/3EMny94D7z0

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

to

==========
[MD Settings][MD User Manager] create/manage profile desktop shortcut (Windows
only)

1. Adds a "Create a desktop shortcut for this user" checkbox to the "Add person"
flow in the new MD user manager. https://screenshot.googleplex.com/m0DJQ7EYaFq

2. Adds a toggle in the "Edit person" subpage in MD settings to allow users to
either remove an existing desktop shortcut or add a new desktop shortcut for an
existing profile. https://screenshot.googleplex.com/3EMny94D7z0

BUG=597984

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

[Moe, 2016-11-15 16:02:24]

Description was changed from

==========
[MD Settings][MD User Manager] create/manage profile desktop shortcut (Windows
only)

1. Adds a "Create a desktop shortcut for this user" checkbox to the "Add person"
flow in the new MD user manager. https://screenshot.googleplex.com/m0DJQ7EYaFq

2. Adds a toggle in the "Edit person" subpage in MD settings to allow users to
either remove an existing desktop shortcut or add a new desktop shortcut for an
existing profile. https://screenshot.googleplex.com/3EMny94D7z0

BUG=597984

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

to

==========
[MD Settings][MD User Manager] create/manage profile desktop shortcut (Windows
only)

1. Adds a "Create a desktop shortcut for this user" checkbox to the "Add person"
flow in the new MD user manager.
https://screenshot.googleplex.com/m0DJQ7EYaFq

2. Adds a toggle in the "Edit person" subpage in MD settings to allow users to
either remove an existing desktop shortcut or add a new desktop shortcut for an
existing profile.
https://screenshot.googleplex.com/3EMny94D7z0

BUG=597984

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

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

Thanks for the super quick review Tommy.

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

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:98: value: false
On 2016/11/15 01:59:50, tommycli wrote:
> Can you double check on Windows whether it is indeed false by default? I
thought
> I remembered it being true by default:
> 
> https://www.youtube.com/watch?v=QHRVHL_Ts9Y&t=1m40s

You're right. It has to be true by default.

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/md...
chrome/browser/resources/md_user_manager/create_profile.js:396:
supervisedUser.name, supervisedUser.iconURL, false, true,
On 2016/11/15 01:59:50, tommycli wrote:
> Maybe inline comment the param names...
> 
> false /* createShortcut */, true /* isSupervised */

Done.

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/manage_profile.html (right):

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/manage_profile.html:25: <div
class="settings-box first" hidden="[[!isProfileShortcutsEnabled_]]">
On 2016/11/15 01:59:50, tommycli wrote:
> consider just dom-ifing this out instead of hiding.

Done.

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/manage_profile.js (right):

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/manage_profile.js:90: }
On 2016/11/15 01:59:50, tommycli wrote:
> It looks like the UI state won't be synchronized across multiple tabs, if you
> open this subpage in multiple tabs.
> 
> But it looks like it wasn't synchronized in the old version too. Can you
confirm
> on your windows box? If it's no worse than before, it's fine.

I doesn't seem to be synced in the old settings page either. so i guess we're
good. phew...

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/se...
File
chrome/browser/resources/settings/people_page/manage_profile_browser_proxy.js
(right):

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/manage_profile_browser_proxy.js:31:
hasProfileShortcut: function() {},
On 2016/11/15 01:59:50, tommycli wrote:
> Since this is an async getter, maybe change this to getHasProfileShortcut.

Done.

https://codereview.chromium.org/2498153002/diff/1/chrome/test/data/webui/md_u...
File chrome/test/data/webui/md_user_manager/create_profile_tests.js (right):

https://codereview.chromium.org/2498153002/diff/1/chrome/test/data/webui/md_u...
chrome/test/data/webui/md_user_manager/create_profile_tests.js:457:
teardown(function(done) {
On 2016/11/15 01:59:50, tommycli wrote:
> Somewhat beyond the scope of this patch: But why is it important to remove the
> element and wait for async tasks?
> 
> In settings tests, the whole body is cleared and the element is appended on
> setup:
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/people_p...
> 
> Not a deal breaker, just curious

PolymerTest.clearBody() should eliminate the need to remove the element at the
end of the test. But shouldn't it be done at teardown instead of setup? It's
possible to have a behavior on detach that throws an error.
As for allowing async tasks to finish, it was michaelpg@'s idea to add this to
the user manager tests. I don't fully remember the context, but i think there
was an error in an async task that would appear in the logs as if it was
happening in the next tests. By adding this line, we were able to see where the
error was actually happening.

https://codereview.chromium.org/2498153002/diff/1/chrome/test/data/webui/md_u...
chrome/test/data/webui/md_user_manager/create_profile_tests.js:470:
assertFalse(createShortcutCheckbox.checked);
On 2016/11/15 01:59:50, tommycli wrote:
> I am surprised it's unchecked by default, as stated in prev. comment

Done.

[tommycli, 2016-11-15 16:18:37]

thanks! lgtm

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/people_page/manage_profile.js (right):

https://codereview.chromium.org/2498153002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/manage_profile.js:90: }
On 2016/11/15 16:03:10, moe wrote:
> On 2016/11/15 01:59:50, tommycli wrote:
> > It looks like the UI state won't be synchronized across multiple tabs, if
you
> > open this subpage in multiple tabs.
> > 
> > But it looks like it wasn't synchronized in the old version too. Can you
> confirm
> > on your windows box? If it's no worse than before, it's fine.
> 
> I doesn't seem to be synced in the old settings page either. so i guess we're
> good. phew...

We're "good". hehe. That being said, I think it's not that big of a deal.

https://codereview.chromium.org/2498153002/diff/1/chrome/test/data/webui/md_u...
File chrome/test/data/webui/md_user_manager/create_profile_tests.js (right):

https://codereview.chromium.org/2498153002/diff/1/chrome/test/data/webui/md_u...
chrome/test/data/webui/md_user_manager/create_profile_tests.js:457:
teardown(function(done) {
On 2016/11/15 16:03:10, moe wrote:
> On 2016/11/15 01:59:50, tommycli wrote:
> > Somewhat beyond the scope of this patch: But why is it important to remove
the
> > element and wait for async tasks?
> > 
> > In settings tests, the whole body is cleared and the element is appended on
> > setup:
> >
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/people_p...
> > 
> > Not a deal breaker, just curious
> 
> PolymerTest.clearBody() should eliminate the need to remove the element at the
> end of the test. But shouldn't it be done at teardown instead of setup? It's
> possible to have a behavior on detach that throws an error.
> As for allowing async tasks to finish, it was michaelpg@'s idea to add this to
> the user manager tests. I don't fully remember the context, but i think there
> was an error in an async task that would appear in the logs as if it was
> happening in the next tests. By adding this line, we were able to see where
the
> error was actually happening.

Ah okay! That is a very good explanation. Thanks for the education!

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

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

[commit-bot: I haz the power, 2016-11-15 17:28:12]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Moe, 2016-11-15 17:30:32]

The CQ bit was checked by mahmadi@chromium.org

[commit-bot: I haz the power, 2016-11-15 17:30:57]

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 18:29:21]

Description was changed from

==========
[MD Settings][MD User Manager] create/manage profile desktop shortcut (Windows
only)

1. Adds a "Create a desktop shortcut for this user" checkbox to the "Add person"
flow in the new MD user manager.
https://screenshot.googleplex.com/m0DJQ7EYaFq

2. Adds a toggle in the "Edit person" subpage in MD settings to allow users to
either remove an existing desktop shortcut or add a new desktop shortcut for an
existing profile.
https://screenshot.googleplex.com/3EMny94D7z0

BUG=597984

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

to

==========
[MD Settings][MD User Manager] create/manage profile desktop shortcut (Windows
only)

1. Adds a "Create a desktop shortcut for this user" checkbox to the "Add person"
flow in the new MD user manager.
https://screenshot.googleplex.com/m0DJQ7EYaFq

2. Adds a toggle in the "Edit person" subpage in MD settings to allow users to
either remove an existing desktop shortcut or add a new desktop shortcut for an
existing profile.
https://screenshot.googleplex.com/3EMny94D7z0

BUG=597984

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

[commit-bot: I haz the power, 2016-11-15 18:29:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-15 18:34:30]

Description was changed from

==========
[MD Settings][MD User Manager] create/manage profile desktop shortcut (Windows
only)

1. Adds a "Create a desktop shortcut for this user" checkbox to the "Add person"
flow in the new MD user manager.
https://screenshot.googleplex.com/m0DJQ7EYaFq

2. Adds a toggle in the "Edit person" subpage in MD settings to allow users to
either remove an existing desktop shortcut or add a new desktop shortcut for an
existing profile.
https://screenshot.googleplex.com/3EMny94D7z0

BUG=597984

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

to

==========
[MD Settings][MD User Manager] create/manage profile desktop shortcut (Windows
only)

1. Adds a "Create a desktop shortcut for this user" checkbox to the "Add person"
flow in the new MD user manager.
https://screenshot.googleplex.com/m0DJQ7EYaFq

2. Adds a toggle in the "Edit person" subpage in MD settings to allow users to
either remove an existing desktop shortcut or add a new desktop shortcut for an
existing profile.
https://screenshot.googleplex.com/3EMny94D7z0

BUG=597984

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/528aca2484f31bcf04808c18ab7147eceaf6cc99
Cr-Commit-Position: refs/heads/master@{#432218}
==========

[commit-bot: I haz the power, 2016-11-15 18:34:32]

Patchset 2 (id:??) landed as
https://crrev.com/528aca2484f31bcf04808c18ab7147eceaf6cc99
Cr-Commit-Position: refs/heads/master@{#432218}