MD Settings: Move easy unlock from people to lock screen.

MD Settings: Move easy unlock from people to lock screen.

Move easy unlock from people to lock screen. Also need to hide it from options, once it is moved.

BUG=703998
TEST=browser_tests --gtest_filter="SettingsEasyUnlockBrowserTest*"
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2787153002
Cr-Commit-Position: refs/heads/master@{#464172}
Committed: https://chromium.googlesource.com/chromium/src/+/598efef49cb88d56172cf5d69f7cccde5cbbc4bb

[sammiequon, 2017-03-30 21:39:11]

Description was changed from

==========
MD Settings: Move easy unlock from people to lock screen.


BUG=
TEST=none
==========

to

==========
MD Settings: Move easy unlock from people to lock screen.


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

[sammiequon, 2017-04-03 22:52:15]

Description was changed from

==========
MD Settings: Move easy unlock from people to lock screen.


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

to

==========
MD Settings: Move easy unlock from people to lock screen.

Move easy unlock from people to lock screen. Also need to hide it from options,
once it is moved.

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

[sammiequon, 2017-04-03 22:56:00]

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

[commit-bot: I haz the power, 2017-04-03 22:56:43]

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

[sammiequon, 2017-04-03 23:09:40]

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

[commit-bot: I haz the power, 2017-04-03 23:10:03]

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, 2017-04-04 00:35:19]

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

[commit-bot: I haz the power, 2017-04-04 00:35:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sammiequon, 2017-04-04 16:52:47]

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

[commit-bot: I haz the power, 2017-04-04 16:53:13]

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

[sammiequon, 2017-04-04 16:53:57]

Description was changed from

==========
MD Settings: Move easy unlock from people to lock screen.

Move easy unlock from people to lock screen. Also need to hide it from options,
once it is moved.

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

to

==========
MD Settings: Move easy unlock from people to lock screen.

Move easy unlock from people to lock screen. Also need to hide it from options,
once it is moved.

BUG=
TEST=browser_tests --gtest_filter="SettingsEasyUnlockBrowserTest*"
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[sammiequon, 2017-04-04 16:54:32]

Description was changed from

==========
MD Settings: Move easy unlock from people to lock screen.

Move easy unlock from people to lock screen. Also need to hide it from options,
once it is moved.

BUG=
TEST=browser_tests --gtest_filter="SettingsEasyUnlockBrowserTest*"
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Move easy unlock from people to lock screen.

Move easy unlock from people to lock screen. Also need to hide it from options,
once it is moved.

BUG=703998
TEST=browser_tests --gtest_filter="SettingsEasyUnlockBrowserTest*"
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2017-04-04 18:10:37]

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

[commit-bot: I haz the power, 2017-04-04 18:10:38]

Dry run: This issue passed the CQ dry run.

[sammiequon, 2017-04-04 19:01:11]

sammiequon@chromium.org changed reviewers:
+ jdufault@chromium.org

[sammiequon, 2017-04-04 19:02:46]

jdufault@ - Please take a look. Thanks!

[jdufault, 2017-04-06 21:04:45]

lgtm, most of the code was just moved and not changed, right?

https://codereview.chromium.org/2787153002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/lock_screen.html (right):

