Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2538303002:
md-settings: Added settings for fingerprint unlock. (Closed)
|
Created:
4 years ago by sammiequon Modified:
3 years, 10 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, extensions-reviews_chromium.org, alemate+watch_chromium.org, michaelpg+watch-md-settings_chromium.org, achuith+watch_chromium.org, michaelpg+watch-md-ui_chromium.org, michaelpg+watch-options_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmd-settings: Added settings for fingerprint unlock.
The mocks are not finished yet, but this CL has some of the basic logics implemented. Everything is hidden behind the feature flag QuickUnlockFingerprint.
current mocks - https://docs.google.com/a/google.com/presentation/d/1XEdVznkCAunQKF_AK5bTPgeLFUKmhKp9ntaqm2hKfXY/edit?usp=sharing : Slide 13 - 22
BUG=672269
TEST=NONE
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2538303002
Cr-Commit-Position: refs/heads/master@{#447370}
Committed: https://chromium.googlesource.com/chromium/src/+/8af85f21e7a0ff7be805f3ed4e56b96fe3b272d7
Patch Set 1 #
Total comments: 30
Patch Set 2 : Fixed patch set 1 errors. #
Total comments: 13
Patch Set 3 : Fixed patch set 2 errors. #Patch Set 4 : Added flags to histograms.xml. #
Total comments: 12
Patch Set 5 : Fixed patch set 4 errors. #Patch Set 6 : Rebased. #
Total comments: 6
Patch Set 7 : Fixed patch set 6 errors. #
Total comments: 10
Patch Set 8 : Fixed patch set 7 errors. #
Total comments: 29
Patch Set 9 : Fixed patch set 8 errors. #Patch Set 10 : Fixed patch set 8 errors. #
Total comments: 2
Patch Set 11 : Rebased. #Patch Set 12 : Fixed patch set 10 errors. #
Total comments: 2
Patch Set 13 : Rebased. #Patch Set 14 : Nit. #
Total comments: 36
Patch Set 15 : Rebased. #Patch Set 16 : Fixed patch set 14 errors. #Patch Set 17 : Rebased. #Patch Set 18 : Modified the flags. #
Total comments: 21
Patch Set 19 : Fixed patch set 18 errors. #Patch Set 20 : Rebased. #
Total comments: 10
Patch Set 21 : Rebased. #Patch Set 22 : Fixed patch set 20 errors. #Patch Set 23 : Reparent to master. #Patch Set 24 : Rebased. #Patch Set 25 : Fixed patch set 22 errors. #
Total comments: 4
Patch Set 26 : Fixed patch set 25 errors. #Patch Set 27 : Rebased. #Messages
Total messages: 134 (88 generated)
Description was changed from ========== cros: Added settings for fingerprint unlock. The mocks are not finished yet, but this CL has some of the basic logics implemented. Everything is hidden behind the feature flag QuickUnlockFingerprint. BUG= TEST=NONE ========== to ========== cros: Added settings for fingerprint unlock. The mocks are not finished yet, but this CL has some of the basic logics implemented. Everything is hidden behind the feature flag QuickUnlockFingerprint. BUG= TEST=NONE 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: This issue passed the CQ dry run.
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...
Description was changed from ========== cros: Added settings for fingerprint unlock. The mocks are not finished yet, but this CL has some of the basic logics implemented. Everything is hidden behind the feature flag QuickUnlockFingerprint. BUG= TEST=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Added settings for fingerprint unlock. The mocks are not finished yet, but this CL has some of the basic logics implemented. Everything is hidden behind the feature flag QuickUnlockFingerprint. BUG= TEST=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jdufault@ - Please take a look. Thanks!
https://codereview.chromium.org/2538303002/diff/20001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/app/settings_str... chrome/app/settings_strings.grdp:2119: Finger The number should be a parameter in the string. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:35: prefs::kEnableQuickUnlockFingerprint, false, I believe the pref is syncable by default, so this flag is not needed (also drop the include). https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:69: return base::FeatureList::IsEnabled(features::kQuickUnlockPin) && Why are these tied together like this? https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/prefs_util.cc:261: (*s_whitelist)[::prefs::kEnableAutoScreenLock] = This looks like a duplicate entry. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/prefs_util.cc:263: (*s_whitelist)[::prefs::kEnableQuickUnlockFingerprint] = This should go in people section? https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/fingerprint_list.html (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.html:9: <iron-iconset-svg name="fingerprint" size="24"> There's a standard comment when the svg is defined locally explaining why. Please add it here. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.html:25: display: flex; I'm going to assume most of the CSS here will change when you have finalized mocks. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.html:39: <div id="list-area"> Do you need all of these ids, "root", "list-area"? https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.js:13: * <settings-fingerprint-list></settings-fingerprint-list> Include items binding in your example. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.js:72: var isRtl = document.dir == 'rtl'; There is a lot of complexity here to try to figure out the lowest numbered finger - I'm also not sure it will work properly in every locale. We can probably just ignore the problem; if the user has already renamed some fingerprints, they are probably going to rename the new one too. So what about just using the length of items to get the next fingerprint id? For example, if we have fingers: "Left" "Right" Adding a new one will give "Left" "Right" "Finger 3" https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.js:90: this.push('items', { 'name': isRtl ? index + ' ' + defaultName : Use a string format parameter to handle this logic; special case the first finger to use a different i18n resource. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.js:95: * Deletes a fingerprint from |items|. Should signal to the fingerprint module What is the "fingerprint module"? // TODO(sammiequon): Remove fingerprint using private API once it is ready. See crbug.com/xxx. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:48: label="Enable fingerprint login and authentication"> Localize this https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.js (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.js:80: fingerprintUnlockEnabled_: { I think it makes a little bit more sense to handle this inside of lock_screen; people_page doesn't have any logic that needs this value. It may be nice to be able to externally control if fingerprint is enabled/disabled for testing though, but that can be added back pretty easily if needed. quickUnlockEnabled_ is defined here because it changes the structure of people page (it adds or removes a settings-subpage).
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2538303002/diff/20001/chrome/app/settings_str... File chrome/app/settings_strings.grdp (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/app/settings_str... chrome/app/settings_strings.grdp:2119: Finger On 2016/12/01 17:11:19, jdufault wrote: > The number should be a parameter in the string. Done. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:35: prefs::kEnableQuickUnlockFingerprint, false, On 2016/12/01 17:11:19, jdufault wrote: > I believe the pref is syncable by default, so this flag is not needed (also drop > the include). Done. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:69: return base::FeatureList::IsEnabled(features::kQuickUnlockPin) && On 2016/12/01 17:11:19, jdufault wrote: > Why are these tied together like this? The fingerprint settings are under the lock screen so we cannot set it up without kQuickUnlockPin enabled so it is not usuable with kQuickUnlockPin turned off. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/settings_private/prefs_util.cc (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/prefs_util.cc:261: (*s_whitelist)[::prefs::kEnableAutoScreenLock] = On 2016/12/01 17:11:19, jdufault wrote: > This looks like a duplicate entry. Done. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/settings_private/prefs_util.cc:263: (*s_whitelist)[::prefs::kEnableQuickUnlockFingerprint] = On 2016/12/01 17:11:19, jdufault wrote: > This should go in people section? Oops. Done. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/fingerprint_list.html (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.html:9: <iron-iconset-svg name="fingerprint" size="24"> On 2016/12/01 17:11:19, jdufault wrote: > There's a standard comment when the svg is defined locally explaining why. > Please add it here. Done. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.html:25: display: flex; On 2016/12/01 17:11:19, jdufault wrote: > I'm going to assume most of the CSS here will change when you have finalized > mocks. Yes. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.html:39: <div id="list-area"> On 2016/12/01 17:11:19, jdufault wrote: > Do you need all of these ids, "root", "list-area"? Nope. This were expected to be needed for the final mocks. Removed. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.js:13: * <settings-fingerprint-list></settings-fingerprint-list> On 2016/12/01 17:11:20, jdufault wrote: > Include items binding in your example. Done. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.js:72: var isRtl = document.dir == 'rtl'; On 2016/12/01 17:11:19, jdufault wrote: > There is a lot of complexity here to try to figure out the lowest numbered > finger - I'm also not sure it will work properly in every locale. We can > probably just ignore the problem; if the user has already renamed some > fingerprints, they are probably going to rename the new one too. > > So what about just using the length of items to get the next fingerprint id? > > For example, if we have fingers: > "Left" > "Right" > > Adding a new one will give > "Left" > "Right" > "Finger 3" Done. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.js:90: this.push('items', { 'name': isRtl ? index + ' ' + defaultName : On 2016/12/01 17:11:19, jdufault wrote: > Use a string format parameter to handle this logic; special case the first > finger to use a different i18n resource. Done. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.js:95: * Deletes a fingerprint from |items|. Should signal to the fingerprint module On 2016/12/01 17:11:20, jdufault wrote: > What is the "fingerprint module"? > > // TODO(sammiequon): Remove fingerprint using private API once it is ready. See > crbug.com/xxx. Done. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.html:48: label="Enable fingerprint login and authentication"> On 2016/12/01 17:11:20, jdufault wrote: > Localize this Done. https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/people_page.js (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/people_page/people_page.js:80: fingerprintUnlockEnabled_: { On 2016/12/01 17:11:20, jdufault wrote: > I think it makes a little bit more sense to handle this inside of lock_screen; > people_page doesn't have any logic that needs this value. > > It may be nice to be able to externally control if fingerprint is > enabled/disabled for testing though, but that can be added back pretty easily if > needed. > > quickUnlockEnabled_ is defined here because it changes the structure of people > page (it adds or removes a settings-subpage). 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:69: return base::FeatureList::IsEnabled(features::kQuickUnlockPin) && On 2016/12/01 20:52:29, sammiequon wrote: > On 2016/12/01 17:11:19, jdufault wrote: > > Why are these tied together like this? > > The fingerprint settings are under the lock screen so we cannot set it up > without kQuickUnlockPin enabled so it is not usuable with kQuickUnlockPin turned > off. You should update the comment with that info. https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/fingerprint_list.html (right): https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.html:4: <link rel="import" href="chrome://resources/polymer/v1_0/iron-iconset-svg/iron-iconset-svg.html"> Drop import after addressing comment below https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.html:17: <path d="M6 19c0 1.1.9 2 2 2h8c1.1 0 2-.9 2-2V7H6v12zM19 4h-3.5l-1-1h-5l-1 1H5v2h14V4z"> Reuse existing icon in [1] instead of redefining it (as described in the goo.gl link). 1: https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/icons.htm... https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.js:63: onAddFingerprint_: function() { If you invert the first for loop this function can be significantly simplified. onAddFingerprint_: function() { // Find and add the first unused fingerprint name. for (var i = 0; i < MAX_NUMBER_FINGERPRINTS_ALLOWED; ++i) { var fingerprintName = this.i18n('lockScreenFingerprintName', i); if (this.items.includes(fingerprintName)) continue; this.push('items', { 'name' : fingerprintName }); break; } } https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.js:109: canAddNewFingerprint_: function(length) { It seems like passing only the items length makes this function a leaky abstraction; I think it would be better to pass the entire |items| array. https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.js (right): https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.js:48: return loadTimeData.getBoolean('fingerprintUnlockEnabled'); This is not defined inside of options so all of the options related browser tests are failing. https://codereview.chromium.org/2538303002/diff/60001/chrome/common/chrome_fe... File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2538303002/diff/60001/chrome/common/chrome_fe... chrome/common/chrome_features.cc:228: base::FEATURE_DISABLED_BY_DEFAULT}; You need to also add an entry in about_flags.cc so this will show up in chrome://flags
Forgot to rebase. Sorry! https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2538303002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:69: return base::FeatureList::IsEnabled(features::kQuickUnlockPin) && On 2016/12/02 23:02:11, jdufault wrote: > On 2016/12/01 20:52:29, sammiequon wrote: > > On 2016/12/01 17:11:19, jdufault wrote: > > > Why are these tied together like this? > > > > The fingerprint settings are under the lock screen so we cannot set it up > > without kQuickUnlockPin enabled so it is not usuable with kQuickUnlockPin > turned > > off. > > You should update the comment with that info. Done. https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/fingerprint_list.html (right): https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.html:4: <link rel="import" href="chrome://resources/polymer/v1_0/iron-iconset-svg/iron-iconset-svg.html"> On 2016/12/02 23:02:11, jdufault wrote: > Drop import after addressing comment below Done. https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.html:17: <path d="M6 19c0 1.1.9 2 2 2h8c1.1 0 2-.9 2-2V7H6v12zM19 4h-3.5l-1-1h-5l-1 1H5v2h14V4z"> On 2016/12/02 23:02:11, jdufault wrote: > Reuse existing icon in [1] instead of redefining it (as described in the goo.gl > link). > > 1: > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/icons.htm... Done. https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.js:63: onAddFingerprint_: function() { On 2016/12/02 23:02:12, jdufault wrote: > If you invert the first for loop this function can be significantly simplified. > > onAddFingerprint_: function() { > // Find and add the first unused fingerprint name. > for (var i = 0; i < MAX_NUMBER_FINGERPRINTS_ALLOWED; ++i) { > var fingerprintName = this.i18n('lockScreenFingerprintName', i); > if (this.items.includes(fingerprintName)) > continue; > this.push('items', { 'name' : fingerprintName }); > break; > } > } Done. https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.js:109: canAddNewFingerprint_: function(length) { On 2016/12/02 23:02:12, jdufault wrote: > It seems like passing only the items length makes this function a leaky > abstraction; I think it would be better to pass the entire |items| array. It seems like pop & slice do not notify the |items| has changed and only |items.length|. https://www.polymer-project.org/1.0/docs/devguide/model-data#override-dirty-c... https://www.polymer-project.org/1.0/docs/api/Polymer.Base#method-notifySplices can achieve this though something needs to be called after every array change. What do you think? https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/lock_screen.js (right): https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/lock_screen.js:48: return loadTimeData.getBoolean('fingerprintUnlockEnabled'); On 2016/12/02 23:02:12, jdufault wrote: > This is not defined inside of options so all of the options related browser > tests are failing. Done. Yeah i noticed this after the trybots returned negative. https://codereview.chromium.org/2538303002/diff/60001/chrome/common/chrome_fe... File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2538303002/diff/60001/chrome/common/chrome_fe... chrome/common/chrome_features.cc:228: base::FEATURE_DISABLED_BY_DEFAULT}; On 2016/12/02 23:02:12, jdufault wrote: > You need to also add an entry in about_flags.cc so this will show up in > chrome://flags 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_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.
https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/people_page/fingerprint_list.js:109: canAddNewFingerprint_: function(length) { On 2016/12/03 21:40:51, sammiequon wrote: > On 2016/12/02 23:02:12, jdufault wrote: > > It seems like passing only the items length makes this function a leaky > > abstraction; I think it would be better to pass the entire |items| array. > > It seems like pop & slice do not notify the |items| has changed and only > |items.length|. > > https://www.polymer-project.org/1.0/docs/devguide/model-data#override-dirty-c... > https://www.polymer-project.org/1.0/docs/api/Polymer.Base#method-notifySplices > > can achieve this though something needs to be called after every array change. > What do you think? There is also items.splices that you could listen to, but I don't think that would be much better than listening to items.length. Feel free to leave as is. 1: https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... https://codereview.chromium.org/2538303002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:13: * <settings-fingerprint-list items="{{items}}"></settings-fingerprint-list> should this be fingerprints="{{items}}"? Actually, it looks like fingerprints is private so it should not be in the example. Sorry about the ring-around. https://codereview.chromium.org/2538303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:39: fingerprints: { Add _ to the end of the name. https://codereview.chromium.org/2538303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:66: for (var i = 0; i < this.fingerprints.length; ++i) { You could also implement this as: return !!this.fingerprints.find(function (e) { return e.name == name; }); https://codereview.chromium.org/2538303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:67: if (this.fingerprints[i]['name'] == name) Replace ['name'] with .name https://codereview.chromium.org/2538303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:79: // should be in the format "Finger x" where x should be the lowest unused Update comment (the discussion about the required format of the string is not needed/confusing). https://codereview.chromium.org/2538303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:84: var fingerprintName = this.i18n('lockScreenFingerprintNewName', 1); Do not seed an initial fingerprintName value.
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 sammiequon@chromium.org
Patchset #5 (id:120001) has been deleted
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
https://codereview.chromium.org/2538303002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:13: * <settings-fingerprint-list items="{{items}}"></settings-fingerprint-list> On 2016/12/05 22:29:36, jdufault wrote: > should this be fingerprints="{{items}}"? > > Actually, it looks like fingerprints is private so it should not be in the > example. Sorry about the ring-around. Done. https://codereview.chromium.org/2538303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:39: fingerprints: { On 2016/12/05 22:29:36, jdufault wrote: > Add _ to the end of the name. Done. https://codereview.chromium.org/2538303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:66: for (var i = 0; i < this.fingerprints.length; ++i) { On 2016/12/05 22:29:36, jdufault wrote: > You could also implement this as: > > > return !!this.fingerprints.find(function (e) { > return e.name == name; > }); Done. https://codereview.chromium.org/2538303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:67: if (this.fingerprints[i]['name'] == name) On 2016/12/05 22:29:36, jdufault wrote: > Replace ['name'] with .name Done. https://codereview.chromium.org/2538303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:79: // should be in the format "Finger x" where x should be the lowest unused On 2016/12/05 22:29:36, jdufault wrote: > Update comment (the discussion about the required format of the string is not > needed/confusing). Done. https://codereview.chromium.org/2538303002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:84: var fingerprintName = this.i18n('lockScreenFingerprintNewName', 1); On 2016/12/05 22:29:36, jdufault wrote: > Do not seed an initial fingerprintName value. Done.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #6 (id:160001) 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: Try jobs failed on following builders: 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_...)
Patchset #6 (id:180001) 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.
Make sure you are coordinating with xiaoyinh@ in regards to the flag. https://codereview.chromium.org/2538303002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:10: * items: The fingerprints to display. Remove properties doc https://codereview.chromium.org/2538303002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2538303002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:47: pref="{{prefs.settings.enable_quick_unlock_fingerprint}}" 4 space indent? https://codereview.chromium.org/2538303002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:52: <settings-fingerprint-list></settings-fingerprint-list> Indentation looks a bit wonky here.
Will do. https://codereview.chromium.org/2538303002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:10: * items: The fingerprints to display. On 2016/12/06 17:35:07, jdufault wrote: > Remove properties doc Done. https://codereview.chromium.org/2538303002/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2538303002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:47: pref="{{prefs.settings.enable_quick_unlock_fingerprint}}" On 2016/12/06 17:35:07, jdufault wrote: > 4 space indent? Thanks. Thought the editor would handle it. Done. https://codereview.chromium.org/2538303002/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:52: <settings-fingerprint-list></settings-fingerprint-list> On 2016/12/06 17:35:07, jdufault wrote: > Indentation looks a bit wonky here. Done.
lgtm https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:30: // Returns true if the fingerprint unlock feature flag is present. nit: newline above https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:79: if (this.isNameUsed_(fingerprintName)) A more general way to handle this would be if (!this.findFingerprintByName_(fingerprintName)) continue; where findFingerprintByName returns the entire fingerprint object. I don't think it is too important to refactor here, just something to keep in mind. https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:52: <settings-fingerprint-list></settings-fingerprint-list> <settings> should be indented 2 spaces instead of 4. https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_ui.cc:145: html_source->AddBoolean("fingerprintUnlockEnabled", Move above "androidAppsAllowed" call so this is next to "pinUnlockEnabled" https://codereview.chromium.org/2538303002/diff/220001/chrome/common/chrome_f... File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2538303002/diff/220001/chrome/common/chrome_f... chrome/common/chrome_features.cc:226: // Enables or disables PIN quick unlock settings integration. Can you update both of these comments to remove the "settings integration" string? They should just say // Enables or disables PIN quick unlock. // Enables or disables fingerprint quick unlock.
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...
sammiequon@chromium.org changed reviewers: + dbeam@chromium.org, stevenjb@chromium.org
dbeam@, stevenjb@ - Please take a look. Thanks! https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:30: // Returns true if the fingerprint unlock feature flag is present. On 2016/12/06 18:13:42, jdufault wrote: > nit: newline above Done. https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:79: if (this.isNameUsed_(fingerprintName)) On 2016/12/06 18:13:42, jdufault wrote: > A more general way to handle this would be > > if (!this.findFingerprintByName_(fingerprintName)) > continue; > > where findFingerprintByName returns the entire fingerprint object. > > I don't think it is too important to refactor here, just something to keep in > mind. Ok. https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:52: <settings-fingerprint-list></settings-fingerprint-list> On 2016/12/06 18:13:42, jdufault wrote: > <settings> should be indented 2 spaces instead of 4. Done. https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_ui.cc:145: html_source->AddBoolean("fingerprintUnlockEnabled", On 2016/12/06 18:13:42, jdufault wrote: > Move above "androidAppsAllowed" call so this is next to "pinUnlockEnabled" Done. https://codereview.chromium.org/2538303002/diff/220001/chrome/common/chrome_f... File chrome/common/chrome_features.cc (right): https://codereview.chromium.org/2538303002/diff/220001/chrome/common/chrome_f... chrome/common/chrome_features.cc:226: // Enables or disables PIN quick unlock settings integration. On 2016/12/06 18:13:42, jdufault wrote: > Can you update both of these comments to remove the "settings integration" > string? They should just say > > // Enables or disables PIN quick unlock. > > // Enables or disables fingerprint quick unlock. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/06 18:55:38, sammiequon wrote: > dbeam@, stevenjb@ - Please take a look. Thanks! > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/chromeo... > File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/chromeo... > chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:30: // Returns > true if the fingerprint unlock feature flag is present. > On 2016/12/06 18:13:42, jdufault wrote: > > nit: newline above > > Done. > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... > File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... > chrome/browser/resources/settings/people_page/fingerprint_list.js:79: if > (this.isNameUsed_(fingerprintName)) > On 2016/12/06 18:13:42, jdufault wrote: > > A more general way to handle this would be > > > > if (!this.findFingerprintByName_(fingerprintName)) > > continue; > > > > where findFingerprintByName returns the entire fingerprint object. > > > > I don't think it is too important to refactor here, just something to keep in > > mind. > > Ok. > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... > File chrome/browser/resources/settings/people_page/lock_screen.html (right): > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... > chrome/browser/resources/settings/people_page/lock_screen.html:52: > <settings-fingerprint-list></settings-fingerprint-list> > On 2016/12/06 18:13:42, jdufault wrote: > > <settings> should be indented 2 spaces instead of 4. > > Done. > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/ui/webu... > chrome/browser/ui/webui/settings/md_settings_ui.cc:145: > html_source->AddBoolean("fingerprintUnlockEnabled", > On 2016/12/06 18:13:42, jdufault wrote: > > Move above "androidAppsAllowed" call so this is next to "pinUnlockEnabled" > > Done. > > https://codereview.chromium.org/2538303002/diff/220001/chrome/common/chrome_f... > File chrome/common/chrome_features.cc (right): > > https://codereview.chromium.org/2538303002/diff/220001/chrome/common/chrome_f... > chrome/common/chrome_features.cc:226: // Enables or disables PIN quick unlock > settings integration. > On 2016/12/06 18:13:42, jdufault wrote: > > Can you update both of these comments to remove the "settings integration" > > string? They should just say > > > > // Enables or disables PIN quick unlock. > > > > // Enables or disables fingerprint quick unlock. > > Done. This issue needs a BUG= (Which hopefully has some preliminary mocks at least?)
Description was changed from ========== cros: Added settings for fingerprint unlock. The mocks are not finished yet, but this CL has some of the basic logics implemented. Everything is hidden behind the feature flag QuickUnlockFingerprint. BUG= TEST=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Added settings for fingerprint unlock. The mocks are not finished yet, but this CL has some of the basic logics implemented. Everything is hidden behind the feature flag QuickUnlockFingerprint. current mocks - https://docs.google.com/a/google.com/presentation/d/1XEdVznkCAunQKF_AK5bTPgeL... BUG=672269 TEST=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== cros: Added settings for fingerprint unlock. The mocks are not finished yet, but this CL has some of the basic logics implemented. Everything is hidden behind the feature flag QuickUnlockFingerprint. current mocks - https://docs.google.com/a/google.com/presentation/d/1XEdVznkCAunQKF_AK5bTPgeL... BUG=672269 TEST=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Added settings for fingerprint unlock. The mocks are not finished yet, but this CL has some of the basic logics implemented. Everything is hidden behind the feature flag QuickUnlockFingerprint. current mocks - https://docs.google.com/a/google.com/presentation/d/1XEdVznkCAunQKF_AK5bTPgeL... : Slide 13 - 22 BUG=672269 TEST=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
On 2016/12/07 23:12:42, stevenjb wrote: > On 2016/12/06 18:55:38, sammiequon wrote: > > dbeam@, stevenjb@ - Please take a look. Thanks! > > > > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/chromeo... > > File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): > > > > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/chromeo... > > chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:30: // Returns > > true if the fingerprint unlock feature flag is present. > > On 2016/12/06 18:13:42, jdufault wrote: > > > nit: newline above > > > > Done. > > > > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... > > File chrome/browser/resources/settings/people_page/fingerprint_list.js > (right): > > > > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... > > chrome/browser/resources/settings/people_page/fingerprint_list.js:79: if > > (this.isNameUsed_(fingerprintName)) > > On 2016/12/06 18:13:42, jdufault wrote: > > > A more general way to handle this would be > > > > > > if (!this.findFingerprintByName_(fingerprintName)) > > > continue; > > > > > > where findFingerprintByName returns the entire fingerprint object. > > > > > > I don't think it is too important to refactor here, just something to keep > in > > > mind. > > > > Ok. > > > > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... > > File chrome/browser/resources/settings/people_page/lock_screen.html (right): > > > > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/resourc... > > chrome/browser/resources/settings/people_page/lock_screen.html:52: > > <settings-fingerprint-list></settings-fingerprint-list> > > On 2016/12/06 18:13:42, jdufault wrote: > > > <settings> should be indented 2 spaces instead of 4. > > > > Done. > > > > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/ui/webu... > > File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): > > > > > https://codereview.chromium.org/2538303002/diff/220001/chrome/browser/ui/webu... > > chrome/browser/ui/webui/settings/md_settings_ui.cc:145: > > html_source->AddBoolean("fingerprintUnlockEnabled", > > On 2016/12/06 18:13:42, jdufault wrote: > > > Move above "androidAppsAllowed" call so this is next to "pinUnlockEnabled" > > > > Done. > > > > > https://codereview.chromium.org/2538303002/diff/220001/chrome/common/chrome_f... > > File chrome/common/chrome_features.cc (right): > > > > > https://codereview.chromium.org/2538303002/diff/220001/chrome/common/chrome_f... > > chrome/common/chrome_features.cc:226: // Enables or disables PIN quick unlock > > settings integration. > > On 2016/12/06 18:13:42, jdufault wrote: > > > Can you update both of these comments to remove the "settings integration" > > > string? They should just say > > > > > > // Enables or disables PIN quick unlock. > > > > > > // Enables or disables fingerprint quick unlock. > > > > Done. > > This issue needs a BUG= > (Which hopefully has some preliminary mocks at least?) Added a bug.
We should have some basic tests for this page as well, see people_page_*_test.js for other examples. https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.html (right): https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:4: <link rel="import" href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button.html"> +paper-button https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:11: <style> Only one style section: <style include="settings-shared"> page-specific-styling... </style> https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:28: <div> div needed? https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:29: <template is="dom-repeat" items="{{fingerprints_}}"> Does this have the expected keyboard focus behavior? You may want to use iron-list instead to get that (see other examples of iron-list and dom-repeat in Settings). https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:36: <div class="settings-box continuation radio-indent"> Use "settings-box first" since this is the first settings-box. (Styling is the same as continuation). https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:38: disabled="[[!canAddNewFingerprint_(fingerprints_.length)]]"> 4 space indent https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:45: </paper-button> This is a little odd, having conditional divs inside a button. Can we make the button conditional and just show formatted text when disabled instead? https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:34: * @private Combine lines: @private {Array<Object>} https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:57: * Check if the registered fingerprints has a fingerprint with name |name|. s/the registered fingerprints/|fingerprints_|/ (or 'the list of registered fingerprints') https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:74: // TODO(sammiequon): Add fingerprint using private API once it is ready. Rather than committing fake code in the initial UI, consider implementing a fake interface that can later be replaced with the real interface once implemented. That will require less fake code here, and will assist in testing the UI later (i.e. we can use the fake interface implementation in tests). https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:783: chromeos::IsFingerprintUnlockEnabled()); Is this implemented in the options UI? I only see changes in md-settings.
Patchset #9 (id:260001) has been deleted
I do have some basic tests for this in another CL not sent out yet. It is dependent on another CL which is dependent on this CL. I can merge the relevant parts if you prefer. https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.html (right): https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:4: <link rel="import" href="chrome://resources/polymer/v1_0/paper-icon-button/paper-icon-button.html"> On 2016/12/07 23:25:40, stevenjb wrote: > +paper-button Done. https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:11: <style> On 2016/12/07 23:25:40, stevenjb wrote: > Only one style section: > > <style include="settings-shared"> > page-specific-styling... > </style> Done. https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:28: <div> On 2016/12/07 23:25:40, stevenjb wrote: > div needed? Done. https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:29: <template is="dom-repeat" items="{{fingerprints_}}"> On 2016/12/07 23:25:40, stevenjb wrote: > Does this have the expected keyboard focus behavior? You may want to use > iron-list instead to get that (see other examples of iron-list and dom-repeat in > Settings). Yeah iron-list works a bit better with the tabbing. Done. https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:36: <div class="settings-box continuation radio-indent"> On 2016/12/07 23:25:40, stevenjb wrote: > Use "settings-box first" since this is the first settings-box. (Styling is the > same as continuation). > Done. https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:38: disabled="[[!canAddNewFingerprint_(fingerprints_.length)]]"> On 2016/12/07 23:25:40, stevenjb wrote: > 4 space indent Done. https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:45: </paper-button> On 2016/12/07 23:25:40, stevenjb wrote: > This is a little odd, having conditional divs inside a button. Can we make the > button conditional and just show formatted text when disabled instead? Done. https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:34: * @private On 2016/12/07 23:25:40, stevenjb wrote: > Combine lines: > @private {Array<Object>} Done. https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:57: * Check if the registered fingerprints has a fingerprint with name |name|. On 2016/12/07 23:25:40, stevenjb wrote: > s/the registered fingerprints/|fingerprints_|/ (or 'the list of registered > fingerprints') Done. https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:74: // TODO(sammiequon): Add fingerprint using private API once it is ready. On 2016/12/07 23:25:40, stevenjb wrote: > Rather than committing fake code in the initial UI, consider implementing a fake > interface that can later be replaced with the real interface once implemented. > That will require less fake code here, and will assist in testing the UI later > (i.e. we can use the fake interface implementation in tests). I do not think too much of the initial code here will be deleted at the end. Once the api is ready we just have signal it to enroll/delete a fingerprint. The stuff in this CL should hopefully be only altered minimally. https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:783: chromeos::IsFingerprintUnlockEnabled()); On 2016/12/07 23:25:40, stevenjb wrote: > Is this implemented in the options UI? I only see changes in md-settings. It's meant to be md-settings only, but it has to be under the lock_screen.html which is options only, so I add the data to options as well. Is there a better approach?
https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:74: // TODO(sammiequon): Add fingerprint using private API once it is ready. On 2016/12/08 03:50:50, sammiequon wrote: > On 2016/12/07 23:25:40, stevenjb wrote: > > Rather than committing fake code in the initial UI, consider implementing a > fake > > interface that can later be replaced with the real interface once implemented. > > That will require less fake code here, and will assist in testing the UI later > > (i.e. we can use the fake interface implementation in tests). > > I do not think too much of the initial code here will be deleted at the end. > Once the api is ready we just have signal it to enroll/delete a fingerprint. The > stuff in this CL should hopefully be only altered minimally. Shouldn't this whole function just be: chrome.fingerprintPrivate.addFingerprint()? Determining the name is something I would expect the model to do, not the UI. I would really much prefer to see this fake implementation in a fake api, i.e. fakeFingerprintPrivate. Same with onFingerprintDelete. https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:783: chromeos::IsFingerprintUnlockEnabled()); On 2016/12/08 03:50:50, sammiequon wrote: > On 2016/12/07 23:25:40, stevenjb wrote: > > Is this implemented in the options UI? I only see changes in md-settings. > > It's meant to be md-settings only, but it has to be under the lock_screen.html > which is options only, so I add the data to options as well. Is there a better > approach? Is this used in this CL? If so I missed it. If not, it should be added in the CL that uses it.
Description was changed from ========== cros: Added settings for fingerprint unlock. The mocks are not finished yet, but this CL has some of the basic logics implemented. Everything is hidden behind the feature flag QuickUnlockFingerprint. current mocks - https://docs.google.com/a/google.com/presentation/d/1XEdVznkCAunQKF_AK5bTPgeL... : Slide 13 - 22 BUG=672269 TEST=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== md-settings: Added settings for fingerprint unlock. The mocks are not finished yet, but this CL has some of the basic logics implemented. Everything is hidden behind the feature flag QuickUnlockFingerprint. current mocks - https://docs.google.com/a/google.com/presentation/d/1XEdVznkCAunQKF_AK5bTPgeL... : Slide 13 - 22 BUG=672269 TEST=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:74: // TODO(sammiequon): Add fingerprint using private API once it is ready. On 2016/12/08 16:49:40, stevenjb wrote: > On 2016/12/08 03:50:50, sammiequon wrote: > > On 2016/12/07 23:25:40, stevenjb wrote: > > > Rather than committing fake code in the initial UI, consider implementing a > > fake > > > interface that can later be replaced with the real interface once > implemented. > > > That will require less fake code here, and will assist in testing the UI > later > > > (i.e. we can use the fake interface implementation in tests). > > > > I do not think too much of the initial code here will be deleted at the end. > > Once the api is ready we just have signal it to enroll/delete a fingerprint. > The > > stuff in this CL should hopefully be only altered minimally. > > Shouldn't this whole function just be: > > chrome.fingerprintPrivate.addFingerprint()? > > Determining the name is something I would expect the model to do, not the UI. I > would really much prefer to see this fake implementation in a fake api, i.e. > fakeFingerprintPrivate. > > Same with onFingerprintDelete. I put the naming stuff here since the users change names here via paper-input so i thought it'll be nice to keep them in the same place. I'm not strongly against moving this all out into the fakeFingerprintPrivate (next CL) though. What do you think? https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:783: chromeos::IsFingerprintUnlockEnabled()); On 2016/12/08 16:49:41, stevenjb wrote: > On 2016/12/08 03:50:50, sammiequon wrote: > > On 2016/12/07 23:25:40, stevenjb wrote: > > > Is this implemented in the options UI? I only see changes in md-settings. > > > > It's meant to be md-settings only, but it has to be under the lock_screen.html > > which is options only, so I add the data to options as well. Is there a better > > approach? > > Is this used in this CL? If so I missed it. If not, it should be added in the CL > that uses it. Yes, lock_screen.html was added previously so this CL only needs to set the appropriate data.
https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:783: chromeos::IsFingerprintUnlockEnabled()); On 2016/12/08 20:22:40, sammiequon wrote: > On 2016/12/08 16:49:41, stevenjb wrote: > > On 2016/12/08 03:50:50, sammiequon wrote: > > > On 2016/12/07 23:25:40, stevenjb wrote: > > > > Is this implemented in the options UI? I only see changes in md-settings. > > > > > > It's meant to be md-settings only, but it has to be under the > lock_screen.html > > > which is options only, so I add the data to options as well. Is there a > better > > > approach? > > > > Is this used in this CL? If so I missed it. If not, it should be added in the > CL > > that uses it. > > Yes, lock_screen.html was added previously so this CL only needs to set the > appropriate data. Could you go ahead and separate those changes from this CL then? Thanks.
https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:783: chromeos::IsFingerprintUnlockEnabled()); On 2016/12/08 20:50:21, stevenjb wrote: > On 2016/12/08 20:22:40, sammiequon wrote: > > On 2016/12/08 16:49:41, stevenjb wrote: > > > On 2016/12/08 03:50:50, sammiequon wrote: > > > > On 2016/12/07 23:25:40, stevenjb wrote: > > > > > Is this implemented in the options UI? I only see changes in > md-settings. > > > > > > > > It's meant to be md-settings only, but it has to be under the > > lock_screen.html > > > > which is options only, so I add the data to options as well. Is there a > > better > > > > approach? > > > > > > Is this used in this CL? If so I missed it. If not, it should be added in > the > > CL > > > that uses it. > > > > Yes, lock_screen.html was added previously so this CL only needs to set the > > appropriate data. > > Could you go ahead and separate those changes from this CL then? Thanks. Hmmm, maybe I didn't explain properly in the previous comments. Originally I meant for this to be md-settings only but the trybots would not pass because lock_screen.html is in both md-settings and options and fingerprint_list.html is inside lock_screen.html. So I added this to the fingerprint resources to options as well (to get the trybots working). I'm told md-settings for chromeOS may not be ready in time so I tested that this works on options. But if you did understand, how do I seperate the options stuff and still pass the trybots? Thanks a bunch!
OK, I understand now. I requested a couple of comments that will help clarify that. I would still like to see some basic tests at least, either as part of quick_unlock_authenticate_browsertest_chromeos.js or as a separate test suite. https://codereview.chromium.org/2538303002/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/2538303002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_ui.cc:272: path_to_idr_map_[kFingerprintListJSPath] = IDR_OPTIONS_FINGERPRINT_LIST_JS; Could you add comments above kSetupPin and kFingerprint indicating that they are part of the LockScreen UI?
https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/2538303002/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:783: chromeos::IsFingerprintUnlockEnabled()); On 2016/12/08 21:44:01, sammiequon wrote: > Hmmm, maybe I didn't explain properly in the previous comments. Originally I > meant for this to be md-settings only We should make sure this works in both options and md-settings. Please test in both - I expect that options will pretty much just work, though. I'm not sure the current timeline for md-settings on CrOS, but md-settings has been delayed a few times now.
Patchset #11 (id:320001) has been deleted
stevenjb@ - I submitted you another CL to you with basic tests. I can merge it to this one if you prefer. jdufault@ - Yes it works on options. https://codereview.chromium.org/2538303002/diff/300001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/2538303002/diff/300001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_ui.cc:272: path_to_idr_map_[kFingerprintListJSPath] = IDR_OPTIONS_FINGERPRINT_LIST_JS; On 2016/12/09 17:17:43, stevenjb wrote: > Could you add comments above kSetupPin and kFingerprint indicating that they are > part of the LockScreen UI? Done.
lgtm https://codereview.chromium.org/2538303002/diff/360001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/2538303002/diff/360001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_ui.cc:269: // These are part of the LockScreen UI. nit: You may as well include the lock screen files above in this grouping.
sammiequon@chromium.org changed reviewers: + isherman@chromium.org
dbeam@ - Please take a look. Thanks! The file stevenjb@ doesn't have owners is c/b/r/options_resources.grd. isherman@ - Please take a look at histograms.xml. Thanks! https://codereview.chromium.org/2538303002/diff/360001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/2538303002/diff/360001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_ui.cc:269: // These are part of the LockScreen UI. On 2016/12/12 18:35:21, stevenjb wrote: > nit: You may as well include the lock screen files above in this grouping. Done.
histograms.xml lgtm
https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:73: return base::FeatureList::IsEnabled(features::kQuickUnlockPin) && can we rename the Pin version to just kQuickUnlock? https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.html (right): https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:20: } this is not the right color. we have a CSS variable for this (like --settings-border or something) if you control this DOM and it'll never be weird on you, just do something like: .list-item:not([index=0]) { @apply(--settings-border-var-name-or-whatever); } you also shouldn't be putting borders on top and bottom in settings, afaik https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:32: <paper-input value="{{item.name}}"></paper-input> why can't we just drop the {name:} stuff and just bind directly to value="{{item}}" here and just make fingerprints_ an !Array<string>? https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:33: <paper-icon-button icon="cr:delete" on-tap="onFingerprintDelete_"> could this be <button is="paper-icon-button-light"> instead? https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:39: <paper-button is="action-link" on-tap="onAddFingerprint_" what is this is="action-link" doing here? https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:10: * <settings-fingerprint-list></settings-fingerprint-list> can you drop this @fileoverview? we decided these don't really add much value and are just fluff https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:12: if we stick with the object wrapper with a name key: /** @typedef {{name: string}} */ var Fingerprint; or cr.exportPath('settings'); /** @typedef {{name: string}} */ settings.Fingerprint; https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:19: * @const nit: @const {number} or just @const https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:33: * @private {Array<Object>} !Array<!Fingerprint> https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:40: notify: true why do you need this notify true? it seems like you're using .push() and .splice() so that should be notifying for you... https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:71: this.push('fingerprints_', { 'name': fingerprintName }); this.push('fingerprints_', {'name': fingerprintName}); (no spaces around curlies) https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:71: this.push('fingerprints_', { 'name': fingerprintName }); wait, why are you using {name: <string>} as the item template? and not just ... <string>? https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:72: this.$.fingerprintsList.fire('iron-resize'); nit: notifyResize() instead of fire('iron-resize') https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:86: * Returns the text to be displayed for the add fingerprint button. @param https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:90: getFingerprintButtonText_: function(length) { can you name this argument something other than "length"? it kind of already exists. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:99: * Checks whether another fingerprint can be added. @param
https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:73: return base::FeatureList::IsEnabled(features::kQuickUnlockPin) && On 2016/12/13 19:53:23, Dan Beam wrote: > can we rename the Pin version to just kQuickUnlock? Should I add a TODO and bug or do you prefer to do it in this CL? https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.html (right): https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:20: } On 2016/12/13 19:53:23, Dan Beam wrote: > this is not the right color. we have a CSS variable for this (like > --settings-border or something) > > if you control this DOM and it'll never be weird on you, just do something like: > > .list-item:not([index=0]) { > @apply(--settings-border-var-name-or-whatever); > } > > you also shouldn't be putting borders on top and bottom in settings, afaik The updated (but I don't think final) mocks have no border. I keep these tips in mind though for futures. :) https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:32: <paper-input value="{{item.name}}"></paper-input> On 2016/12/13 19:53:23, Dan Beam wrote: > why can't we just drop the {name:} stuff and just bind directly to > value="{{item}}" here and just make fingerprints_ an !Array<string>? I was expecting the list to have more than just a 'name' value later, but we can change that if it ever happens later. Done. https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:33: <paper-icon-button icon="cr:delete" on-tap="onFingerprintDelete_"> On 2016/12/13 19:53:23, Dan Beam wrote: > could this be <button is="paper-icon-button-light"> instead? Done. https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:39: <paper-button is="action-link" on-tap="onAddFingerprint_" On 2016/12/13 19:53:23, Dan Beam wrote: > what is this is="action-link" doing here? Done. https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:10: * <settings-fingerprint-list></settings-fingerprint-list> On 2016/12/13 19:53:23, Dan Beam wrote: > can you drop this @fileoverview? we decided these don't really add much value > and are just fluff Done. https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:12: On 2016/12/13 19:53:24, Dan Beam wrote: > if we stick with the object wrapper with a name key: > > /** @typedef {{name: string}} */ > var Fingerprint; > > or > > cr.exportPath('settings'); > > /** @typedef {{name: string}} */ > settings.Fingerprint; Changed to array of string. https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:19: * @const On 2016/12/13 19:53:24, Dan Beam wrote: > nit: @const {number} or just @const Done. https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:33: * @private {Array<Object>} On 2016/12/13 19:53:24, Dan Beam wrote: > !Array<!Fingerprint> Done. https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:40: notify: true On 2016/12/13 19:53:24, Dan Beam wrote: > why do you need this notify true? it seems like you're using .push() and > .splice() so that should be notifying for you... Done. https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:71: this.push('fingerprints_', { 'name': fingerprintName }); On 2016/12/13 19:53:23, Dan Beam wrote: > this.push('fingerprints_', {'name': fingerprintName}); > > (no spaces around curlies) Done. https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:71: this.push('fingerprints_', { 'name': fingerprintName }); On 2016/12/13 19:53:24, Dan Beam wrote: > wait, why are you using {name: <string>} as the item template? and not just ... > <string>? Done. https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:72: this.$.fingerprintsList.fire('iron-resize'); On 2016/12/13 19:53:24, Dan Beam wrote: > nit: notifyResize() instead of fire('iron-resize') Done. https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:86: * Returns the text to be displayed for the add fingerprint button. On 2016/12/13 19:53:24, Dan Beam wrote: > @param Done. https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:90: getFingerprintButtonText_: function(length) { On 2016/12/13 19:53:24, Dan Beam wrote: > can you name this argument something other than "length"? it kind of already > exists. > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... Done. https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:99: * Checks whether another fingerprint can be added. On 2016/12/13 19:53:24, Dan Beam wrote: > @param Done.
https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:73: return base::FeatureList::IsEnabled(features::kQuickUnlockPin) && On 2016/12/13 19:53:23, Dan Beam wrote: > can we rename the Pin version to just kQuickUnlock? That seems confusing; in the context of kQuickUnlock, kQuickUnlockFingerprint, kQuickUnlockPattern (likely future addition), kQuickUnlockPin is significantly more descriptive than kQuickUnlock. If the name is too long, I would prefer kPinUnlock, kFingerprintUnlock, etc.
https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:73: return base::FeatureList::IsEnabled(features::kQuickUnlockPin) && On 2016/12/14 17:58:57, jdufault wrote: > On 2016/12/13 19:53:23, Dan Beam wrote: > > can we rename the Pin version to just kQuickUnlock? > > That seems confusing; in the context of kQuickUnlock, kQuickUnlockFingerprint, > kQuickUnlockPattern (likely future addition), kQuickUnlockPin is significantly > more descriptive than kQuickUnlock. > > If the name is too long, I would prefer kPinUnlock, kFingerprintUnlock, etc. it's not too long, it's confusing that the PIN version enables the fingerprint version of unlock
https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:73: return base::FeatureList::IsEnabled(features::kQuickUnlockPin) && On 2016/12/14 22:25:17, Dan Beam wrote: > On 2016/12/14 17:58:57, jdufault wrote: > > On 2016/12/13 19:53:23, Dan Beam wrote: > > > can we rename the Pin version to just kQuickUnlock? > > > > That seems confusing; in the context of kQuickUnlock, kQuickUnlockFingerprint, > > kQuickUnlockPattern (likely future addition), kQuickUnlockPin is significantly > > more descriptive than kQuickUnlock. > > > > If the name is too long, I would prefer kPinUnlock, kFingerprintUnlock, etc. > > it's not too long, it's confusing that the PIN version enables the fingerprint > version of unlock Yes, I agree. I think renaming only the flag/pref makes sense since it accounts for more than just PIN. Though I'm not sure doing so is worthwhile, as the flag is going to go away soonish; it is on by default for m57.
Patchset #18 (id:480001) has been deleted
dbeam@ - Please take a look. Thanks! https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2538303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:73: return base::FeatureList::IsEnabled(features::kQuickUnlockPin) && On 2016/12/14 23:32:56, jdufault wrote: > On 2016/12/14 22:25:17, Dan Beam wrote: > > On 2016/12/14 17:58:57, jdufault wrote: > > > On 2016/12/13 19:53:23, Dan Beam wrote: > > > > can we rename the Pin version to just kQuickUnlock? > > > > > > That seems confusing; in the context of kQuickUnlock, > kQuickUnlockFingerprint, > > > kQuickUnlockPattern (likely future addition), kQuickUnlockPin is > significantly > > > more descriptive than kQuickUnlock. > > > > > > If the name is too long, I would prefer kPinUnlock, kFingerprintUnlock, etc. > > > > it's not too long, it's confusing that the PIN version enables the fingerprint > > version of unlock > > Yes, I agree. I think renaming only the flag/pref makes sense since it accounts > for more than just PIN. Though I'm not sure doing so is worthwhile, as the flag > is going to go away soonish; it is on by default for m57. I removed the dependence on kQuickUnlockPin.
https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... File chrome/browser/resources/options_resources.grd (right): https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/options_resources.grd:73: flattenhtml="true" why do you need flattenhtml="true"? https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.html (right): https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:18: <div> what is this <div> doing for you? https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:20: <iron-list id="fingerprintsList" items="[[fingerprints_]]"> indent off https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:31: </iron-list> do you want to place your iron-list in a <div class="settings-box">? https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:38: onAddFingerprint_: function() { onAddFingerprint_: function() { var numPrints = this.fingerprints_.length; if (!this.canAddNewFingerPrint_(numPrints)) return; var printName = this.i18n('lockScreenFingerprintNewName', numPrints); this.push('fingerprints_', printName); https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:50: this.$.fingerprintsList.notifyResize(); why do you need notifyResize()? https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:60: this.$.fingerprintsList.notifyResize(); why do you need notifyResize()? https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:84: return numFingerprints < MAX_NUMBER_FINGERPRINTS_ALLOWED; you only ever pass this.fingerprints_.length for numFingerprints, afaict. could you just do this instead? this.fingerprints_.length < MAX_NUMBER_FINGERPRINTS_ALLOWED; https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:60: <div class="settings-box single-column screen-lock"> what does the .screen-lock class does? https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/settings_resources.grd:1195: allowexternalscript="true" /> i think you can remove flattenhtml="true" and allowexternalscript="true"
https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... File chrome/browser/resources/options_resources.grd (right): https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/options_resources.grd:73: flattenhtml="true" On 2017/01/10 23:23:45, Dan Beam wrote: > why do you need flattenhtml="true"? What do flattenhtml and allowexternalscript do? Thanks! https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.html (right): https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:18: <div> On 2017/01/10 23:23:45, Dan Beam wrote: > what is this <div> doing for you? Done. https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:20: <iron-list id="fingerprintsList" items="[[fingerprints_]]"> On 2017/01/10 23:23:45, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:31: </iron-list> On 2017/01/10 23:23:45, Dan Beam wrote: > do you want to place your iron-list in a <div class="settings-box">? The mocks are not yet finalized yet so I'm not certain yet. https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:38: onAddFingerprint_: function() { On 2017/01/10 23:23:45, Dan Beam wrote: > onAddFingerprint_: function() { > var numPrints = this.fingerprints_.length; > if (!this.canAddNewFingerPrint_(numPrints)) > return; > var printName = this.i18n('lockScreenFingerprintNewName', numPrints); > this.push('fingerprints_', printName); This would give something slightly different than the old code. For example create a new fingerprint 'Finger 0', rename it to 'Finger 1' then create a new fingerprint. The new fingerprint would be 'Finger 1' where the old one would be 'Finger 0'. https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:50: this.$.fingerprintsList.notifyResize(); On 2017/01/10 23:23:46, Dan Beam wrote: > why do you need notifyResize()? Done. https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:60: this.$.fingerprintsList.notifyResize(); On 2017/01/10 23:23:45, Dan Beam wrote: > why do you need notifyResize()? Done. https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:84: return numFingerprints < MAX_NUMBER_FINGERPRINTS_ALLOWED; On 2017/01/10 23:23:45, Dan Beam wrote: > you only ever pass this.fingerprints_.length for numFingerprints, afaict. could > you just do this instead? > > this.fingerprints_.length < MAX_NUMBER_FINGERPRINTS_ALLOWED; That is correct. If I do it that way, canAddNewFingerprint_/getFingerprintButtonText_ don't get called when a print is added/deleted. https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/lock_screen.html (right): https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/lock_screen.html:60: <div class="settings-box single-column screen-lock"> On 2017/01/10 23:23:46, Dan Beam wrote: > what does the .screen-lock class does? This is for c/b/r/o/c/quick_unlock_configure_overlay.js to continue hiding this checkbox and not hide the newly added fingerprint checkbox. https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/settings_resources.grd:1195: allowexternalscript="true" /> On 2017/01/10 23:23:46, Dan Beam wrote: > i think you can remove flattenhtml="true" and allowexternalscript="true" Done.
dbeam@ - Please take a look. Thanks! https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:50: this.$.fingerprintsList.notifyResize(); On 2017/01/11 00:29:09, sammiequon wrote: > On 2017/01/10 23:23:46, Dan Beam wrote: > > why do you need notifyResize()? > > Done. Actually, without this, on options the list does not update. On settings it's fine.
On 2017/01/17 19:59:09, sammiequon wrote: > dbeam@ - Please take a look. Thanks! > > https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... > File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): > > https://codereview.chromium.org/2538303002/diff/470023/chrome/browser/resourc... > chrome/browser/resources/settings/people_page/fingerprint_list.js:50: > this.$.fingerprintsList.notifyResize(); > On 2017/01/11 00:29:09, sammiequon wrote: > > On 2017/01/10 23:23:46, Dan Beam wrote: > > > why do you need notifyResize()? > > > > Done. > > Actually, without this, on options the list does not update. On settings it's > fine. dbeam@ - Ping/Please take a look. Thanks!
why can't we do these again? https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:49: this.push('fingerprints_', fingerprintName); this.push('fingerprints_', this.i18n('lockScreenFingerprintNewName', this.fingerprints_.length + 1)); https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:81: canAddNewFingerprint_: function(numFingerprints) { canAddNewFingerprint_: function() { return this.fingerprints_.length < MAX_NUMBER_FINGERPRINTS_ALLOWED; },
https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:49: this.push('fingerprints_', fingerprintName); On 2017/01/26 00:27:27, Dan Beam wrote: > this.push('fingerprints_', > this.i18n('lockScreenFingerprintNewName', this.fingerprints_.length + 1)); This is a bit different behavior. For example if we have two names 'Finger 1' and 'Finger 2' and we rename 'Finger 2' to 'Finger', and then add a third fingerprint, the previous would give a new name of 'Finger 2' while this way would give a new name of 'Finger 3'. https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:81: canAddNewFingerprint_: function(numFingerprints) { On 2017/01/26 00:27:27, Dan Beam wrote: > canAddNewFingerprint_: function() { > return this.fingerprints_.length < MAX_NUMBER_FINGERPRINTS_ALLOWED; > }, I tried that but this function does not fire when I add/delete a fingerprint, with or without notifyResize. https://www.polymer-project.org/1.0/docs/devguide/data-binding#annotated-comp... - 'it is only computed once if it has no dependencies', but we need it to recompute everytime we add/delete.
https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:49: this.push('fingerprints_', fingerprintName); On 2017/01/26 00:48:23, sammiequon wrote: > On 2017/01/26 00:27:27, Dan Beam wrote: > > this.push('fingerprints_', > > this.i18n('lockScreenFingerprintNewName', this.fingerprints_.length + 1)); > > This is a bit different behavior. For example if we have two names 'Finger 1' > and 'Finger 2' and we rename 'Finger 2' to 'Finger', and then add a third > fingerprint, the previous would give a new name of 'Finger 2' while this way > would give a new name of 'Finger 3'. how can you rename fingerprints? https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:81: canAddNewFingerprint_: function(numFingerprints) { On 2017/01/26 00:48:23, sammiequon wrote: > On 2017/01/26 00:27:27, Dan Beam wrote: > > canAddNewFingerprint_: function() { > > return this.fingerprints_.length < MAX_NUMBER_FINGERPRINTS_ALLOWED; > > }, > > I tried that but this function does not fire when I add/delete a fingerprint, > with or without notifyResize. > https://www.polymer-project.org/1.0/docs/devguide/data-binding#annotated-comp... > - 'it is only computed once if it has no dependencies', but we need it to > recompute everytime we add/delete. fingerprints_.*?
https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:49: this.push('fingerprints_', fingerprintName); On 2017/01/26 04:12:04, Dan Beam wrote: > On 2017/01/26 00:48:23, sammiequon wrote: > > On 2017/01/26 00:27:27, Dan Beam wrote: > > > this.push('fingerprints_', > > > this.i18n('lockScreenFingerprintNewName', this.fingerprints_.length + > 1)); > > > > This is a bit different behavior. For example if we have two names 'Finger 1' > > and 'Finger 2' and we rename 'Finger 2' to 'Finger', and then add a third > > fingerprint, the previous would give a new name of 'Finger 2' while this way > > would give a new name of 'Finger 3'. > > how can you rename fingerprints? The fingerprint names on displayed on paper-inputs. https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:81: canAddNewFingerprint_: function(numFingerprints) { On 2017/01/26 04:12:04, Dan Beam wrote: > On 2017/01/26 00:48:23, sammiequon wrote: > > On 2017/01/26 00:27:27, Dan Beam wrote: > > > canAddNewFingerprint_: function() { > > > return this.fingerprints_.length < MAX_NUMBER_FINGERPRINTS_ALLOWED; > > > }, > > > > I tried that but this function does not fire when I add/delete a fingerprint, > > with or without notifyResize. > > > https://www.polymer-project.org/1.0/docs/devguide/data-binding#annotated-comp... > > - 'it is only computed once if it has no dependencies', but we need it to > > recompute everytime we add/delete. > > fingerprints_.*? Done.
Patchset #23 (id:590001) 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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 sammiequon@chromium.org
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...
https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:81: canAddNewFingerprint_: function(numFingerprints) { On 2017/01/26 15:56:45, sammiequon wrote: > On 2017/01/26 04:12:04, Dan Beam wrote: > > On 2017/01/26 00:48:23, sammiequon wrote: > > > On 2017/01/26 00:27:27, Dan Beam wrote: > > > > canAddNewFingerprint_: function() { > > > > return this.fingerprints_.length < MAX_NUMBER_FINGERPRINTS_ALLOWED; > > > > }, > > > > > > I tried that but this function does not fire when I add/delete a > fingerprint, > > > with or without notifyResize. > > > > > > https://www.polymer-project.org/1.0/docs/devguide/data-binding#annotated-comp... > > > - 'it is only computed once if it has no dependencies', but we need it to > > > recompute everytime we add/delete. > > > > fingerprints_.*? > > Done. no, i mean, in the binding, [[canAddNewFingerprint_(fingerprints_.*)]]
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/530001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:81: canAddNewFingerprint_: function(numFingerprints) { On 2017/01/27 01:14:15, Dan Beam wrote: > On 2017/01/26 15:56:45, sammiequon wrote: > > On 2017/01/26 04:12:04, Dan Beam wrote: > > > On 2017/01/26 00:48:23, sammiequon wrote: > > > > On 2017/01/26 00:27:27, Dan Beam wrote: > > > > > canAddNewFingerprint_: function() { > > > > > return this.fingerprints_.length < MAX_NUMBER_FINGERPRINTS_ALLOWED; > > > > > }, > > > > > > > > I tried that but this function does not fire when I add/delete a > > fingerprint, > > > > with or without notifyResize. > > > > > > > > > > https://www.polymer-project.org/1.0/docs/devguide/data-binding#annotated-comp... > > > > - 'it is only computed once if it has no dependencies', but we need it to > > > > recompute everytime we add/delete. > > > > > > fingerprints_.*? > > > > Done. > > no, i mean, in the binding, [[canAddNewFingerprint_(fingerprints_.*)]] Done.
very close to lg https://codereview.chromium.org/2538303002/diff/650001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.html (right): https://codereview.chromium.org/2538303002/diff/650001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:38: $i18n{lockScreenFingerprintWarning} in this case, because you're only adding italic style, you could probably replace <div class="warning-text"> and the .warning-text { font-style: italic; } with just <i>$i18n{lockScreenFingerprintWarning}</i> https://codereview.chromium.org/2538303002/diff/650001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/650001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:43: for (var i = 1; i <= MAX_NUMBER_FINGERPRINTS_ALLOWED; ++i) { i still think this loop is confusing. if i reach i>=MAX_NUMBER_FINGERPRINTS_ALLOWED, do I still want to push fingerprintName into this.fingerprints_? can you restructure the code so it's clearer this isn't a potential programming error? maybe: for (var i = 1; i <= MAX_NUMBER_FINGERPRINTS_ALLOWED; ++i) { var fingerprintName = this.i18n('lockScreenFingerprintNewName', i); if (!this.fingerprints_.includes(fingerprintName)) { this.push('fingerprints_', fingerprintName); break; } }
Patchset #26 (id:670001) has been deleted
https://codereview.chromium.org/2538303002/diff/650001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.html (right): https://codereview.chromium.org/2538303002/diff/650001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.html:38: $i18n{lockScreenFingerprintWarning} On 2017/01/27 23:11:07, Dan Beam wrote: > in this case, because you're only adding italic style, you could probably > replace <div class="warning-text"> and the .warning-text { font-style: italic; } > with just > > <i>$i18n{lockScreenFingerprintWarning}</i> Done. https://codereview.chromium.org/2538303002/diff/650001/chrome/browser/resourc... File chrome/browser/resources/settings/people_page/fingerprint_list.js (right): https://codereview.chromium.org/2538303002/diff/650001/chrome/browser/resourc... chrome/browser/resources/settings/people_page/fingerprint_list.js:43: for (var i = 1; i <= MAX_NUMBER_FINGERPRINTS_ALLOWED; ++i) { On 2017/01/27 23:11:07, Dan Beam wrote: > i still think this loop is confusing. > > if i reach i>=MAX_NUMBER_FINGERPRINTS_ALLOWED, do I still want to push > fingerprintName into this.fingerprints_? can you restructure the code so it's > clearer this isn't a potential programming error? > > maybe: > > for (var i = 1; i <= MAX_NUMBER_FINGERPRINTS_ALLOWED; ++i) { > var fingerprintName = this.i18n('lockScreenFingerprintNewName', i); > if (!this.fingerprints_.includes(fingerprintName)) { > this.push('fingerprints_', fingerprintName); > break; > } > } i should never > MAX_NUMBER_FINGERPRINTS_ALLOWED since this event cannot be fired when fingerprints_ has MAX_NUMBER_FINGERPRINTS_ALLOWED elements. So we should push nothing if reaches that case. Done.
sweet, thanks for hanging in there lgtm
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: 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_...)
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 jdufault@chromium.org, stevenjb@chromium.org, isherman@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2538303002/#ps710001 (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": 710001, "attempt_start_ts": 1485906545293080,
"parent_rev": "3f08de832e9ef7535f2129f35a9b069e3da92e57", "commit_rev":
"8af85f21e7a0ff7be805f3ed4e56b96fe3b272d7"}
Message was sent while issue was closed.
Description was changed from ========== md-settings: Added settings for fingerprint unlock. The mocks are not finished yet, but this CL has some of the basic logics implemented. Everything is hidden behind the feature flag QuickUnlockFingerprint. current mocks - https://docs.google.com/a/google.com/presentation/d/1XEdVznkCAunQKF_AK5bTPgeL... : Slide 13 - 22 BUG=672269 TEST=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== md-settings: Added settings for fingerprint unlock. The mocks are not finished yet, but this CL has some of the basic logics implemented. Everything is hidden behind the feature flag QuickUnlockFingerprint. current mocks - https://docs.google.com/a/google.com/presentation/d/1XEdVznkCAunQKF_AK5bTPgeL... : Slide 13 - 22 BUG=672269 TEST=NONE CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2538303002 Cr-Commit-Position: refs/heads/master@{#447370} Committed: https://chromium.googlesource.com/chromium/src/+/8af85f21e7a0ff7be805f3ed4e56... ==========
Message was sent while issue was closed.
Committed patchset #27 (id:710001) as https://chromium.googlesource.com/chromium/src/+/8af85f21e7a0ff7be805f3ed4e56... |
