Add the authenticate screen for the quick unlock settings.

Add the authenticate screen for the quick unlock settings.

The authenticate screen verifies the user's account password, since the quick
unlock settings control how the device is unlocked from the lock screen after
the user has signed in.

This also adds the entry-point for quick unlock settings to the people page.

The authenticate screen is available from the /quickUnlock/authenticate
endpoint.

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

Committed: https://crrev.com/7982a71bf7df88606e941c00cbedf62a10077a38
Cr-Commit-Position: refs/heads/master@{#402984}

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

Description was changed from

==========
Add the authenticate screen for the quick unlock settings.

The authenticate screen verifies the user's account password, since the quick
unlock settings control how the device is unlocked from the lock screen after
the user has signed in.

This also adds the entry-point for quick unlock settings to the people page.

The authenticate screen is available from the /quickUnlock/authenticate
endpoint.

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

to

==========
Add the authenticate screen for the quick unlock settings.

The authenticate screen verifies the user's account password, since the quick
unlock settings control how the device is unlocked from the lock screen after
the user has signed in.

This also adds the entry-point for quick unlock settings to the people page.

The authenticate screen is available from the /quickUnlock/authenticate
endpoint.

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

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

Description was changed from

==========
Add the authenticate screen for the quick unlock settings.

The authenticate screen verifies the user's account password, since the quick
unlock settings control how the device is unlocked from the lock screen after
the user has signed in.

This also adds the entry-point for quick unlock settings to the people page.

The authenticate screen is available from the /quickUnlock/authenticate
endpoint.

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

to

==========
Add the authenticate screen for the quick unlock settings.

The authenticate screen verifies the user's account password, since the quick
unlock settings control how the device is unlocked from the lock screen after
the user has signed in.

This also adds the entry-point for quick unlock settings to the people page.

The authenticate screen is available from the /quickUnlock/authenticate
endpoint.

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

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

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

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

Tommy, PTAL. Thanks!

[tommycli, 2016-06-24 00:56:39]

jdufault: Here you are, thanks!

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

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.html:12:
flex-grow: 0 !important;
!important is really discouraged.

If you don't need flexbox (and i think you may not), try class="settings-box
block".

https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.html:17:
flex-grow: 1;
Does this make the password line span the whole line? Try:
      paper-input {
        width: var(--paper-input-max-width);
      }

That's used in many other parts of settings.

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.html:21:
margin-bottom: 22px;
Is this to make the settings box a bit taller? Maybe try "settings-box two-line"
class? Or if there's a good reason, add a comment to explain the number.

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.html:29:
<div class="middle">
Is the middle class to produce the horizontal line displayed in the mocks?

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

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.js:84:
startDelayedPasswordCheck_: function() {
There's no Submit button? Can the user still press Enter to submit the password?

Is there any precedent for auto-checking the entered password after 500ms? This
seems highly unusual.

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.js:123:
setTimeout(this.clearAccountPassword_.bind(this),
We would want to cancel this timeout if we re-entered the password right?

But more importantly, can we not store the password in a JS variable? Someone
could just grab it with inspector.

Can the C++ side just 'remember' that the user is authenticated for 10 minutes?

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

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_routing_behavior.js:40:
navigateToScreen: function(screen) {
With only 3 callsites, I'm not sure it's worthwhile to abstract this method into
a behavior.

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_routing_behavior.js:54:
getSubpagesForScreen_: function(screen) {
I think we can eliminate this method, since each of these switch cases has only
one callsite.

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_routing_behavior.js:64:
};
I don't think this behavior is "worth it". I don't think it decreases the SLOC
by much, if at all.

[jdufault, 2016-06-29 19:06:39]

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

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.html:12:
flex-grow: 0 !important;
On 2016/06/24 00:56:38, tommycli wrote:
> !important is really discouraged.
> 
> If you don't need flexbox (and i think you may not), try class="settings-box
> block".
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin...

Done.

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.html:17:
flex-grow: 1;
On 2016/06/24 00:56:38, tommycli wrote:
> Does this make the password line span the whole line? Try:
>       paper-input {
>         width: var(--paper-input-max-width);
>       }
> 
> That's used in many other parts of settings.

I've realigned with mocks and the password line no longer takes the full line
width.

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.html:21:
margin-bottom: 22px;
On 2016/06/24 00:56:38, tommycli wrote:
> Is this to make the settings box a bit taller? Maybe try "settings-box
two-line"
> class? Or if there's a good reason, add a comment to explain the number.

I've added some comments and solved the problem differently, but essentially the
paper-input element has a hidden floating element which makes makes the
secondary-action vertical splitter be vertically unbalanced (top-heavy).

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.html:29:
<div class="middle">
On 2016/06/24 00:56:38, tommycli wrote:
> Is the middle class to produce the horizontal line displayed in the mocks? 

Removed

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

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.js:84:
startDelayedPasswordCheck_: function() {
On 2016/06/24 00:56:38, tommycli wrote:
> There's no Submit button? Can the user still press Enter to submit the
password?
> 
> Is there any precedent for auto-checking the entered password after 500ms?
This
> seems highly unusual.

Right, there is no submit button. Pressing enter will call checkPasswordNow_.

I'm not sure if there is precedent for auto-submit, but since there is no submit
button in the mocks I think it makes a lot of sense.

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.js:123:
setTimeout(this.clearAccountPassword_.bind(this),
On 2016/06/24 00:56:39, tommycli wrote:
> We would want to cancel this timeout if we re-entered the password right?

Done

> But more importantly, can we not store the password in a JS variable? Someone
> could just grab it with inspector.
> 
> Can the C++ side just 'remember' that the user is authenticated for 10
minutes?

It's non-trivial at this point to make the C++ side remember; based on offline
discussion, going with a closure.

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

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_routing_behavior.js:40:
navigateToScreen: function(screen) {
On 2016/06/24 00:56:39, tommycli wrote:
> With only 3 callsites, I'm not sure it's worthwhile to abstract this method
into
> a behavior.

Done.

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_routing_behavior.js:54:
getSubpagesForScreen_: function(screen) {
On 2016/06/24 00:56:39, tommycli wrote:
> I think we can eliminate this method, since each of these switch cases has
only
> one callsite.

Done.

https://codereview.chromium.org/2091143002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/people_page/quick_unlock_routing_behavior.js:64:
};
On 2016/06/24 00:56:39, tommycli wrote:
> I don't think this behavior is "worth it". I don't think it decreases the SLOC
> by much, if at all.

I've removed everything except isScreenActive.

[tommycli, 2016-06-29 21:04:23]

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

https://codereview.chromium.org/2091143002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.js:14: *
password by itself is not avaialble for other elements to access.
typo on "available"

https://codereview.chromium.org/2091143002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.js:42: *
authentication screen needs to be redispalyed to recapture the user's
"redisplayed" typo

https://codereview.chromium.org/2091143002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.js:45:
*/
How about revise the comment to similar to:

If the user has entered a password, this method will be a partially-applied
chrome.quickUnlockPrivate.setModes method call (with the password parameter
applied). Otherwise, this is null, and the authentication screen needs to be
redisplayed. The value stored here will be reset after
PASSWORD_ACTIVE_DURATION_MS milliseconds.

It doesn't have the be the exact above text, just reword a bit.

https://codereview.chromium.org/2091143002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.js:120:
}.bind(this));
Here i'd recommend something instead like this:

var onPasswordChecked = function(valid) { ... }.bind(this);
chrome.quickUnlockPrivate.getActiveModes ...

That way you can eliminate one of the bind(this) calls.

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

https://codereview.chromium.org/2091143002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_routing_behavior.js:13:
var QuickUnlockRoutingBehavior = {
Okay this is fine for now. We might be able to eliminate this after I look at
refactoring routing, but I'm fine with it for now, esp. since it's pretty small
and just one method.

[jdufault, 2016-06-29 22:02:04]

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

https://codereview.chromium.org/2091143002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.js:14: *
password by itself is not avaialble for other elements to access.
On 2016/06/29 21:04:23, tommycli wrote:
> typo on "available"

Done.

https://codereview.chromium.org/2091143002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.js:42: *
authentication screen needs to be redispalyed to recapture the user's
On 2016/06/29 21:04:23, tommycli wrote:
> "redisplayed" typo

Done.

https://codereview.chromium.org/2091143002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.js:45:
*/
On 2016/06/29 21:04:23, tommycli wrote:
> How about revise the comment to similar to:
> 
> If the user has entered a password, this method will be a partially-applied
> chrome.quickUnlockPrivate.setModes method call (with the password parameter
> applied). Otherwise, this is null, and the authentication screen needs to be
> redisplayed. The value stored here will be reset after
> PASSWORD_ACTIVE_DURATION_MS milliseconds.
> 
> It doesn't have the be the exact above text, just reword a bit.

Done.

https://codereview.chromium.org/2091143002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_authenticate.js:120:
}.bind(this));
On 2016/06/29 21:04:23, tommycli wrote:
> Here i'd recommend something instead like this:
> 
> var onPasswordChecked = function(valid) { ... }.bind(this);
> chrome.quickUnlockPrivate.getActiveModes ...
> 
> That way you can eliminate one of the bind(this) calls.

I've pulled the chrome.quickUnlockPrivate calls out into a helper function;
hopefully that is a bit better.

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

https://codereview.chromium.org/2091143002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/people_page/quick_unlock_routing_behavior.js:13:
var QuickUnlockRoutingBehavior = {
On 2016/06/29 21:04:23, tommycli wrote:
> Okay this is fine for now. We might be able to eliminate this after I look at
> refactoring routing, but I'm fine with it for now, esp. since it's pretty
small
> and just one method.

sg, it'd be nice to move this method (or something similar) to a more general
lib.

[tommycli, 2016-06-29 22:03:49]

lgtm

[jdufault, 2016-06-29 22:06:10]

The CQ bit was checked by jdufault@chromium.org

[commit-bot: I haz the power, 2016-06-29 22:11:25]

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-06-29 23:12:50]

Description was changed from

==========
Add the authenticate screen for the quick unlock settings.

The authenticate screen verifies the user's account password, since the quick
unlock settings control how the device is unlocked from the lock screen after
the user has signed in.

This also adds the entry-point for quick unlock settings to the people page.

The authenticate screen is available from the /quickUnlock/authenticate
endpoint.

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

to

==========
Add the authenticate screen for the quick unlock settings.

The authenticate screen verifies the user's account password, since the quick
unlock settings control how the device is unlocked from the lock screen after
the user has signed in.

This also adds the entry-point for quick unlock settings to the people page.

The authenticate screen is available from the /quickUnlock/authenticate
endpoint.

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

[commit-bot: I haz the power, 2016-06-29 23:12:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-29 23:13:06]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 23:16:53]

Description was changed from

==========
Add the authenticate screen for the quick unlock settings.

The authenticate screen verifies the user's account password, since the quick
unlock settings control how the device is unlocked from the lock screen after
the user has signed in.

This also adds the entry-point for quick unlock settings to the people page.

The authenticate screen is available from the /quickUnlock/authenticate
endpoint.

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

to

==========
Add the authenticate screen for the quick unlock settings.

The authenticate screen verifies the user's account password, since the quick
unlock settings control how the device is unlocked from the lock screen after
the user has signed in.

This also adds the entry-point for quick unlock settings to the people page.

The authenticate screen is available from the /quickUnlock/authenticate
endpoint.

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

Committed: https://crrev.com/7982a71bf7df88606e941c00cbedf62a10077a38
Cr-Commit-Position: refs/heads/master@{#402984}
==========

[commit-bot: I haz the power, 2016-06-29 23:16:54]

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