Add a settings screen that lets the user choose how they unlock their device.

Add a settings screen that lets the user choose how they unlock their device.

Currently, the user can unlock their device using one of three methods:
  - their password,
  - a PIN or password,
  - no authentication.

The setup pin screen is in a dependent CL.

The choose-method screen is available from the /quickUnlock/chooseMethod
endpoint.

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

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

[jdufault, 2016-06-23 22:13:43]

Description was changed from

==========
Add a settings screen that lets the user choose how they unlock their device.

Currently, the user can unlock their device using one of three methods:
  - their password,
  - a PIN or password,
  - no authentication.

The setup pin screen is in a dependent CL.

The choose-method screen is available from the /quickUnlock/chooseMethod
endpoint.

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

to

==========
Add a settings screen that lets the user choose how they unlock their device.

Currently, the user can unlock their device using one of three methods:
  - their password,
  - a PIN or password,
  - no authentication.

The setup pin screen is in a dependent CL.

The choose-method screen is available from the /quickUnlock/chooseMethod
endpoint.

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

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

Description was changed from

==========
Add a settings screen that lets the user choose how they unlock their device.

Currently, the user can unlock their device using one of three methods:
  - their password,
  - a PIN or password,
  - no authentication.

The setup pin screen is in a dependent CL.

The choose-method screen is available from the /quickUnlock/chooseMethod
endpoint.

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

to

==========
Add a settings screen that lets the user choose how they unlock their device.

Currently, the user can unlock their device using one of three methods:
  - their password,
  - a PIN or password,
  - no authentication.

The setup pin screen is in a dependent CL.

The choose-method screen is available from the /quickUnlock/chooseMethod
endpoint.

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

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

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

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

Tommy, PTAL. Thanks!

