Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2427073002:
cros: Fixed 3 bugs for pin unlock. (Closed)
|
Created:
4 years, 2 months ago by sammiequon Modified:
4 years, 1 month ago CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Fixed 3 bugs for pin unlock.
User name would flash on startup before the rest of the user pod. Pin keyboard would show up for users with disabled keyboards and the easy unlock icon would clash with the re-authentication icon. I hide the easy unlock icon whenever the re-authentication icon is shown.
BUG=655221, 654300, 655308
TEST=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/a77a4ddde837ba57fca3195127cd4f1ad8f96eab
Cr-Commit-Position: refs/heads/master@{#427152}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Fixed patch set 1 errors. #
Total comments: 4
Patch Set 3 : Fixed patch set 2 errors. #
Total comments: 2
Patch Set 4 : Rebased. #Patch Set 5 : Fixed patch set 3 errors. #
Total comments: 2
Patch Set 6 : Fixed patch set 5 errors. #Patch Set 7 : Rebased. #
Dependent Patchsets: Messages
Total messages: 41 (25 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== cros: Fixed 3 bugs for pin unlock. User name would flash on startup before the rest of the user pod. Pin keyboard would show up for users with disabled keyboards and the easy unlock icon would clash with the re-authentication icon. I hide the easy unlock icon whenever the re-authentication icon is shown. BUG= 655221, 654300, 655308 TEST=none ========== to ========== cros: Fixed 3 bugs for pin unlock. User name would flash on startup before the rest of the user pod. Pin keyboard would show up for users with disabled keyboards and the easy unlock icon would clash with the re-authentication icon. I hide the easy unlock icon whenever the re-authentication icon is shown. BUG= 655221, 654300, 655308 TEST=none ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
On 2016/10/18 21:46:07, sammiequon wrote: > mailto:sammiequon@chromium.org changed reviewers: > + mailto:jdufault@chromium.org jdufault@ - Please take a look. Thanks!
https://codereview.chromium.org/2427073002/diff/20001/ui/login/account_picker... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2427073002/diff/20001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:255: for (var i in users) { Don't we usually do a for (var i = 0; i < users.length; ++i) {} loop? iirc, I think there are subtle gotchas with this form. https://codereview.chromium.org/2427073002/diff/20001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:257: this.disablePinKeyboardForUser(users[i].username); Why not put this inside of loadPinKeyboardIfNeeded_? Maybe rename that method to initializePinKeyboardStateForUsers_? https://codereview.chromium.org/2427073002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2427073002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:199: .pod.focused .name-container { What is the name-container used for? We want to always hide it whenever the pod is focused?
https://codereview.chromium.org/2427073002/diff/20001/ui/login/account_picker... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2427073002/diff/20001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:255: for (var i in users) { On 2016/10/19 18:12:29, jdufault wrote: > Don't we usually do a for (var i = 0; i < users.length; ++i) {} loop? iirc, I > think there are subtle gotchas with this form. Done. https://codereview.chromium.org/2427073002/diff/20001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:257: this.disablePinKeyboardForUser(users[i].username); On 2016/10/19 18:12:29, jdufault wrote: > Why not put this inside of loadPinKeyboardIfNeeded_? Maybe rename that method to > initializePinKeyboardStateForUsers_? Done. https://codereview.chromium.org/2427073002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2427073002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:199: .pod.focused .name-container { On 2016/10/19 18:12:29, jdufault wrote: > What is the name-container used for? We want to always hide it whenever the pod > is focused? The name container shows the name when there are multiple pods. It is hidden in place of the password container when the pod is focused.
https://codereview.chromium.org/2427073002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2427073002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:199: .pod.focused .name-container { On 2016/10/19 21:08:10, sammiequon wrote: > On 2016/10/19 18:12:29, jdufault wrote: > > What is the name-container used for? We want to always hide it whenever the > pod > > is focused? > > The name container shows the name when there are multiple pods. It is hidden in > place of the password container when the pod is focused. What used to hide this if the PIN keyboard was not enabled? https://codereview.chromium.org/2427073002/diff/40001/ui/login/account_picker... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2427073002/diff/40001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:228: * the PIN keyboard for users who are allowed to use PIN unlock. nit: who are not* https://codereview.chromium.org/2427073002/diff/40001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:235: showPinKeyboardAsync(); I wonder if calling this multiple times will cause a problem / performance slowdown. I know the comment on the function says it won't, but I think the implementation is slightly wrong. Can you change showPinKeyboardAsync so that it calls cr.ui.login.ResourceLoader.hasDeferredAssets instead of cr.ui.login.ResourceLoader.alreadyLoadedAssets
Description was changed from ========== cros: Fixed 3 bugs for pin unlock. User name would flash on startup before the rest of the user pod. Pin keyboard would show up for users with disabled keyboards and the easy unlock icon would clash with the re-authentication icon. I hide the easy unlock icon whenever the re-authentication icon is shown. BUG= 655221, 654300, 655308 TEST=none ========== to ========== cros: Fixed 3 bugs for pin unlock. User name would flash on startup before the rest of the user pod. Pin keyboard would show up for users with disabled keyboards and the easy unlock icon would clash with the re-authentication icon. I hide the easy unlock icon whenever the re-authentication icon is shown. BUG= 655221, 654300, 655308 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2427073002/diff/20001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/2427073002/diff/20001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:199: .pod.focused .name-container { On 2016/10/19 22:13:25, jdufault wrote: > On 2016/10/19 21:08:10, sammiequon wrote: > > On 2016/10/19 18:12:29, jdufault wrote: > > > What is the name-container used for? We want to always hide it whenever the > > pod > > > is focused? > > > > The name container shows the name when there are multiple pods. It is hidden > in > > place of the password container when the pod is focused. > > What used to hide this if the PIN keyboard was not enabled? This same section. I removed it earlier because the name was looking funny on the old mocks. https://codereview.chromium.org/2427073002/diff/40001/ui/login/account_picker... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2427073002/diff/40001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:228: * the PIN keyboard for users who are allowed to use PIN unlock. On 2016/10/19 22:13:25, jdufault wrote: > nit: who are not* Done. https://codereview.chromium.org/2427073002/diff/40001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:235: showPinKeyboardAsync(); On 2016/10/19 22:13:25, jdufault wrote: > I wonder if calling this multiple times will cause a problem / performance > slowdown. I know the comment on the function says it won't, but I think the > implementation is slightly wrong. Can you change showPinKeyboardAsync so that it > calls > > cr.ui.login.ResourceLoader.hasDeferredAssets > > instead of > > cr.ui.login.ResourceLoader.alreadyLoadedAssets Done.
https://codereview.chromium.org/2427073002/diff/60001/ui/login/account_picker... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2427073002/diff/60001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:238: // users include those who have not set up pin as those who have not I'm not sure we need to include a comment here. If you want to keep it, please cleanup it a little, ie: // Disable PIN for users which cannot authenticate with it. For // example, users who have not set up PIN or users who have not // entered their account password recently. This comment explains the user.showPin value - I'm not sure that variable is confusing, though. If anything, we should be explaining why we want to explicitly call disableKeyboardForUser, ie, if we do not, then the PIN keyboard can appear when there are >1 users.
https://codereview.chromium.org/2427073002/diff/60001/ui/login/account_picker... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2427073002/diff/60001/ui/login/account_picker... ui/login/account_picker/screen_account_picker.js:238: // users include those who have not set up pin as those who have not On 2016/10/21 19:09:56, jdufault wrote: > I'm not sure we need to include a comment here. If you want to keep it, please > cleanup it a little, ie: > > // Disable PIN for users which cannot authenticate with it. For > // example, users who have not set up PIN or users who have not > // entered their account password recently. > > This comment explains the user.showPin value - I'm not sure that variable is > confusing, though. If anything, we should be explaining why we want to > explicitly call disableKeyboardForUser, ie, if we do not, then the PIN keyboard > can appear when there are >1 users. Done.
lgtm https://codereview.chromium.org/2427073002/diff/100001/ui/login/account_picke... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2427073002/diff/100001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:240: // will appear for any user if their is at least one user who has PIN Disable pin => Disable PIN for users which => for users who their => there
https://codereview.chromium.org/2427073002/diff/100001/ui/login/account_picke... File ui/login/account_picker/screen_account_picker.js (right): https://codereview.chromium.org/2427073002/diff/100001/ui/login/account_picke... ui/login/account_picker/screen_account_picker.js:240: // will appear for any user if their is at least one user who has PIN On 2016/10/21 22:27:41, jdufault wrote: > Disable pin => Disable PIN > for users which => for users who > their => there Done.
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== cros: Fixed 3 bugs for pin unlock. User name would flash on startup before the rest of the user pod. Pin keyboard would show up for users with disabled keyboards and the easy unlock icon would clash with the re-authentication icon. I hide the easy unlock icon whenever the re-authentication icon is shown. BUG= 655221, 654300, 655308 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Fixed 3 bugs for pin unlock. User name would flash on startup before the rest of the user pod. Pin keyboard would show up for users with disabled keyboards and the easy unlock icon would clash with the re-authentication icon. I hide the easy unlock icon whenever the re-authentication icon is shown. BUG= 655221, 654300, 655308 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + xiyuan@chromium.org
On 2016/10/21 23:48:43, sammiequon wrote: > https://codereview.chromium.org/2427073002/diff/100001/ui/login/account_picke... > File ui/login/account_picker/screen_account_picker.js (right): > > https://codereview.chromium.org/2427073002/diff/100001/ui/login/account_picke... > ui/login/account_picker/screen_account_picker.js:240: // will appear for any > user if their is at least one user who has PIN > On 2016/10/21 22:27:41, jdufault wrote: > > Disable pin => Disable PIN > > for users which => for users who > > their => there > > Done. xiyuan@ - Please take a look. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org Link to the patchset: https://codereview.chromium.org/2427073002/#ps120001 (title: "Fixed patch set 5 errors.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sammiequon@chromium.org
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/2427073002/#ps140001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== cros: Fixed 3 bugs for pin unlock. User name would flash on startup before the rest of the user pod. Pin keyboard would show up for users with disabled keyboards and the easy unlock icon would clash with the re-authentication icon. I hide the easy unlock icon whenever the re-authentication icon is shown. BUG= 655221, 654300, 655308 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Fixed 3 bugs for pin unlock. User name would flash on startup before the rest of the user pod. Pin keyboard would show up for users with disabled keyboards and the easy unlock icon would clash with the re-authentication icon. I hide the easy unlock icon whenever the re-authentication icon is shown. BUG= 655221, 654300, 655308 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== cros: Fixed 3 bugs for pin unlock. User name would flash on startup before the rest of the user pod. Pin keyboard would show up for users with disabled keyboards and the easy unlock icon would clash with the re-authentication icon. I hide the easy unlock icon whenever the re-authentication icon is shown. BUG= 655221, 654300, 655308 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Fixed 3 bugs for pin unlock. User name would flash on startup before the rest of the user pod. Pin keyboard would show up for users with disabled keyboards and the easy unlock icon would clash with the re-authentication icon. I hide the easy unlock icon whenever the re-authentication icon is shown. BUG= 655221, 654300, 655308 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/a77a4ddde837ba57fca3195127cd4f1ad8f96eab Cr-Commit-Position: refs/heads/master@{#427152} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/a77a4ddde837ba57fca3195127cd4f1ad8f96eab Cr-Commit-Position: refs/heads/master@{#427152} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
