Add settings screen that lets a user setup a PIN to unlock their device with.

Add settings screen that lets a user setup a PIN to unlock their device with.

When a PIN is enabled, when the user locks their device (but *only* when they
are signed in), the lockscreen will show a PIN keyboard they can use to
authenticate against.

The setup-pin screen is accessed from the choose method screen (added in a
dependency CL). It is available from the /quickUnlock/setupPin endpoint.

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

Committed: https://crrev.com/67cc9c5013f62aeb24b87a3a513bd590cdfc186c
Cr-Commit-Position: refs/heads/master@{#405348}

[jdufault, 2016-06-23 22:19:05]

Description was changed from

==========
Add settings screen that lets a user setup a PIN to unlock their device with.

When a PIN is enabled, when the user locks their device (but *only* when they
are signed in), the lockscreen will show a PIN keyboard they can use to
authenticate against.

The setup-pin screen is accessed from the choose method screen (added in a
dependency CL). It is available from the /quickUnlock/setupPin endpoint.

BUG=603217
==========

to

==========
Add settings screen that lets a user setup a PIN to unlock their device with.

When a PIN is enabled, when the user locks their device (but *only* when they
are signed in), the lockscreen will show a PIN keyboard they can use to
authenticate against.

The setup-pin screen is accessed from the choose method screen (added in a
dependency CL). It is available from the /quickUnlock/setupPin endpoint.

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

[jdufault, 2016-06-23 22:20:38]

Description was changed from

==========
Add settings screen that lets a user setup a PIN to unlock their device with.

When a PIN is enabled, when the user locks their device (but *only* when they
are signed in), the lockscreen will show a PIN keyboard they can use to
authenticate against.

The setup-pin screen is accessed from the choose method screen (added in a
dependency CL). It is available from the /quickUnlock/setupPin endpoint.

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

to

==========
Add settings screen that lets a user setup a PIN to unlock their device with.

When a PIN is enabled, when the user locks their device (but *only* when they
are signed in), the lockscreen will show a PIN keyboard they can use to
authenticate against.

The setup-pin screen is accessed from the choose method screen (added in a
dependency CL). It is available from the /quickUnlock/setupPin endpoint.

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

[jdufault, 2016-06-23 22:20:38]

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

[jdufault, 2016-06-23 22:20:49]

Tommy, PTAL. Thanks!

[tommycli, 2016-07-13 19:31:12]

some initial comments

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.html
(right):

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.html:15:
margin-left: 10px;
We already have --icon-icon-spacing used like this:
https://cs.chromium.org/search/?q=iron-icon-spacing&sq=package:chromium&type=cs

      iron-icon {
        -webkit-margin-end: var(--iron-icon-spacing);
      }

Does that work for you? If not, I'd question why this is 10px and everywhere
else in settings uses the standard 16px spacing.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js
(right):

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:24: var
QuickUnlockSetupPinStep = {
I think the confirm/choose stuff could be simplified if it was just a boolean
onConfirmStep_ that implies it's on the choose step if false.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:35: var
QuickUnlockSetupPinProblemType = {
If there's not going to be more than 3 errors, I think this extra level of
indirection is unneeded. Perhaps just do: postMessage(messageId, problemClass)?

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:65: var
SHOW_PROBLEM_DELAY_IN_MS = 500;
Is the delay mechanism a requirement? Because other places in Polymer seem to do
"instant" feedback when the user types something invalid.

See the paper-input validation demos:
https://elements.polymer-project.org/elements/paper-input?view=demo:demo/inde...

Less is more imo

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:70:
behaviors: [
formatting seems odd here

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:91:
titleMessage_: String,
Instead of a message variable, how about two dom-if if="onConfirmStep_" in the
HTML -- or alternatively a computed binding i.e.
getTitleMessage(isConfirmStep_). Less state better.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:97:
continueMessage_: String,
Ditto here.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:103:
showProblem_: Boolean,
I think you can axe this boolean and just make interpret an empty-string or null
problemMessage_ as not showing any problem.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:224:
setTimeout(this.updateStyles.bind(this));
Hmm... does it not work without this call? I've never seen this before, and I've
seen dynamic css classes before.

[jdufault, 2016-07-13 20:38:13]

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.html
(right):

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.html:15:
margin-left: 10px;
On 2016/07/13 19:31:11, tommycli wrote:
> We already have --icon-icon-spacing used like this:
>
https://cs.chromium.org/search/?q=iron-icon-spacing&sq=package:chromium&type=cs
> 
>       iron-icon {
>         -webkit-margin-end: var(--iron-icon-spacing);
>       }
> 
> Does that work for you? If not, I'd question why this is 10px and everywhere
> else in settings uses the standard 16px spacing.

Done.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js
(right):

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:24: var
QuickUnlockSetupPinStep = {
On 2016/07/13 19:31:11, tommycli wrote:
> I think the confirm/choose stuff could be simplified if it was just a boolean
> onConfirmStep_ that implies it's on the choose step if false.

Done.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:35: var
QuickUnlockSetupPinProblemType = {
On 2016/07/13 19:31:12, tommycli wrote:
> If there's not going to be more than 3 errors, I think this extra level of
> indirection is unneeded. Perhaps just do: postMessage(messageId,
problemClass)?

I've done some renaming, but this makes the showProblem call really easy to
read.

  if (isWeak)
    showProblem(ProblemInfo.TOO_SHORT);

vs

  if (isWeak) {
    showProblem('quickUnlockConfigurePinChoosePinTooShort',
                'error');
  }

The second one also needs wrapping, which makes it even harder to read.

Despite the indirection, I think the readability is increased because the
surrounding logic is easier to parse.

If you feel strongly, I can remove the indirection.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:65: var
SHOW_PROBLEM_DELAY_IN_MS = 500;
On 2016/07/13 19:31:12, tommycli wrote:
> Is the delay mechanism a requirement? Because other places in Polymer seem to
do
> "instant" feedback when the user types something invalid.
> 
> See the paper-input validation demos:
>
https://elements.polymer-project.org/elements/paper-input?view=demo:demo/inde...
> 
> Less is more imo

Without the delay, as you type the PIN you'll immediately be warned it has to be
4 digits or more.

I've removed the delay.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:70:
behaviors: [
On 2016/07/13 19:31:12, tommycli wrote:
> formatting seems odd here

I've added a newline. I'm not sure otherwise what you mean.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:91:
titleMessage_: String,
On 2016/07/13 19:31:11, tommycli wrote:
> Instead of a message variable, how about two dom-if if="onConfirmStep_" in the
> HTML -- or alternatively a computed binding i.e.
> getTitleMessage(isConfirmStep_). Less state better.

Done, I like this better.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:97:
continueMessage_: String,
On 2016/07/13 19:31:11, tommycli wrote:
> Ditto here.

Done.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:103:
showProblem_: Boolean,
On 2016/07/13 19:31:12, tommycli wrote:
> I think you can axe this boolean and just make interpret an empty-string or
null
> problemMessage_ as not showing any problem.

Done.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:224:
setTimeout(this.updateStyles.bind(this));
On 2016/07/13 19:31:11, tommycli wrote:
> Hmm... does it not work without this call? I've never seen this before, and
I've
> seen dynamic css classes before.

The icon color is not updated without the call. I'm guessing iron-icon is doing
something special that makes the call necessary.

[tommycli, 2016-07-13 20:57:10]

Cool.. getting close. Some more CSS/HTML nits below. Some reduce reuse recycle
if at all possible.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js
(right):

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:35: var
QuickUnlockSetupPinProblemType = {
On 2016/07/13 20:38:13, jdufault wrote:
> On 2016/07/13 19:31:12, tommycli wrote:
> > If there's not going to be more than 3 errors, I think this extra level of
> > indirection is unneeded. Perhaps just do: postMessage(messageId,
> problemClass)?
> 
> I've done some renaming, but this makes the showProblem call really easy to
> read.
> 
>   if (isWeak)
>     showProblem(ProblemInfo.TOO_SHORT);
> 
> vs
> 
>   if (isWeak) {
>     showProblem('quickUnlockConfigurePinChoosePinTooShort',
>                 'error');
>   }
> 
> The second one also needs wrapping, which makes it even harder to read.
> 
> Despite the indirection, I think the readability is increased because the
> surrounding logic is easier to parse.
> 
> If you feel strongly, I can remove the indirection.

Okay, sounds good. Thanks for trying it.

https://codereview.chromium.org/2098663002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:224:
setTimeout(this.updateStyles.bind(this));
On 2016/07/13 20:38:13, jdufault wrote:
> On 2016/07/13 19:31:11, tommycli wrote:
> > Hmm... does it not work without this call? I've never seen this before, and
> I've
> > seen dynamic css classes before.
> 
> The icon color is not updated without the call. I'm guessing iron-icon is
doing
> something special that makes the call necessary.

Cool. Getting rid of the setTimeout makes it much cleaner anyways. Thanks for
trying it.

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

https://codereview.chromium.org/2098663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.html:14:
.icon-box > iron-icon {
Could we just eliminate the icon-box class and just make the css selector
iron-icon?

https://codereview.chromium.org/2098663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.html:23:
--iron-icon-fill-color: var(--paper-grey-500);
Does it work if the --iron-icon-fill-color rule is consolidated onto the
.warning rule? and same with the .error rule

https://codereview.chromium.org/2098663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.html:45:
<div class="settings-box" hidden$="[[hasProblem_(problemMessage_)]]">
I think we have an opportunity to get rid of the hasProblem_ method. Can we do
something similar to:

<div class="settings-box [[problemClass_]]">
  <template is="dom-if" if="[[!problemMessage_]]">
    <!-- title stuff -->
  </template>
  <template is="dom-if" if="[[problemMessage_]]">
    <!-- error message stuff -->
  </template>
</div>

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

https://codereview.chromium.org/2098663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:265:
computeTitleMessage_: function(isConfirmStep) {
nit: Although it violates the Polymer guide, most of the computed bindings in
Settings use the getTitleMessage_ format. This is optional, just a nitpick. Same
below

[jdufault, 2016-07-13 21:28:21]

Patchset #5 (id:80001) has been deleted

[jdufault, 2016-07-13 21:30:18]

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

https://codereview.chromium.org/2098663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.html:14:
.icon-box > iron-icon {
On 2016/07/13 20:57:10, tommycli wrote:
> Could we just eliminate the icon-box class and just make the css selector
> iron-icon?

Done.

https://codereview.chromium.org/2098663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.html:23:
--iron-icon-fill-color: var(--paper-grey-500);
On 2016/07/13 20:57:10, tommycli wrote:
> Does it work if the --iron-icon-fill-color rule is consolidated onto the
> .warning rule? and same with the .error rule

Nope :/

https://codereview.chromium.org/2098663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.html:45:
<div class="settings-box" hidden$="[[hasProblem_(problemMessage_)]]">
On 2016/07/13 20:57:10, tommycli wrote:
> I think we have an opportunity to get rid of the hasProblem_ method. Can we do
> something similar to:
> 
> <div class="settings-box [[problemClass_]]">
>   <template is="dom-if" if="[[!problemMessage_]]">
>     <!-- title stuff -->
>   </template>
>   <template is="dom-if" if="[[problemMessage_]]">
>     <!-- error message stuff -->
>   </template>
> </div>

I tried that, it works for the error message, but not for the title.

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

https://codereview.chromium.org/2098663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:265:
computeTitleMessage_: function(isConfirmStep) {
On 2016/07/13 20:57:10, tommycli wrote:
> nit: Although it violates the Polymer guide, most of the computed bindings in
> Settings use the getTitleMessage_ format. This is optional, just a nitpick.
Same
> below

Done.

[tommycli, 2016-07-13 21:56:26]

ok lgtm except for the last issue:

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

https://codereview.chromium.org/2098663002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.html:45:
<div class="settings-box" hidden$="[[hasProblem_(problemMessage_)]]">
On 2016/07/13 21:30:17, jdufault wrote:
> On 2016/07/13 20:57:10, tommycli wrote:
> > I think we have an opportunity to get rid of the hasProblem_ method. Can we
do
> > something similar to:
> > 
> > <div class="settings-box [[problemClass_]]">
> >   <template is="dom-if" if="[[!problemMessage_]]">
> >     <!-- title stuff -->
> >   </template>
> >   <template is="dom-if" if="[[problemMessage_]]">
> >     <!-- error message stuff -->
> >   </template>
> > </div>
> 
> I tried that, it works for the error message, but not for the title.

I guess !stringVar_ doesn't work... ok.

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

https://codereview.chromium.org/2098663002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:141: *
@private
@param and @return annotations plz

And same comment elsewhere that take parameters or returns things.

[jdufault, 2016-07-13 22:10:39]

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

https://codereview.chromium.org/2098663002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:141: *
@private
On 2016/07/13 21:56:26, tommycli wrote:
> @param and @return annotations plz
> 
> And same comment elsewhere that take parameters or returns things.

Done.

[tommycli, 2016-07-13 22:15:10]

lgtm. This should get tests before it goes to users, but otherwise it's looking
decent.

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

https://codereview.chromium.org/2098663002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:174:
return !!(pin && pin.length >= 4);
this seems odd.

Does !!pin && pin.length >=4 work?

[jdufault, 2016-07-13 22:18:39]

Thanks!

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

https://codereview.chromium.org/2098663002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/quick_unlock_setup_pin.js:174:
return !!(pin && pin.length >= 4);
On 2016/07/13 22:15:10, tommycli wrote:
> this seems odd.
> 
> Does !!pin && pin.length >=4 work?

Done.

[jdufault, 2016-07-13 22:18:42]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2016-07-13 22:18:43]

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/2098663002/#ps140001
(title: "Nit")

[commit-bot: I haz the power, 2016-07-13 22:20:15]

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

Description was changed from

==========
Add settings screen that lets a user setup a PIN to unlock their device with.

When a PIN is enabled, when the user locks their device (but *only* when they
are signed in), the lockscreen will show a PIN keyboard they can use to
authenticate against.

The setup-pin screen is accessed from the choose method screen (added in a
dependency CL). It is available from the /quickUnlock/setupPin endpoint.

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

to

==========
Add settings screen that lets a user setup a PIN to unlock their device with.

When a PIN is enabled, when the user locks their device (but *only* when they
are signed in), the lockscreen will show a PIN keyboard they can use to
authenticate against.

The setup-pin screen is accessed from the choose method screen (added in a
dependency CL). It is available from the /quickUnlock/setupPin endpoint.

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

[commit-bot: I haz the power, 2016-07-13 23:35:05]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-07-13 23:35:14]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 23:36:45]

Description was changed from

==========
Add settings screen that lets a user setup a PIN to unlock their device with.

When a PIN is enabled, when the user locks their device (but *only* when they
are signed in), the lockscreen will show a PIN keyboard they can use to
authenticate against.

The setup-pin screen is accessed from the choose method screen (added in a
dependency CL). It is available from the /quickUnlock/setupPin endpoint.

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

to

==========
Add settings screen that lets a user setup a PIN to unlock their device with.

When a PIN is enabled, when the user locks their device (but *only* when they
are signed in), the lockscreen will show a PIN keyboard they can use to
authenticate against.

The setup-pin screen is accessed from the choose method screen (added in a
dependency CL). It is available from the /quickUnlock/setupPin endpoint.

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

Committed: https://crrev.com/67cc9c5013f62aeb24b87a3a513bd590cdfc186c
Cr-Commit-Position: refs/heads/master@{#405348}
==========

[commit-bot: I haz the power, 2016-07-13 23:36:46]

Patchset 7 (id:??) landed as
https://crrev.com/67cc9c5013f62aeb24b87a3a513bd590cdfc186c
Cr-Commit-Position: refs/heads/master@{#405348}