Add fingerprint UI in user pod

Add fingerprint UI in user pod

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

Review-Url: https://codereview.chromium.org/2733953008
Cr-Commit-Position: refs/heads/master@{#456236}
Committed: https://chromium.googlesource.com/chromium/src/+/52a6b029471456658415ca04e0b69dfce232bed0

[xiaoyinh(OOO Sep 11-29), 2017-03-08 22:13:36]

Description was changed from

==========
Add fingerprint UI in user pod

BUG=695952
==========

to

==========
Add fingerprint UI in user pod

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

[xiaoyinh(OOO Sep 11-29), 2017-03-08 22:14:07]

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

[commit-bot: I haz the power, 2017-03-08 22:14:34]

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

[xiaoyinh(OOO Sep 11-29), 2017-03-08 22:31:06]

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

[commit-bot: I haz the power, 2017-03-08 22:31:47]

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

[xiaoyinh(OOO Sep 11-29), 2017-03-08 22:50:22]

xiaoyinh@chromium.org changed reviewers:
+ jdufault@chromium.org, xiyuan@chromium.org

[xiaoyinh(OOO Sep 11-29), 2017-03-08 22:50:23]

Please take a look. Thanks!

[jdufault, 2017-03-08 23:21:28]

Generally lg, some minor comments.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.css (right):

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.css:294: .fingerprint-icon-image,
What about applying inside of the div, you also apply the custom-icon class?
That would eliminate the changes on line 294, 306, and 420. Feel free to ignore
if there is styling that is applied to .custom-icon that does not also apply to
fingerprint-icon-image.

So class="custom-icon fingerprint-icon-image"

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.css:577: color:grey;
nit: space after :

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.css:581: color:#4285F4;
nit: space after :

Is this one of the material design colors such that we can reuse a constant?

ie,

color: var(--google-blue-500);

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.css:585: color:#DB4437;
ditto

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:147: {state: 0, class: 'default'},
fingerprintDefault? Or remove fingerprint prefix from other class names?

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:739: * Whether the user has recently
authencated with fingerprint.
nit: authencated -> authenticated

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:791:
this.fingerprintIconElement.tabIndex = UserPodTabOrder.POD_CUSTOM_ICON;
Does tabbing work properly when easy unlock and PIN are both shown?

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:1999: this.submitButton.disabled =
this.passwordElement.value.length <= 0;
Since you're here, can you change these <= to just ==?

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3108:
this.setUserPodFingerprintIcon(false, username);
Looks like you're missing the third 'state' parameter here.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3108:
this.setUserPodFingerprintIcon(false, username);
Please annotate the false parameter, ie, false /*visible*/

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3148: true, username,
FINGERPRINT_STATES.DEFAULT);
Please annotate true parameter, ie, true /*visible*/

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3157: setUserPodFingerprintIcon:
function(visible, username, state) {
Instead of having |visible| and |state| be separate, there could be a HIDDEN
state. Though that might mess up some of the state mapping logic you have. Up to
you.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3219: if (pod.fingerprintIconElement)
nit: {}

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3237: if (state ==
FINGERPRINT_STATES.FINGERPRINT_SIGNIN) {
There are only two ifs here so it is probably not worth doing, but you could
create a mapping from state -> string id, then do

  mapping = {
      FINGERPRINT_STATES.FINGERPRINT_FAILED: 'fingerprintSigninFailed',
      ...
    };
  placeholderStr = loadTimeData.getString(mapping[state]);

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3514:
this.setUserPodFingerprintIcon(false, pod.user.username);
Annotate false parameter

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3555: true, podToFocus.user.username,
FINGERPRINT_STATES.DEFAULT);
Annotate true parameter

[commit-bot: I haz the power, 2017-03-08 23:35:02]

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

[commit-bot: I haz the power, 2017-03-08 23:35:03]

Dry run: This issue passed the CQ dry run.

[xiaoyinh(OOO Sep 11-29), 2017-03-09 20:52:35]

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

[commit-bot: I haz the power, 2017-03-09 20:52:52]

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

[xiaoyinh(OOO Sep 11-29), 2017-03-09 20:59:12]

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.css (right):

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.css:294: .fingerprint-icon-image,
On 2017/03/08 23:21:26, jdufault wrote:
> What about applying inside of the div, you also apply the custom-icon class?
> That would eliminate the changes on line 294, 306, and 420. Feel free to
ignore
> if there is styling that is applied to .custom-icon that does not also apply
to
> fingerprint-icon-image.
> 
> So class="custom-icon fingerprint-icon-image"

Thanks for the suggestion. I've applied the custom-icon class along with
fingerprint-icon-image.
I didn't apply "custom-icon-container" with "fingerprint-icon-container" though,
because get customIconElement() are querying 'custom-icon-container', and just
want to avoid confusion with customIconElement vs 
fingerprintIconElement.
Let me know if you think otherwise.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.css:577: color:grey;
On 2017/03/08 23:21:27, jdufault wrote:
> nit: space after :

Done.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.css:581: color:#4285F4;
On 2017/03/08 23:21:26, jdufault wrote:
> nit: space after :
> 
> Is this one of the material design colors such that we can reuse a constant?
> 
> ie,
> 
> color: var(--google-blue-500);

Done.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.css:585: color:#DB4437;
On 2017/03/08 23:21:27, jdufault wrote:
> ditto

Done.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:147: {state: 0, class: 'default'},
On 2017/03/08 23:21:28, jdufault wrote:
> fingerprintDefault? Or remove fingerprint prefix from other class names?

Done.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:739: * Whether the user has recently
authencated with fingerprint.
On 2017/03/08 23:21:27, jdufault wrote:
> nit: authencated -> authenticated

Done.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:791:
this.fingerprintIconElement.tabIndex = UserPodTabOrder.POD_CUSTOM_ICON;
On 2017/03/08 23:21:27, jdufault wrote:
> Does tabbing work properly when easy unlock and PIN are both shown?

easy unlock seems to set the tabIndex only when the icon is interactive.
e.g. Click on the icon will hard lock the user pod.
Removing this line for fingerprint icon since we don't perform actions when
click on the icon.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:1999: this.submitButton.disabled =
this.passwordElement.value.length <= 0;
On 2017/03/08 23:21:27, jdufault wrote:
> Since you're here, can you change these <= to just ==?

Done.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3108:
this.setUserPodFingerprintIcon(false, username);
On 2017/03/08 23:21:28, jdufault wrote:
> Please annotate the false parameter, ie, false /*visible*/

Done.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3108:
this.setUserPodFingerprintIcon(false, username);
On 2017/03/08 23:21:27, jdufault wrote:
> Looks like you're missing the third 'state' parameter here.

I've consolidate 'visible' with 'state'.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3148: true, username,
FINGERPRINT_STATES.DEFAULT);
On 2017/03/08 23:21:27, jdufault wrote:
> Please annotate true parameter, ie, true /*visible*/