https://codereview.chromium.org/2787153002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/lock_screen.html:25: #easy-unlock
.start {
nit: use easyUnlock for id name since that's what everything else in the file
uses.

https://codereview.chromium.org/2787153002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/lock_screen.js (right):

https://codereview.chromium.org/2787153002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/lock_screen.js:291:
e.preventDefault();
Can the preventDefault be removed? Looks like this was copied but we should
figure out why it is there and add a comment.

[sammiequon, 2017-04-06 22:23:51]

Thanks! Just moved, plus a couple tweaks (options, tests) to make things work.

https://codereview.chromium.org/2787153002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/lock_screen.html (right):

https://codereview.chromium.org/2787153002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/lock_screen.html:25: #easy-unlock
.start {
On 2017/04/06 21:04:45, jdufault wrote:
> nit: use easyUnlock for id name since that's what everything else in the file
> uses.

Done.

https://codereview.chromium.org/2787153002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/lock_screen.js (right):

https://codereview.chromium.org/2787153002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/lock_screen.js:291:
e.preventDefault();
On 2017/04/06 21:04:45, jdufault wrote:
> Can the preventDefault be removed? Looks like this was copied but we should
> figure out why it is there and add a comment.

Seems like this is settings-wide thing explicitly added in
https://codereview.chromium.org/2557073003. Do we still need a comment?

[jdufault, 2017-04-06 22:31:45]

https://codereview.chromium.org/2787153002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/lock_screen.js (right):

https://codereview.chromium.org/2787153002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/lock_screen.js:291:
e.preventDefault();
On 2017/04/06 22:23:50, sammiequon wrote:
> On 2017/04/06 21:04:45, jdufault wrote:
> > Can the preventDefault be removed? Looks like this was copied but we should
> > figure out why it is there and add a comment.
> 
> Seems like this is settings-wide thing explicitly added in
> https://codereview.chromium.org/2557073003. Do we still need a comment?

It if clearly breaks w/o e.preventDefault() no comment, but if the breakage is
subtle please add one.

[sammiequon, 2017-04-06 23:36:05]

sammiequon@chromium.org changed reviewers:
+ stevenjb@chromium.org

[sammiequon, 2017-04-06 23:36:28]

stevenjb@ - Please take a look. Thanks!

https://codereview.chromium.org/2787153002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/lock_screen.js (right):

https://codereview.chromium.org/2787153002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/lock_screen.js:291:
e.preventDefault();
On 2017/04/06 22:31:44, jdufault wrote:
> On 2017/04/06 22:23:50, sammiequon wrote:
> > On 2017/04/06 21:04:45, jdufault wrote:
> > > Can the preventDefault be removed? Looks like this was copied but we
should
> > > figure out why it is there and add a comment.
> > 
> > Seems like this is settings-wide thing explicitly added in
> > https://codereview.chromium.org/2557073003. Do we still need a comment?
> 
> It if clearly breaks w/o e.preventDefault() no comment, but if the breakage is
> subtle please add one.

Done.

[jdufault, 2017-04-06 23:40:45]

https://codereview.chromium.org/2787153002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.js (right):

https://codereview.chromium.org/2787153002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.js:291: // Prevent the
end of the tap event to focus what is underneath it as the
I'm confused by what this means

"to focus what is underneath it as the dialog comes up" doesn't have a subject."

should 'to' be 'from'?

// Prevent the tap event from focusing what is under the dialog.

[sammiequon, 2017-04-06 23:56:15]

https://codereview.chromium.org/2787153002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.js (right):

https://codereview.chromium.org/2787153002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.js:291: // Prevent the
end of the tap event to focus what is underneath it as the
On 2017/04/06 23:40:44, jdufault wrote:
> I'm confused by what this means
> 
> "to focus what is underneath it as the dialog comes up" doesn't have a
subject."
> 
> should 'to' be 'from'?
> 
> // Prevent the tap event from focusing what is under the dialog.

Done.

[stevenjb, 2017-04-07 17:54:19]

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.html (right):

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:27: padding: 11px
0;
We try to avoid this sort of thing as much as possible. Could you clarify the
comment some? Also, does the padding need to be top and bottom? We probably also
shouldn't override the horizontal padding (i.e. use top and or bottom
explicitly). Can we use a value from settings_shared_css.html?

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:96: </template>
Instead of having two templated sections here, just do:

[[getEasyUnlockDescription_(easyUnlockEnabled_,
'$i18nPolymer{easyUnlockSetupIntro}', '$i18nPolymer{easyUnlockDescription}')]]

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:110: </template>
Let's clean this up as Dan suggested:

if="[[getShowEasyUnlockToggle(easyUnlockEnabled_,
easyUnlockProximityDetectionAllowed_)]]"

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.js (right):

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.js:94: },
This doesn't look like it needs to be a property? We should set it up
consistently with fingerprintBrowserProxy_.

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.js:299:
this.showEasyUnlockTurnOffDialog_ = false;
We should restore focus to the dialog button on close, see crbug.com/709148 for
an example CL.

