Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2698773004:
Settings: Hide fingerprint setup from options. (Closed)
|
Created:
3 years, 10 months ago by sammiequon Modified:
3 years, 9 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSettings: Hide fingerprint setup from options.
Currently the fingerprint setup shows up on options, but does not work properly, possibly due to reliance on MD Settings code. Hide the fingerprint stuff on options, it is not needed for options. Also, the enable screen lock toggle was showing when it should be hidden.
TEST=manual
BUG=693144
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2698773004
Cr-Commit-Position: refs/heads/master@{#453275}
Committed: https://chromium.googlesource.com/chromium/src/+/9c896117345da9d357b33abb157f9882597094b1
Patch Set 1 #Patch Set 2 : Rebased. #
Total comments: 2
Patch Set 3 : Fixed patch set 2 errors. #
Total comments: 2
Patch Set 4 : Fixed patch set 2 errors. #
Total comments: 2
Patch Set 5 : Fixed patch set 4 errors. #Patch Set 6 : Fixed patch set 4 errors. #Patch Set 7 : Rebased. #
Messages
Total messages: 40 (30 generated)
Description was changed from ========== settings: Hide fingerprint stuff from options. Currently the fingerprint stuff shows up on options, but does not work properly, possibly due to reliance on MD Settings code. Hide the fingerprint stuff on options, it is not needed for options. TEST=manual BUG=693144 ========== to ========== settings: Hide fingerprint stuff from options. Currently the fingerprint stuff shows up on options, but does not work properly, possibly due to reliance on MD Settings code. Hide the fingerprint stuff on options, it is not needed for options. TEST=manual BUG=693144 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== settings: Hide fingerprint stuff from options. Currently the fingerprint stuff shows up on options, but does not work properly, possibly due to reliance on MD Settings code. Hide the fingerprint stuff on options, it is not needed for options. TEST=manual BUG=693144 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== settings: Hide fingerprint stuff from options. Currently the fingerprint stuff shows up on options, but does not work properly, possibly due to reliance on MD Settings code. Hide the fingerprint stuff on options, it is not needed for options. Also, the enable screen lock toggle was showing when it should be hidden. TEST=manual BUG=693144 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== settings: Hide fingerprint stuff from options. Currently the fingerprint stuff shows up on options, but does not work properly, possibly due to reliance on MD Settings code. Hide the fingerprint stuff on options, it is not needed for options. Also, the enable screen lock toggle was showing when it should be hidden. TEST=manual BUG=693144 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== settings: Hide fingerprint setup from options. Currently the fingerprint setup shows up on options, but does not work properly, possibly due to reliance on MD Settings code. Hide the fingerprint stuff on options, it is not needed for options. Also, the enable screen lock toggle was showing when it should be hidden. TEST=manual BUG=693144 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== settings: Hide fingerprint setup from options. Currently the fingerprint setup shows up on options, but does not work properly, possibly due to reliance on MD Settings code. Hide the fingerprint stuff on options, it is not needed for options. Also, the enable screen lock toggle was showing when it should be hidden. TEST=manual BUG=693144 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Settings: Hide fingerprint setup from options. Currently the fingerprint setup shows up on options, but does not work properly, possibly due to reliance on MD Settings code. Hide the fingerprint stuff on options, it is not needed for options. Also, the enable screen lock toggle was showing when it should be hidden. TEST=manual BUG=693144 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) closure_compilation on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
sammiequon@chromium.org changed reviewers: + stevenjb@chromium.org
stevenjb@ - Please take a look. Thanks!
https://codereview.chromium.org/2698773004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2698773004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:53: <div class="fingerprint-div"> For unique identifiers use an id, not a class, e.g. fingerprintDiv.
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.
https://codereview.chromium.org/2698773004/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2698773004/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:53: <div class="fingerprint-div"> On 2017/02/22 22:07:45, stevenjb wrote: > For unique identifiers use an id, not a class, e.g. fingerprintDiv. Done.
stevenjb@google.com changed reviewers: + stevenjb@google.com
https://codereview.chromium.org/2698773004/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js (right): https://codereview.chromium.org/2698773004/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:68: var checkbox = lockScreen.root.querySelector('div.settings-box'); Why this selector change? It seems fragile. It wasn't a great selector before (and it looks like maybe it didn't work? I don't see it in lock_screen.html). It's also not a great variable name; I don't see any checkboxes in lock_screen.html, I assume it's for the toggle? A unique id might be a good idea here also.
https://codereview.chromium.org/2698773004/diff/40001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js (right): https://codereview.chromium.org/2698773004/diff/40001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:68: var checkbox = lockScreen.root.querySelector('div.settings-box'); On 2017/02/23 17:10:57, stevenjb (google-dont-use) wrote: > Why this selector change? It seems fragile. It wasn't a great selector before > (and it looks like maybe it didn't work? I don't see it in lock_screen.html). > It's also not a great variable name; I don't see any checkboxes in > lock_screen.html, I assume it's for the toggle? A unique id might be a good idea > here also. > Yeah, I noticed it was not working while working on this CL so I tweaked it to work. Changed to use unique id.
If we just need to add or move the comment, this lgtm. https://codereview.chromium.org/2698773004/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js (right): https://codereview.chromium.org/2698773004/diff/60001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:69: screenLockDiv.hidden = true; One more thing - did we / do we / should we show this somewhere, somehow? We should either move the comment in line 71 above this (and elim any blank lines between these) or add its own comment.
Thanks! https://codereview.chromium.org/2698773004/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js (right): https://codereview.chromium.org/2698773004/diff/60001/chrome/browser/resource... chrome/browser/resources/options/chromeos/quick_unlock_configure_overlay.js:69: screenLockDiv.hidden = true; On 2017/02/23 23:19:22, stevenjb wrote: > One more thing - did we / do we / should we show this somewhere, somehow? We > should either move the comment in line 71 above this (and elim any blank lines > between these) or add its own comment. This setting is located in the sync page in options, so the ported lock-screen version is a duplicate. Added a comment.
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...
Great, thanks! lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2698773004/#ps120001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488221472625790,
"parent_rev": "6c951afb46ca38b4740049ee852ab6e877fd10c9", "commit_rev":
"9c896117345da9d357b33abb157f9882597094b1"}
Message was sent while issue was closed.
Description was changed from ========== Settings: Hide fingerprint setup from options. Currently the fingerprint setup shows up on options, but does not work properly, possibly due to reliance on MD Settings code. Hide the fingerprint stuff on options, it is not needed for options. Also, the enable screen lock toggle was showing when it should be hidden. TEST=manual BUG=693144 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Settings: Hide fingerprint setup from options. Currently the fingerprint setup shows up on options, but does not work properly, possibly due to reliance on MD Settings code. Hide the fingerprint stuff on options, it is not needed for options. Also, the enable screen lock toggle was showing when it should be hidden. TEST=manual BUG=693144 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2698773004 Cr-Commit-Position: refs/heads/master@{#453275} Committed: https://chromium.googlesource.com/chromium/src/+/9c896117345da9d357b33abb157f... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/9c896117345da9d357b33abb157f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