Done.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3157: setUserPodFingerprintIcon:
function(visible, username, state) {
On 2017/03/08 23:21:27, jdufault wrote:
> Instead of having |visible| and |state| be separate, there could be a HIDDEN
> state. Though that might mess up some of the state mapping logic you have. Up
to
> you.

Thanks. I added a hidden class to map with hidden state, let me know if this
makes sense.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3219: if (pod.fingerprintIconElement)
On 2017/03/08 23:21:27, jdufault wrote:
> nit: {}

Done.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3237: if (state ==
FINGERPRINT_STATES.FINGERPRINT_SIGNIN) {
On 2017/03/08 23:21:27, jdufault wrote:
> There are only two ifs here so it is probably not worth doing, but you could
> create a mapping from state -> string id, then do
> 
>   mapping = {
>       FINGERPRINT_STATES.FINGERPRINT_FAILED: 'fingerprintSigninFailed',
>       ...
>     };
>   placeholderStr = loadTimeData.getString(mapping[state]);

Thanks for the suggestion. I will create the mapping when there's more states.
Mind if I leave it like this for now?

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3514:
this.setUserPodFingerprintIcon(false, pod.user.username);
On 2017/03/08 23:21:27, jdufault wrote:
> Annotate false parameter

Done.

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3555: true, podToFocus.user.username,
FINGERPRINT_STATES.DEFAULT);
On 2017/03/08 23:21:27, jdufault wrote:
> Annotate true parameter

Done.

[jdufault, 2017-03-09 21:11:26]

lgtm

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/2733953008/diff/20001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3237: if (state ==
FINGERPRINT_STATES.FINGERPRINT_SIGNIN) {
On 2017/03/09 20:59:12, xiaoyinh wrote:
> On 2017/03/08 23:21:27, jdufault wrote:
> > There are only two ifs here so it is probably not worth doing, but you could
> > create a mapping from state -> string id, then do
> > 
> >   mapping = {
> >       FINGERPRINT_STATES.FINGERPRINT_FAILED: 'fingerprintSigninFailed',
> >       ...
> >     };
> >   placeholderStr = loadTimeData.getString(mapping[state]);
> 
> Thanks for the suggestion. I will create the mapping when there's more states.
> Mind if I leave it like this for now?

Yep, as-is is good - with only two elements it's not worth the overhead.

https://codereview.chromium.org/2733953008/diff/40001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/2733953008/diff/40001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3199:
resetIconAndPasswordField_:function(pod) {
nit: space after ':'

https://codereview.chromium.org/2733953008/diff/40001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3214: 'Unable to remove user pod
fingerprint icon: user pod not found.');
You might be able to get the console.error on one line with a string like 

  'No user pod found (when removing fingerprint icon)'

Though the current string is also good.

[commit-bot: I haz the power, 2017-03-09 21:38:08]

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

[commit-bot: I haz the power, 2017-03-09 21:38:10]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-03-10 17:51:10]

lgtm

https://codereview.chromium.org/2733953008/diff/40001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/2733953008/diff/40001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:151: {state: 3, class: 'failed'}
nit: Can replace the numbers with the state const defined above?
i.e.
var FINGERPRINT_STATES_MAPPING = [
  {state: FINGERPRINT_STATES.HIDDEN, class: 'hidden'},
  ...
];

https://codereview.chromium.org/2733953008/diff/40001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:1912:
(this.getElementsByClassName('auth-container'))[0]);
nit: wrap the bubbleAnchor logic into a helper and use it here and in
handleFingerprintIconMouseOver_.

https://codereview.chromium.org/2733953008/diff/40001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3178: 'aria-label',
loadTimeData.getString('fingerprintIconMessage'));
The aria-label seems to be a const. Why not put it in html ?

  <div class="fingerprint-icon-container" hidden
      i18n-values="aria-label:fingerprintIconMessage">