[sammiequon, 2017-04-11 00:28:45]

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.html (right):

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:27: padding: 11px
0;
On 2017/04/07 17:54:18, stevenjb wrote:
> We try to avoid this sort of thing as much as possible. Could you clarify the
> comment some? Also, does the padding need to be top and bottom? We probably
also
> shouldn't override the horizontal padding (i.e. use top and or bottom
> explicitly). Can we use a value from settings_shared_css.html?

I'm not entirely sure what this does. I played around with removing it, but I
can't really see a difference. It was added here
https://codereview.chromium.org/2482553002, but maybe its obsolete now?

Ok, I see what it does now, when there's too much element inside .start (when
the easy unlock toggle is shown), the text gets squashed up to the top, so this
11px emulates what happens in regular .start divs, which have a space between
the text and bounds (because they usually only have ore or two lines of text).

I didn't see anything in shared settings with top bottom padding. Would this
comment suffice?

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:96: </template>
On 2017/04/07 17:54:18, stevenjb wrote:
> Instead of having two templated sections here, just do:
> 
> [[getEasyUnlockDescription_(easyUnlockEnabled_,
> '$i18nPolymer{easyUnlockSetupIntro}', '$i18nPolymer{easyUnlockDescription}')]]

Done.

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:110: </template>
On 2017/04/07 17:54:18, stevenjb wrote:
> Let's clean this up as Dan suggested:
> 
> if="[[getShowEasyUnlockToggle(easyUnlockEnabled_,
> easyUnlockProximityDetectionAllowed_)]]"

Done.

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.js (right):

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.js:94: },
On 2017/04/07 17:54:19, stevenjb wrote:
> This doesn't look like it needs to be a property? We should set it up
> consistently with fingerprintBrowserProxy_.

Done.

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.js:299:
this.showEasyUnlockTurnOffDialog_ = false;
On 2017/04/07 17:54:19, stevenjb wrote:
> We should restore focus to the dialog button on close, see crbug.com/709148
for
> an example CL.

Done.

[stevenjb, 2017-04-11 17:29:38]

lgtm w/ nits

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.html (right):

https://codereview.chromium.org/2787153002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:27: padding: 11px
0;
On 2017/04/11 00:28:44, sammiequon wrote:
> On 2017/04/07 17:54:18, stevenjb wrote:
> > We try to avoid this sort of thing as much as possible. Could you clarify
the
> > comment some? Also, does the padding need to be top and bottom? We probably
> also
> > shouldn't override the horizontal padding (i.e. use top and or bottom
> > explicitly). Can we use a value from settings_shared_css.html?
> 
> I'm not entirely sure what this does. I played around with removing it, but I
> can't really see a difference. It was added here
> https://codereview.chromium.org/2482553002, but maybe its obsolete now?
> 
> Ok, I see what it does now, when there's too much element inside .start (when
> the easy unlock toggle is shown), the text gets squashed up to the top, so
this
> 11px emulates what happens in regular .start divs, which have a space between
> the text and bounds (because they usually only have ore or two lines of text).
> 
> I didn't see anything in shared settings with top bottom padding. Would this
> comment suffice?

Ah. Yeah, in that context I gave the benefit of the doubt that it was necessary.
The improved comment helps, thanks.

https://codereview.chromium.org/2787153002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.html (right):

https://codereview.chromium.org/2787153002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:95:
'$i18nPolymer{easyUnlockSetupIntro}')]]
nit: indent the two lines above 4 spaces

https://codereview.chromium.org/2787153002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.js (right):

https://codereview.chromium.org/2787153002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.js:37: setModes_:
{type: Object, observer: 'onSetModesChanged_'},
nit: You can force multiple lines here (which I prefer) by adding a trailing
comma after the last entry.

