Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1877213003:
[Smart Lock, UI] Fix dissapeared Auto-signin setting. (Closed)
|
Created:
4 years, 8 months ago by melandory Modified:
4 years, 8 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Smart Lock, UI] Fix dissapeared Auto-signin setting.
BUG=601539
Committed: https://crrev.com/0f2cf06a3f34dfc24b959d2fc0d5de9bdd54c221
Cr-Commit-Position: refs/heads/master@{#388747}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 7
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Patch Set 6 : Rebased #Patch Set 7 : #Patch Set 8 : #
Total comments: 4
Patch Set 9 : #
Messages
Total messages: 33 (11 generated)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877213003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877213003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877213003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877213003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
melandory@chromium.org changed reviewers: + bauerb@chromium.org
PTAL, thanks!
https://codereview.chromium.org/1877213003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java (right): https://codereview.chromium.org/1877213003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java:24: private static final String ENABLE_CREDENTIAL_MANAGER_API = "CredentialManagementAPI"; Declaring this constant multiple times defeats the purpose of having a constant in the first place 😃 https://codereview.chromium.org/1877213003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java:105: if (!ChromeFeatureList.isEnabled(ENABLE_CREDENTIAL_MANAGER_API)) { Wouldn't it be a better idea to force-enable this feature for the test? If you want to get real fancy, you could even implement a test annotation that does that (ask dgn@ how). https://codereview.chromium.org/1877213003/diff/40001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1877213003/diff/40001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.cc:37: &features::kCredentialManagementAPI, Can we order these alphabetically before it becomes too cluttered?
https://codereview.chromium.org/1877213003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java (right): https://codereview.chromium.org/1877213003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java:105: if (!ChromeFeatureList.isEnabled(ENABLE_CREDENTIAL_MANAGER_API)) { On 2016/04/13 14:23:01, Bernhard Bauer wrote: > Wouldn't it be a better idea to force-enable this feature for the test? If you > want to get real fancy, you could even implement a test annotation that does > that (ask dgn@ how). So, it turns out there is already @CommandLineFlags.Add("enable-features=<Feature>"), which is probably close enough not to warrant a separate annotation.
https://codereview.chromium.org/1877213003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java (right): https://codereview.chromium.org/1877213003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java:24: private static final String ENABLE_CREDENTIAL_MANAGER_API = "CredentialManagementAPI"; On 2016/04/13 14:23:01, Bernhard Bauer wrote: > Declaring this constant multiple times defeats the purpose of having a constant > in the first place 😃 I haven't fount the right place where to put it. Does ChromeSwitches sounds too bad? https://codereview.chromium.org/1877213003/diff/40001/chrome/browser/android/... File chrome/browser/android/chrome_feature_list.cc (right): https://codereview.chromium.org/1877213003/diff/40001/chrome/browser/android/... chrome/browser/android/chrome_feature_list.cc:37: &features::kCredentialManagementAPI, On 2016/04/13 14:23:01, Bernhard Bauer wrote: > Can we order these alphabetically before it becomes too cluttered? Done.
Forgot to upload a new patch set? https://codereview.chromium.org/1877213003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java (right): https://codereview.chromium.org/1877213003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java:24: private static final String ENABLE_CREDENTIAL_MANAGER_API = "CredentialManagementAPI"; On 2016/04/14 11:23:01, melandory wrote: > On 2016/04/13 14:23:01, Bernhard Bauer wrote: > > Declaring this constant multiple times defeats the purpose of having a > constant > > in the first place 😃 > > I haven't fount the right place where to put it. Does ChromeSwitches sounds too > bad? It's not a switch. Can you just use the one from SavePasswordsPreferences?
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877213003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877213003/60001
On 2016/04/14 11:27:38, Bernhard Bauer wrote: > Forgot to upload a new patch set? I forgot indeed, sorry. > > https://codereview.chromium.org/1877213003/diff/40001/chrome/android/javatest... > File > chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java > (right): > > https://codereview.chromium.org/1877213003/diff/40001/chrome/android/javatest... > chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java:24: > private static final String ENABLE_CREDENTIAL_MANAGER_API = > "CredentialManagementAPI"; > On 2016/04/14 11:23:01, melandory wrote: > > On 2016/04/13 14:23:01, Bernhard Bauer wrote: > > > Declaring this constant multiple times defeats the purpose of having a > > constant > > > in the first place 😃 > > > > I haven't fount the right place where to put it. Does ChromeSwitches sounds > too > > bad? > > It's not a switch. > > Can you just use the one from SavePasswordsPreferences?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
https://codereview.chromium.org/1877213003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java (right): https://codereview.chromium.org/1877213003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:54: public static final String ENABLE_CREDENTIAL_MANAGER_API = "CredentialManagementAPI"; You might have to add @VisibleForTesting to this to prevent Proguard from mucking around with it. https://codereview.chromium.org/1877213003/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java (right): https://codereview.chromium.org/1877213003/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java:105: if (!ChromeFeatureList.isEnabled( This shouldn't be necessary now. In fact, if for some reason the feature does not get enabled, you would silently skip the test, so you might want to actually assert that it's enabled.
The CQ bit was checked by melandory@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877213003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877213003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
PTAL.
https://codereview.chromium.org/1877213003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java (right): https://codereview.chromium.org/1877213003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:56: public static final String ENABLE_CREDENTIAL_MANAGER_API = "CredentialManagementAPI"; Nit: This is the name of the feature, so the ENABLE_ prefix is unnecessary / potentially confusing. https://codereview.chromium.org/1877213003/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java (right): https://codereview.chromium.org/1877213003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java:90: "enable-features=" + SavePasswordsPreferences.ENABLE_CREDENTIAL_MANAGER_API) This seems like it's indented a bit too far -- it should be eight spaces, right?
https://codereview.chromium.org/1877213003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java (right): https://codereview.chromium.org/1877213003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferences.java:56: public static final String ENABLE_CREDENTIAL_MANAGER_API = "CredentialManagementAPI"; On 2016/04/20 10:10:11, Bernhard Bauer wrote: > Nit: This is the name of the feature, so the ENABLE_ prefix is unnecessary / > potentially confusing. Done. https://codereview.chromium.org/1877213003/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java (right): https://codereview.chromium.org/1877213003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java:90: "enable-features=" + SavePasswordsPreferences.ENABLE_CREDENTIAL_MANAGER_API) On 2016/04/20 10:10:11, Bernhard Bauer wrote: > This seems like it's indented a bit too far -- it should be eight spaces, right? Presubmit disagrees: ** Presubmit ERRORS ** chrome/android/javatests/src/org/chromium/chrome/browser/preferences/password/SavePasswordsPreferencesTest.java:90: '"enable-features="' have incorrect indentation level 8, expected level should be 12. But with renaming of variable it's not an issue anymore
lgtm
The CQ bit was checked by melandory@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1877213003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1877213003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Smart Lock, UI] Fix dissapeared Auto-signin setting. BUG=601539 ========== to ========== [Smart Lock, UI] Fix dissapeared Auto-signin setting. BUG=601539 Committed: https://crrev.com/0f2cf06a3f34dfc24b959d2fc0d5de9bdd54c221 Cr-Commit-Position: refs/heads/master@{#388747} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/0f2cf06a3f34dfc24b959d2fc0d5de9bdd54c221 Cr-Commit-Position: refs/heads/master@{#388747} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