[tommycli, 2016-06-29 22:56:10]

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/compiled_resources2.gyp
(right):

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/compiled_resources2.gyp:134:
'target_name': 'quick_unlock_routing_behavior',
These quick unlock entries should be re-ordered alphabetically with the rest of
the file.

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

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.html:20:
#configure-container *> span {
This is just that one error message span right? Just give it an explicit id.

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.html:25:
.align-end {
Does .button-strip within settings-shared work instead of making a new class?

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.html:57:
<paper-button class="action-button" on-tap="onConfigurePin_">
Can we make this onSubmitPin_? It would make clear to the user that it's
expected to go to the next part of the wizard.

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

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:69:
chrome.settingsPrivate.onPrefsChanged.addListener(function(prefs) {
This needs a corresponding removal in detached right?

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:76:
chrome.quickUnlockPrivate.onActiveModesChanged.addListener(
Same here right?

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:108:
var hasScreenLock = this.getPref(ENABLE_LOCK_SCREEN_PREF).value;
Just move that into the if predicate

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:145:
var showPinConfiguration = selected == QuickUnlockUnlockType.PIN_PASSWORD;
Since these vars are only used once each and are pretty self explanatory,
probably just inline them.

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:149:
this.setModes.call(null, [], [], function(didSet) {
I think setModes already has the password partially applied right?

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:150:
if (!didSet) {
Is it standard to print a console error on an unexpected result? What's the
expected UI?

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:163:
computeConfigureContainerStyle_: function(selectedUnlockType) {
Is there a hard UI requirement spelled out to make this fade in and out? If not,
I would say other section of settings don't do this, and I would suggest just
hiding and unhiding with hidden=

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

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_validating_behavior.js:20:
checkForPassword: function() {
Maybe rename this to: askForPasswordIfUnset, or something similar, to make clear
that this will trigger an action rather than just verifying the password.

Likewise, perhaps a better name for the behavior is:
PasswordTimeoutRedirectBehavior -- or something similar.

[jdufault, 2016-06-30 18:10:18]

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/people_page/compiled_resources2.gyp
(right):

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/compiled_resources2.gyp:134:
'target_name': 'quick_unlock_routing_behavior',
On 2016/06/29 22:56:09, tommycli wrote:
> These quick unlock entries should be re-ordered alphabetically with the rest
of
> the file.

Done.

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

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.html:20:
#configure-container *> span {
On 2016/06/29 22:56:09, tommycli wrote:
> This is just that one error message span right? Just give it an explicit id.

Done.

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.html:25:
.align-end {
On 2016/06/29 22:56:09, tommycli wrote:
> Does .button-strip within settings-shared work instead of making a new class?

Nope; I tried putting it on both the div and the button (fwiw, button-strip adds
a text-align: end).

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.html:57:
<paper-button class="action-button" on-tap="onConfigurePin_">
On 2016/06/29 22:56:09, tommycli wrote:
> Can we make this onSubmitPin_? It would make clear to the user that it's
> expected to go to the next part of the wizard.

onSubmitPin_ implies that we are submitting a PIN, but we are actually setting
up the PIN. I'd prefer to use the term 'setup' instead of 'configure', but I
went with 'configure' because that's what the UI string uses.

I can change this if you'd still like, but I'd prefer onSetupPin_ instead of
onSubmitPin_.

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

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:69:
chrome.settingsPrivate.onPrefsChanged.addListener(function(prefs) {
On 2016/06/29 22:56:09, tommycli wrote:
> This needs a corresponding removal in detached right?

Done.

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:76:
chrome.quickUnlockPrivate.onActiveModesChanged.addListener(
On 2016/06/29 22:56:09, tommycli wrote:
> Same here right?

Done.

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:108:
var hasScreenLock = this.getPref(ENABLE_LOCK_SCREEN_PREF).value;
On 2016/06/29 22:56:09, tommycli wrote:
> Just move that into the if predicate

Done.

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:145:
var showPinConfiguration = selected == QuickUnlockUnlockType.PIN_PASSWORD;
On 2016/06/29 22:56:09, tommycli wrote:
> Since these vars are only used once each and are pretty self explanatory,
> probably just inline them.

Done.

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:149:
this.setModes.call(null, [], [], function(didSet) {
On 2016/06/29 22:56:09, tommycli wrote:
> I think setModes already has the password partially applied right?

null is for the `this` object. I'd prefer to use the `this.setModes([], [],
...)` syntax, but closure complains that it's not a callable type.

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:150:
if (!didSet) {
On 2016/06/29 22:56:09, tommycli wrote:
> Is it standard to print a console error on an unexpected result? What's the
> expected UI?

No UI right now, unfortunately. Right now, this function call will only ever
fail if the API is used incorrectly.

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:163:
computeConfigureContainerStyle_: function(selectedUnlockType) {
On 2016/06/29 22:56:09, tommycli wrote:
> Is there a hard UI requirement spelled out to make this fade in and out? If
not,
> I would say other section of settings don't do this, and I would suggest just
> hiding and unhiding with hidden=

No, there's no hard UI requirement, but using hidden looks bad, because the
radio group is also animating in/out. Without the opacity animation, the button
and warning will appear before the radio button finishes animating.

The amount of code is almost the same, and approaches are also conceptually
identical.

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

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_validating_behavior.js:20:
checkForPassword: function() {
On 2016/06/29 22:56:09, tommycli wrote:
> Maybe rename this to: askForPasswordIfUnset, or something similar, to make
clear
> that this will trigger an action rather than just verifying the password.

Done, this is a much better name.

> Likewise, perhaps a better name for the behavior is:
> PasswordTimeoutRedirectBehavior -- or something similar.

Renamed to QuickUnlockPasswordDetectBehavior

[tommycli, 2016-07-01 19:02:42]

cool thx

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

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:150:
if (!didSet) {
On 2016/06/30 18:10:18, jdufault wrote:
> On 2016/06/29 22:56:09, tommycli wrote:
> > Is it standard to print a console error on an unexpected result? What's the
> > expected UI?
> 
> No UI right now, unfortunately. Right now, this function call will only ever
> fail if the API is used incorrectly.

Ah, if it can only fail due to programmer error, I would use:

assert(didSet, 'Failed to clear quick unlock modes.');

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:163:
computeConfigureContainerStyle_: function(selectedUnlockType) {
On 2016/06/30 18:10:18, jdufault wrote:
> On 2016/06/29 22:56:09, tommycli wrote:
> > Is there a hard UI requirement spelled out to make this fade in and out? If
> not,
> > I would say other section of settings don't do this, and I would suggest
just
> > hiding and unhiding with hidden=
> 
> No, there's no hard UI requirement, but using hidden looks bad, because the
> radio group is also animating in/out. Without the opacity animation, the
button
> and warning will appear before the radio button finishes animating.
> 
> The amount of code is almost the same, and approaches are also conceptually
> identical.

Where's the code to the radio group animating in and out?

If UI team didn't specify an animation, I'd recommend against it, since all the
other controls in Settings don't do the opacity animation.

[jdufault, 2016-07-13 00:17:07]

Patchset #3 (id:40001) has been deleted

[jdufault, 2016-07-13 00:19:22]

Sorry about the delay. Also, I think I deleted PS3 when I shouldn't have. I've
uploaded the new changes as PS3 :/

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

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:150:
if (!didSet) {
On 2016/07/01 19:02:42, tommycli wrote:
> On 2016/06/30 18:10:18, jdufault wrote:
> > On 2016/06/29 22:56:09, tommycli wrote:
> > > Is it standard to print a console error on an unexpected result? What's
the
> > > expected UI?
> > 
> > No UI right now, unfortunately. Right now, this function call will only ever
> > fail if the API is used incorrectly.
> 
> Ah, if it can only fail due to programmer error, I would use:
> 
> assert(didSet, 'Failed to clear quick unlock modes.');
> 

Done.

https://codereview.chromium.org/2097673002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_choose_method.js:163:
computeConfigureContainerStyle_: function(selectedUnlockType) {
On 2016/07/01 19:02:42, tommycli wrote:
> On 2016/06/30 18:10:18, jdufault wrote:
> > On 2016/06/29 22:56:09, tommycli wrote:
> > > Is there a hard UI requirement spelled out to make this fade in and out?
If
> > not,
> > > I would say other section of settings don't do this, and I would suggest
> just
> > > hiding and unhiding with hidden=
> > 
> > No, there's no hard UI requirement, but using hidden looks bad, because the
> > radio group is also animating in/out. Without the opacity animation, the
> button
> > and warning will appear before the radio button finishes animating.
> > 
> > The amount of code is almost the same, and approaches are also conceptually
> > identical.
> 
> Where's the code to the radio group animating in and out?
> 
> If UI team didn't specify an animation, I'd recommend against it, since all
the
> other controls in Settings don't do the opacity animation.

The radio group animation is part of paper-radio-group.

I've removed the opacity animation. If there are complaints I can always add it
back.

[tommycli, 2016-07-13 18:15:02]

cool lgtm

[jdufault, 2016-07-13 18:29:55]

The CQ bit was checked by jdufault@chromium.org

[commit-bot: I haz the power, 2016-07-13 18:31:14]

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 19:33:09]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names without "master." prefix is deprecated:
tryserver.chromium.linux. For more details, see http://crbug.com/617627.

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

Description was changed from

==========
Add a settings screen that lets the user choose how they unlock their device.

Currently, the user can unlock their device using one of three methods:
  - their password,
  - a PIN or password,
  - no authentication.

The setup pin screen is in a dependent CL.

The choose-method screen is available from the /quickUnlock/chooseMethod
endpoint.

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

to

==========
Add a settings screen that lets the user choose how they unlock their device.

Currently, the user can unlock their device using one of three methods:
  - their password,
  - a PIN or password,
  - no authentication.

The setup pin screen is in a dependent CL.

The choose-method screen is available from the /quickUnlock/chooseMethod
endpoint.

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

[commit-bot: I haz the power, 2016-07-13 20:01:21]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-07-13 20:01:23]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 20:04:27]

Description was changed from

==========
Add a settings screen that lets the user choose how they unlock their device.

Currently, the user can unlock their device using one of three methods:
  - their password,
  - a PIN or password,
  - no authentication.

The setup pin screen is in a dependent CL.

The choose-method screen is available from the /quickUnlock/chooseMethod
endpoint.

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

to

==========
Add a settings screen that lets the user choose how they unlock their device.

Currently, the user can unlock their device using one of three methods:
  - their password,
  - a PIN or password,
  - no authentication.

The setup pin screen is in a dependent CL.

The choose-method screen is available from the /quickUnlock/chooseMethod
endpoint.

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

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

[commit-bot: I haz the power, 2016-07-13 20:04:29]

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