https://codereview.chromium.org/2787153002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.js:308: return
disabledStr;
nit: return enabled ? enabledStr : disabledStr;

https://codereview.chromium.org/2787153002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.js:312: * @param
{boolean} enabled
nit: easyUnlockEnabled (here and below)

[sammiequon, 2017-04-11 17:57:11]

Thanks!

https://codereview.chromium.org/2787153002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.html (right):

https://codereview.chromium.org/2787153002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.html:95:
'$i18nPolymer{easyUnlockSetupIntro}')]]
On 2017/04/11 17:29:37, stevenjb wrote:
> nit: indent the two lines above 4 spaces

Done.

https://codereview.chromium.org/2787153002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/people_page/lock_screen.js (right):

https://codereview.chromium.org/2787153002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.js:37: setModes_:
{type: Object, observer: 'onSetModesChanged_'},
On 2017/04/11 17:29:38, stevenjb wrote:
> nit: You can force multiple lines here (which I prefer) by adding a trailing
> comma after the last entry.

Done.

https://codereview.chromium.org/2787153002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.js:308: return
disabledStr;
On 2017/04/11 17:29:37, stevenjb wrote:
> nit: return enabled ? enabledStr : disabledStr;

Done.

https://codereview.chromium.org/2787153002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/lock_screen.js:312: * @param
{boolean} enabled
On 2017/04/11 17:29:38, stevenjb wrote:
> nit: easyUnlockEnabled (here and below)
> 

Done.

[sammiequon, 2017-04-11 21:24:30]

The CQ bit was checked by sammiequon@chromium.org

[sammiequon, 2017-04-11 21:24:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
jdufault@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2787153002/#ps160001
(title: "Fixed patch set 8 errors.")

[commit-bot: I haz the power, 2017-04-11 21:25:10]

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, 2017-04-11 21:51:57]

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

[commit-bot: I haz the power, 2017-04-11 21:51:58]

Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[sammiequon, 2017-04-12 05:03:59]

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

[commit-bot: I haz the power, 2017-04-12 05:04:12]

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, 2017-04-12 06:04:17]

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

[commit-bot: I haz the power, 2017-04-12 06:04:18]

Dry run: This issue passed the CQ dry run.

[sammiequon, 2017-04-12 20:53:03]

The CQ bit was checked by sammiequon@chromium.org

[sammiequon, 2017-04-12 20:53:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
jdufault@chromium.org, stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2787153002/#ps180001
(title: "Closure.")

[commit-bot: I haz the power, 2017-04-12 20:53:46]

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, 2017-04-12 22:15:17]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492030383977100,
"parent_rev": "b452deb2b1802b68bf9662308074e4b10904f709", "commit_rev":
"d604829444fd37a2f3394551d60573d2ddf351cd"}

[commit-bot: I haz the power, 2017-04-12 22:21:26]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492030383977100,
"parent_rev": "e709567885e3c8133d1e5260df3b653c329b7d55", "commit_rev":
"598efef49cb88d56172cf5d69f7cccde5cbbc4bb"}

[commit-bot: I haz the power, 2017-04-12 22:22:31]

Description was changed from

==========
MD Settings: Move easy unlock from people to lock screen.

Move easy unlock from people to lock screen. Also need to hide it from options,
once it is moved.

BUG=703998
TEST=browser_tests --gtest_filter="SettingsEasyUnlockBrowserTest*"
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Move easy unlock from people to lock screen.

Move easy unlock from people to lock screen. Also need to hide it from options,
once it is moved.

BUG=703998
TEST=browser_tests --gtest_filter="SettingsEasyUnlockBrowserTest*"
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2787153002
Cr-Commit-Position: refs/heads/master@{#464172}
Committed:
https://chromium.googlesource.com/chromium/src/+/598efef49cb88d56172cf5d69f7c...
==========

[commit-bot: I haz the power, 2017-04-12 22:22:42]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/598efef49cb88d56172cf5d69f7c...