[xiaoyinh(OOO Sep 11-29), 2017-03-10 21:41:16]

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

[commit-bot: I haz the power, 2017-03-10 21:41:32]

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

[xiaoyinh(OOO Sep 11-29), 2017-03-10 21:55:58]

https://codereview.chromium.org/2733953008/diff/40001/ui/login/account_picker...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/2733953008/diff/40001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:151: {state: 3, class: 'failed'}
On 2017/03/10 17:51:10, xiyuan wrote:
> nit: Can replace the numbers with the state const defined above?
> i.e.
> var FINGERPRINT_STATES_MAPPING = [
>   {state: FINGERPRINT_STATES.HIDDEN, class: 'hidden'},
>   ...
> ];

Done.

https://codereview.chromium.org/2733953008/diff/40001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:1912:
(this.getElementsByClassName('auth-container'))[0]);
On 2017/03/10 17:51:10, xiyuan wrote:
> nit: wrap the bubbleAnchor logic into a helper and use it here and in
> handleFingerprintIconMouseOver_.

Done.

https://codereview.chromium.org/2733953008/diff/40001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3178: 'aria-label',
loadTimeData.getString('fingerprintIconMessage'));
On 2017/03/10 17:51:10, xiyuan wrote:
> The aria-label seems to be a const. Why not put it in html ?
> 
>   <div class="fingerprint-icon-container" hidden
>       i18n-values="aria-label:fingerprintIconMessage">

Done.

https://codereview.chromium.org/2733953008/diff/40001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3199:
resetIconAndPasswordField_:function(pod) {
On 2017/03/09 21:11:25, jdufault wrote:
> nit: space after ':'

Done.

https://codereview.chromium.org/2733953008/diff/40001/ui/login/account_picker...
ui/login/account_picker/user_pod_row.js:3214: 'Unable to remove user pod
fingerprint icon: user pod not found.');
On 2017/03/09 21:11:25, jdufault wrote:
> You might be able to get the console.error on one line with a string like 
> 
>   'No user pod found (when removing fingerprint icon)'
> 
> Though the current string is also good.

Done.

[commit-bot: I haz the power, 2017-03-10 22:28:47]

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

[commit-bot: I haz the power, 2017-03-10 22:28:47]

Dry run: This issue passed the CQ dry run.

[xiaoyinh(OOO Sep 11-29), 2017-03-11 00:35:51]

The CQ bit was checked by xiaoyinh@chromium.org

[xiaoyinh(OOO Sep 11-29), 2017-03-11 00:35:52]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
jdufault@chromium.org
Link to the patchset: https://codereview.chromium.org/2733953008/#ps60001
(title: "Incorporate comments")

[commit-bot: I haz the power, 2017-03-11 00:36:20]

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-03-11 00:42:20]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489192551619710,
"parent_rev": "dccc4a73c2b74721e6b7f5ef32859ea84e73603c", "commit_rev":
"52a6b029471456658415ca04e0b69dfce232bed0"}

[commit-bot: I haz the power, 2017-03-11 00:43:04]

Description was changed from

==========
Add fingerprint UI in user pod

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

to

==========
Add fingerprint UI in user pod

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

Review-Url: https://codereview.chromium.org/2733953008
Cr-Commit-Position: refs/heads/master@{#456236}
Committed:
https://chromium.googlesource.com/chromium/src/+/52a6b029471456658415ca04e0b6...
==========

[commit-bot: I haz the power, 2017-03-11 00:43:05]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/52a6b029471456658415ca04e0b6...