Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2092723002:
Redirect users without sync passphrase to passwords.google.com (Closed)
DescriptionRedirect syncing users without sync passphrase to passwords.google.com
This CL redirects syncing users without a sync passphrase to https://passwords.google.com instead of natively displaying passwords. The changes created in this CL are hidden behind the ViewPasswords feature.
BUG=623488
Committed: https://crrev.com/13085d446efc3b8cd2c26338738c04a7516953a8
Cr-Commit-Position: refs/heads/master@{#406823}
Patch Set 1 #Patch Set 2 : Fix redirecting issues #
Total comments: 2
Patch Set 3 : Verify sync passphrase instead of whether sync is enabled #Patch Set 4 : Add tests for redirecting #Patch Set 5 : Fix tests #
Total comments: 5
Patch Set 6 : Modify test #Patch Set 7 : Address passphrase-related comment #Patch Set 8 : Further fix to tests. #
Total comments: 2
Patch Set 9 : Additional fix to test #Patch Set 10 : More test changes #Patch Set 11 : More test changes #Patch Set 12 : Add fake account to test #Patch Set 13 : Further test change #Patch Set 14 : Tweak test #Patch Set 15 : Add feature and enable feature for testing #Patch Set 16 : Fix patch failure #Patch Set 17 : Fix patch issue #Patch Set 18 : Try fixing patch failure #Patch Set 19 : Rebase patch #Patch Set 20 : Testing why testConsumerSignin() is failing #Patch Set 21 : Changes to try and fix failing testConsumerSignin #Patch Set 22 : Checking if trybots run succesfully when failing test is removd #Patch Set 23 : Trying explicitly disabling feature for failing test #Patch Set 24 : Continuing to try fixing failing test #Patch Set 25 : Continuing to tweak test so that it doesn't fail #Patch Set 26 : Continuing to debug testConsumerSignin() #Patch Set 27 : Move account creation inside test #Patch Set 28 : Change to debug testConsumerSignin() #Patch Set 29 : Check trybots after rebase #Patch Set 30 : Fix failing test #Patch Set 31 : Change test_runner.py to original #Patch Set 32 : Add tests for remaining user groups #Patch Set 33 : Modify test_runner.py to original #
Total comments: 9
Patch Set 34 : Address comments #Patch Set 35 : Revert change to testUserRedirect #
Total comments: 12
Patch Set 36 : Move tests to separate class #
Total comments: 8
Patch Set 37 : Adjust tests #
Total comments: 11
Patch Set 38 : Rebase patch #Patch Set 39 : Modify testRedirectUser #
Total comments: 4
Patch Set 40 : Modifying test #Patch Set 41 : Modify account mocking #Patch Set 42 : Modify tests #Patch Set 43 : Modify redirect link #Patch Set 44 : Fix feature implementation #Patch Set 45 : Modify redirect test to verify sync settings #
Total comments: 8
Patch Set 46 : Remove unnecessary sync settings code #Patch Set 47 : Make sync set-up similar across tests #
Total comments: 13
Patch Set 48 : Revert test_runner.py #Patch Set 49 : Make helper method for sync settings #Patch Set 50 : Fix presubmit errors #Messages
Total messages: 56 (24 generated)
Description was changed from ========== Redirect users without sync passphrase to passwords.google.com This CL redirects users without a sync passphrase to https://passwords.google.com instead of natively displaying passwords. BUG=619868 ========== to ========== Redirect users without sync passphrase to passwords.google.com This CL redirects users without a sync passphrase to https://passwords.google.com instead of natively displaying passwords. BUG=619868 ==========
dozsa@google.com changed reviewers: + vabr@chromium.org
On 2016/06/24 12:06:38, dozsa wrote: > mailto:dozsa@google.com changed reviewers: > + mailto:vabr@chromium.org Hi Vaclav, Could you please take a look through this CL? Thank you! Paula
Thanks for the CL, Paula! I have some general comments and one in-code question below. (1) Please clarify the target group of users in the CL description: it is important that they both do use sync and do not use the passphrase. (2) Please assign this CL to the bug number 623488 instead of the umbrella bug 619868. In general, I would advise to file a bug for each such task and make it depend on 619868, otherwise the umbrella bug will be a long mess of commits. (3) Could you do some screenshots of how this looks like for various types of users (syncing with passphrase, syncing without passphrase, perhaps non-syncing as well), attach them to http://crbug.com/623488 and link here? (4) This should have a test. There seems to be https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... already, could you try and add some testcases to verify that the p.g.c site is open for syncing-without-passphrase users and the old settings for everybody else? Thanks! Vaclav https://codereview.chromium.org/2092723002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2092723002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:115: if (!AndroidSyncSettings.isSyncEnabled(getActivity().getApplicationContext())) { Does this check really separate users who use sync but without the custom passphrase from the rest? It sounds like it separates non-syncing users instead.
Description was changed from ========== Redirect users without sync passphrase to passwords.google.com This CL redirects users without a sync passphrase to https://passwords.google.com instead of natively displaying passwords. BUG=619868 ========== to ========== Redirect syncing users without sync passphrase to passwords.google.com This CL redirects syncing users without a sync passphrase to https://passwords.google.com instead of natively displaying passwords. BUG=623488 ==========
https://codereview.chromium.org/2092723002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2092723002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:115: if (!AndroidSyncSettings.isSyncEnabled(getActivity().getApplicationContext())) { On 2016/06/27 08:56:23, vabr (Chromium) wrote: > Does this check really separate users who use sync but without the custom > passphrase from the rest? It sounds like it separates non-syncing users instead. Sorry, you were correct, I was verifying whether users were syncing or not. I've changed the code to separate the syncing users without the custom passphrase.
Thanks, Paula! It seems going in the right direction, but I still have two comments. Thanks, Vaclav https://codereview.chromium.org/2092723002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2092723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:133: mPasswordsPref.setTitle(getResources().getString( Just curious -- why is this necessary now and not before the CL? https://codereview.chromium.org/2092723002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java (right): https://codereview.chromium.org/2092723002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:286: * Verifies that syncing users without a custom passphrase are redirected to The comment says "without" and the code "with" (line 294) custom passphrase.
https://codereview.chromium.org/2092723002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2092723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:133: mPasswordsPref.setTitle(getResources().getString( On 2016/06/27 15:23:44, vabr (Chromium) wrote: > Just curious -- why is this necessary now and not before the CL? The title used to be hardcoded in the xml file, and if I were to leave it there it would be displayed even for syncing users without a custom passphrase. I removed it from there and set it here as a workaround, should I be doing something else? https://codereview.chromium.org/2092723002/diff/80001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java (right): https://codereview.chromium.org/2092723002/diff/80001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:286: * Verifies that syncing users without a custom passphrase are redirected to On 2016/06/27 15:23:44, vabr (Chromium) wrote: > The comment says "without" and the code "with" (line 294) custom passphrase. Sorry, have fixed this in the new version of the test.
Hi Paula, Sorry for spotting this so late: The newly added functionality needs to be active only if the base::Feature you added in https://codereview.chromium.org/2069553002 is enabled. I left some more comments in the test. Thanks! Vaclav https://codereview.chromium.org/2092723002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2092723002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:133: mPasswordsPref.setTitle(getResources().getString( On 2016/06/28 09:53:23, dozsa wrote: > On 2016/06/27 15:23:44, vabr (Chromium) wrote: > > Just curious -- why is this necessary now and not before the CL? > > The title used to be hardcoded in the xml file, and if I were to leave it there > it would be displayed even for syncing users without a custom passphrase. I > removed it from there and set it here as a workaround, should I be doing > something else? Acknowledged, this makes sense, thanks for the explanation. https://codereview.chromium.org/2092723002/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java (right): https://codereview.chromium.org/2092723002/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:71: * passwords.google.com. We should also test the other scenario(s). https://codereview.chromium.org/2092723002/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:80: if (!syncService.isUsingSecondaryPassphrase()) { The line now seems to be a shorthand for: if (true).
Hi Vaclav, I've finally figured out why the test was failing, it's now passing, and I've also added tests for the rest of the user groups (syncing with passphrase and non-syncing). Could you please take a look over my latest changes? Thank you! Paula
Hi Paula, Thanks a lot for your work on this! Please update the CL description to mention that this is hidden behind the ViewPasswords feature. I also have some further comments and questions in the code. Apart from that, your patch LGTM, but please make sure to get it reviewed by some code owner experienced in Java and this part of the code. (Let me know if you want help in selecting the reviewer.) Thanks! Vaclav https://codereview.chromium.org/2092723002/diff/640001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java (right): https://codereview.chromium.org/2092723002/diff/640001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:107: Account mAccount = AccountManagerHelper.createAccountFromName("account@example.com"); nit: Looking at http://source.android.com/source/code-style.html, local variables should not have the "m" prefix. (Also below.) https://codereview.chromium.org/2092723002/diff/640001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:116: overrideProfileSyncService(usingPassphrase); nit: Could you just pass "false" instead of having a special variable for it? https://codereview.chromium.org/2092723002/diff/640001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:121: assertTrue(mainPreferences.onPreferenceClick(mPasswordsPref)); This line asserts that the click was handled. But how do we check that the handling consisted of redirecting to p.g.c? https://codereview.chromium.org/2092723002/diff/640001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:140: overrideProfileSyncService(usingPassphrase); nit: Also here, could you just pass "true"?
Description was changed from ========== Redirect syncing users without sync passphrase to passwords.google.com This CL redirects syncing users without a sync passphrase to https://passwords.google.com instead of natively displaying passwords. BUG=623488 ========== to ========== Redirect syncing users without sync passphrase to passwords.google.com This CL redirects syncing users without a sync passphrase to https://passwords.google.com instead of natively displaying passwords. The changes created in this CL are hidden behind the ViewPasswords feature. BUG=623488 ==========
dozsa@google.com changed reviewers: + bauerb@chromium.org
Hello Bernhard, Could you please take a look over this CL? Thank you, Paula https://codereview.chromium.org/2092723002/diff/640001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java (right): https://codereview.chromium.org/2092723002/diff/640001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:107: Account mAccount = AccountManagerHelper.createAccountFromName("account@example.com"); On 2016/07/01 15:04:44, vabr (Chromium) wrote: > nit: Looking at http://source.android.com/source/code-style.html, local > variables should not have the "m" prefix. (Also below.) Done. https://codereview.chromium.org/2092723002/diff/640001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:116: overrideProfileSyncService(usingPassphrase); On 2016/07/01 15:04:44, vabr (Chromium) wrote: > nit: Could you just pass "false" instead of having a special variable for it? Done. https://codereview.chromium.org/2092723002/diff/640001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:121: assertTrue(mainPreferences.onPreferenceClick(mPasswordsPref)); On 2016/07/01 15:04:44, vabr (Chromium) wrote: > This line asserts that the click was handled. But how do we check that the > handling consisted of redirecting to p.g.c? Acknowledged. https://codereview.chromium.org/2092723002/diff/640001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:140: overrideProfileSyncService(usingPassphrase); On 2016/07/01 15:04:44, vabr (Chromium) wrote: > nit: Also here, could you just pass "true"? Done.
Hi Paula, build/android/test_runner.py seems to have crept in again. :) Also, I have one question below. Thanks! Vaclav https://codereview.chromium.org/2092723002/diff/640001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java (right): https://codereview.chromium.org/2092723002/diff/640001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:121: assertTrue(mainPreferences.onPreferenceClick(mPasswordsPref)); On 2016/07/01 15:41:20, dozsa wrote: > On 2016/07/01 15:04:44, vabr (Chromium) wrote: > > This line asserts that the click was handled. But how do we check that the > > handling consisted of redirecting to p.g.c? > > Acknowledged. I was hoping for a more concrete answer :). Maybe I'm just missing something, but do we check that the handling is actually navigating to the p.g.c page? If not, could we?
https://codereview.chromium.org/2092723002/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2092723002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:52: private static final int ORDER_MANAGE_ACCOUNT_LINK = 2; This is unused. https://codereview.chromium.org/2092723002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:59: private Preference mPasswordsPref; This is also unused. https://codereview.chromium.org/2092723002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:146: + ".password.SavePasswordsPreferences"); Could you get this from the class? https://codereview.chromium.org/2092723002/diff/680001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java (right): https://codereview.chromium.org/2092723002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:77: private static class FakeProfileSyncServiceWithoutPassphrase extends ProfileSyncService { I think you could merge this with FakeProfileSyncServiceWithPassphrase into a single class that takes the flag in its constructor. https://codereview.chromium.org/2092723002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:114: MainPreferences.PREF_SAVED_PASSWORDS); Could you move these tests to a new class? Then you could put a lot of this into the SetUp() method. https://codereview.chromium.org/2092723002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:143: assertEquals(passwordsPref.getFragment(), "org.chromium.chrome.browser.preferences" Expected value goes first.
https://codereview.chromium.org/2092723002/diff/680001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2092723002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:52: private static final int ORDER_MANAGE_ACCOUNT_LINK = 2; On 2016/07/01 16:55:28, Bernhard Bauer wrote: > This is unused. Done. https://codereview.chromium.org/2092723002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:59: private Preference mPasswordsPref; On 2016/07/01 16:55:28, Bernhard Bauer wrote: > This is also unused. Done. https://codereview.chromium.org/2092723002/diff/680001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:146: + ".password.SavePasswordsPreferences"); On 2016/07/01 16:55:28, Bernhard Bauer wrote: > Could you get this from the class? Done. https://codereview.chromium.org/2092723002/diff/680001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java (right): https://codereview.chromium.org/2092723002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:77: private static class FakeProfileSyncServiceWithoutPassphrase extends ProfileSyncService { On 2016/07/01 16:55:28, Bernhard Bauer wrote: > I think you could merge this with FakeProfileSyncServiceWithPassphrase into a > single class that takes the flag in its constructor. Done. https://codereview.chromium.org/2092723002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:114: MainPreferences.PREF_SAVED_PASSWORDS); On 2016/07/01 16:55:28, Bernhard Bauer wrote: > Could you move these tests to a new class? Then you could put a lot of this into > the SetUp() method. Done. https://codereview.chromium.org/2092723002/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PreferencesTest.java:143: assertEquals(passwordsPref.getFragment(), "org.chromium.chrome.browser.preferences" On 2016/07/01 16:55:28, Bernhard Bauer wrote: > Expected value goes first. Done.
https://codereview.chromium.org/2092723002/diff/700001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java (right): https://codereview.chromium.org/2092723002/diff/700001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:52: * Launches the preferences menu and starts the preferences activity named fragmentName. Nit: |fragmentName| in pipe symbols. https://codereview.chromium.org/2092723002/diff/700001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:55: public static Preferences startPreferences(Instrumentation instrumentation, This can be private. Also, you're casting the return value to MainPreferences anyway :) https://codereview.chromium.org/2092723002/diff/700001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:70: boolean flagValue = ChromeFeatureList.isEnabled("ViewPasswords"); Inline? https://codereview.chromium.org/2092723002/diff/700001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:94: AndroidSyncSettings.overrideForTests(getInstrumentation().getTargetContext(), Move this to setup? Also, if overriding the sync service doesn't break testNativePasswordViewNonSyncing(), you could move that into setup as well.
https://codereview.chromium.org/2092723002/diff/700001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java (right): https://codereview.chromium.org/2092723002/diff/700001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:52: * Launches the preferences menu and starts the preferences activity named fragmentName. On 2016/07/05 10:55:31, Bernhard Bauer wrote: > Nit: |fragmentName| in pipe symbols. Done. https://codereview.chromium.org/2092723002/diff/700001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:55: public static Preferences startPreferences(Instrumentation instrumentation, On 2016/07/05 10:55:31, Bernhard Bauer wrote: > This can be private. Also, you're casting the return value to MainPreferences > anyway :) Done. https://codereview.chromium.org/2092723002/diff/700001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:70: boolean flagValue = ChromeFeatureList.isEnabled("ViewPasswords"); On 2016/07/05 10:55:31, Bernhard Bauer wrote: > Inline? Done. https://codereview.chromium.org/2092723002/diff/700001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:94: AndroidSyncSettings.overrideForTests(getInstrumentation().getTargetContext(), On 2016/07/05 10:55:31, Bernhard Bauer wrote: > Move this to setup? Also, if overriding the sync service doesn't break > testNativePasswordViewNonSyncing(), you could move that into setup as well. Done.
https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java (right): https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:58: private static Preferences startPreferences(Instrumentation instrumentation, What I meant was, either return the type of |activity|, i.e. Activity, or do the complete cast here and return MainPreferences. https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:92: @CommandLineFlags.Add("enable-features=" + MainPreferences.VIEW_PASSWORDS) Actually, this confuses me. Doesn't this together with the fake sync service mean we pretend the user does have a custom passphrase? https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:95: assertTrue(mMainPreferences.onPreferenceClick(mPasswordsPref)); And the condition you are verifying here is true in any case, because the implementation doesn't actually depend on the state.
https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java (right): https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:58: private static Preferences startPreferences(Instrumentation instrumentation, On 2016/07/05 12:37:02, Bernhard Bauer wrote: > What I meant was, either return the type of |activity|, i.e. Activity, or do the > complete cast here and return MainPreferences. Done. https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:92: @CommandLineFlags.Add("enable-features=" + MainPreferences.VIEW_PASSWORDS) On 2016/07/05 12:37:02, Bernhard Bauer wrote: > Actually, this confuses me. Doesn't this together with the fake sync service > mean we pretend the user does have a custom passphrase? Sorry, I have no idea why I put the feature in the FakeProfileSyncService method, that has nothing to do with the passphrase. https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:95: assertTrue(mMainPreferences.onPreferenceClick(mPasswordsPref)); On 2016/07/05 12:37:02, Bernhard Bauer wrote: > And the condition you are verifying here is true in any case, because the > implementation doesn't actually depend on the state. Do you have any suggestions on how to verify that the user is being redirected? I've tried checking if the intent is equal to the one in onPreferenceClick, but right now the test is failing (expected:<Intent { flg=0x14000000 cmp=org.chromium.chrome/.browser.preferences.Preferences (has extras) }> but was:<Intent { act=android.intent.action.VIEW dat=https://passwords.google.com }>). I'm not sure how to mock achieving the latter intent.
https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java (right): https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:58: private static Preferences startPreferences(Instrumentation instrumentation, On 2016/07/06 10:14:30, dozsa wrote: > On 2016/07/05 12:37:02, Bernhard Bauer wrote: > > What I meant was, either return the type of |activity|, i.e. Activity, or do > the > > complete cast here and return MainPreferences. > > Done. Not really? If this method is supposed to return the main preferences, make the return type MainPreferences. https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:95: assertTrue(mMainPreferences.onPreferenceClick(mPasswordsPref)); On 2016/07/06 10:14:30, dozsa wrote: > On 2016/07/05 12:37:02, Bernhard Bauer wrote: > > And the condition you are verifying here is true in any case, because the > > implementation doesn't actually depend on the state. > > Do you have any suggestions on how to verify that the user is being redirected? > I've tried checking if the intent is equal to the one in onPreferenceClick, but > right now the test is failing (expected:<Intent { flg=0x14000000 > cmp=org.chromium.chrome/.browser.preferences.Preferences (has extras) }> but > was:<Intent { act=android.intent.action.VIEW http://dat=https://passwords.google.com > }>). I'm not sure how to mock achieving the latter intent. Well, you could try mocking out the observed behavior of the Preference, which is trying to open an Intent (either for a Preference fragment, or a URL view). That would probably be easier in an actual unit test, i.e. a JUnit test that runs on the host. Robolectric lets you do some magic there to e.g. mock out startActivity(). Alternatively, you could rely on the implementation of Preference, which is that it will run the onPreferenceClick handler if there is one set, and if not, open the fragment (if that is set). That means you could examine whether or not an onPreferenceClick handler is set and whether or not a fragment is set. That's probably closer to what you currently have; it would only require looking at the onPreferenceClick handler as well. https://codereview.chromium.org/2092723002/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2092723002/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:125: ForegroundColorSpan colorSpan = new ForegroundColorSpan( This is now unused? Alternatively, extract the color into a local variable. https://codereview.chromium.org/2092723002/diff/760001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java (right): https://codereview.chromium.org/2092723002/diff/760001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:89: if (usingPassphrase) { I liked the fact that the previous version only used a single class. You could pass in |usingPassPhrase| as a parameter and return that in isUsingSecondaryPassphrase().
https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java (right): https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:58: private static Preferences startPreferences(Instrumentation instrumentation, On 2016/07/06 14:19:35, Bernhard Bauer wrote: > On 2016/07/06 10:14:30, dozsa wrote: > > On 2016/07/05 12:37:02, Bernhard Bauer wrote: > > > What I meant was, either return the type of |activity|, i.e. Activity, or do > > the > > > complete cast here and return MainPreferences. > > > > Done. > > Not really? If this method is supposed to return the main preferences, make the > return type MainPreferences. I'm not really sure how to do this, given that I can only use the MainPreferences type if I use the getFragmentForTest() method, and I can't do this within startMainPreferences. https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:95: assertTrue(mMainPreferences.onPreferenceClick(mPasswordsPref)); On 2016/07/06 14:19:35, Bernhard Bauer wrote: > On 2016/07/06 10:14:30, dozsa wrote: > > On 2016/07/05 12:37:02, Bernhard Bauer wrote: > > > And the condition you are verifying here is true in any case, because the > > > implementation doesn't actually depend on the state. > > > > Do you have any suggestions on how to verify that the user is being > redirected? > > I've tried checking if the intent is equal to the one in onPreferenceClick, > but > > right now the test is failing (expected:<Intent { flg=0x14000000 > > cmp=org.chromium.chrome/.browser.preferences.Preferences (has extras) }> but > > was:<Intent { act=android.intent.action.VIEW > http://dat=https://passwords.google.com > > }>). I'm not sure how to mock achieving the latter intent. > > Well, you could try mocking out the observed behavior of the Preference, which > is trying to open an Intent (either for a Preference fragment, or a URL view). > That would probably be easier in an actual unit test, i.e. a JUnit test that > runs on the host. Robolectric lets you do some magic there to e.g. mock out > startActivity(). > > Alternatively, you could rely on the implementation of Preference, which is that > it will run the onPreferenceClick handler if there is one set, and if not, open > the fragment (if that is set). That means you could examine whether or not an > onPreferenceClick handler is set and whether or not a fragment is set. That's > probably closer to what you currently have; it would only require looking at the > onPreferenceClick handler as well. I'm still not entirely sure why my test is failing, despite sync settings being set up correctly, and after discussing this with Vaclav he suggested the following: "Try to cut down the test to the bare minimum to show the issue: setting up the sync service, and then asserting that the sync service setting is as expected." The sync settings are now set up correctly (syncing, no passphrase), and verified in the updated version of the test. https://codereview.chromium.org/2092723002/diff/760001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java (right): https://codereview.chromium.org/2092723002/diff/760001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/MainPreferences.java:125: ForegroundColorSpan colorSpan = new ForegroundColorSpan( On 2016/07/06 14:19:35, Bernhard Bauer wrote: > This is now unused? Alternatively, extract the color into a local variable. Done. https://codereview.chromium.org/2092723002/diff/760001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java (right): https://codereview.chromium.org/2092723002/diff/760001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:89: if (usingPassphrase) { On 2016/07/06 14:19:36, Bernhard Bauer wrote: > I liked the fact that the previous version only used a single class. You could > pass in |usingPassPhrase| as a parameter and return that in > isUsingSecondaryPassphrase(). Done.
Hi Paula, Thanks for the update. I'm now not quite sure what exactly is failing and what is running as expected, could you elaborate on that? Did you mean that testUserRedirectSyncSettings is passing while testSyncingNativePasswordView is failing? I believe the idea was to have testUserRedirectSyncSettings verify that the sync set-up for testSyncingNativePasswordView is correct, but they seem to have each a different sync setup, so not sure what testUserRedirectSyncSettings is telling us about testSyncingNativePasswordView. Cheers, Vaclav https://codereview.chromium.org/2092723002/diff/880001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2092723002/diff/880001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:330: <message name="IDS_REDIRECT_TO_PASSWORDS_TEXT" desc="Text for link to manage passwords on Account Central from Settings page."> nit: "Text for link" is not very clear -- in HTML that would mean whatever is between <a> and </a>, but that's not the case here. What about: desc='Title of password settings entry in main Chrome preferences for users who will be redirected to passwords.google.com to manage their passwords.' https://codereview.chromium.org/2092723002/diff/880001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:334: passwords.google.com nit: This is not really a link, not even a URL. What about: desc='Hostname of the site where users will go if they tap the passwords settings in main Chrome preferences.' https://codereview.chromium.org/2092723002/diff/880001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java (right): https://codereview.chromium.org/2092723002/diff/880001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:42: private int mGetMasterSyncAutomaticallyCalls; The *Calls variables seem to be never read. Could you just remove the CountingMockSyncContentResolverDelegate and use MockSyncContentResolverDelegate? https://codereview.chromium.org/2092723002/diff/880001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:135: public boolean receivedNotification() { Is this getter ever called? If not, could you drop the class in case you don't use it?
Hi Vaclav, Sorry for the confusion. Right now all of the tests are passing, and I've modified the sync set-up so it's the same in all of the tests. Paula https://codereview.chromium.org/2092723002/diff/880001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2092723002/diff/880001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:330: <message name="IDS_REDIRECT_TO_PASSWORDS_TEXT" desc="Text for link to manage passwords on Account Central from Settings page."> On 2016/07/20 11:15:06, vabr (Chromium) wrote: > nit: "Text for link" is not very clear -- in HTML that would mean whatever is > between <a> and </a>, but that's not the case here. What about: > desc='Title of password settings entry in main Chrome preferences for users who > will be redirected to http://passwords.google.com to manage their passwords.' Done. https://codereview.chromium.org/2092723002/diff/880001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:334: passwords.google.com On 2016/07/20 11:15:07, vabr (Chromium) wrote: > nit: This is not really a link, not even a URL. What about: > desc='Hostname of the site where users will go if they tap the passwords > settings in main Chrome preferences.' Done. https://codereview.chromium.org/2092723002/diff/880001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java (right): https://codereview.chromium.org/2092723002/diff/880001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:42: private int mGetMasterSyncAutomaticallyCalls; On 2016/07/20 11:15:07, vabr (Chromium) wrote: > The *Calls variables seem to be never read. Could you just remove the > CountingMockSyncContentResolverDelegate and use MockSyncContentResolverDelegate? Sorry, that was something I used while playing with the sync settings in the test. Removed. https://codereview.chromium.org/2092723002/diff/880001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:135: public boolean receivedNotification() { On 2016/07/20 11:15:07, vabr (Chromium) wrote: > Is this getter ever called? If not, could you drop the class in case you don't > use it? Same as above. Removed.
The CQ bit was checked by dozsa@google.com 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...
Thank you, Paula, I'm glad to hear that the tests now work! :) Cheers, Vaclav https://codereview.chromium.org/2092723002/diff/920001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java (right): https://codereview.chromium.org/2092723002/diff/920001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:136: public void testUserRedirectSyncSettings() throws InterruptedException { Since tests now pass, and you know that the sync set-up works, maybe you don't need this particular test case any more? Or if you keep it, perhaps you could pull out the common set-up code shared with testSyncingNativePasswordView into a helper method?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java (right): https://codereview.chromium.org/2092723002/diff/720001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:58: private static Preferences startPreferences(Instrumentation instrumentation, On 2016/07/20 09:13:50, dozsa wrote: > On 2016/07/06 14:19:35, Bernhard Bauer wrote: > > On 2016/07/06 10:14:30, dozsa wrote: > > > On 2016/07/05 12:37:02, Bernhard Bauer wrote: > > > > What I meant was, either return the type of |activity|, i.e. Activity, or > do > > > the > > > > complete cast here and return MainPreferences. > > > > > > Done. > > > > Not really? If this method is supposed to return the main preferences, make > the > > return type MainPreferences. > > I'm not really sure how to do this, given that I can only use the > MainPreferences type if I use the getFragmentForTest() method, and I can't do > this within startMainPreferences. Oops, misread what the cast refers to. To make this a bit clearer, could you extract the returned Preferences in a local variable, so you'd have one cast per assignment? https://codereview.chromium.org/2092723002/diff/920001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2092723002/diff/920001/build/android/test_run... build/android/test_runner.py:61: '--debug', action='store_const', const='Default', dest='build_type', Why this change? https://codereview.chromium.org/2092723002/diff/920001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java (right): https://codereview.chromium.org/2092723002/diff/920001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:76: /* Javadoc-style comment. https://codereview.chromium.org/2092723002/diff/920001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:92: private static class FakeProfileSyncService extends ProfileSyncService { You could make this an anonymous class in overrideProfileSyncService() so you could leave out the constructor and the member -- up to you. https://codereview.chromium.org/2092723002/diff/920001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:94: private boolean mUsingPassphrase; Make this final? https://codereview.chromium.org/2092723002/diff/920001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:142: //First sync Add a space at the beginning of the comment (also in other places in this file).
I've removed the _googblue_24dp part from the icon names, and thank you for noticing the misplaced xxxhpdi icon. I'm not sure what you mean when you say I should mirror the regular icon? I was told by the UX designer that the icons can be found at https://icons.googleplex.com/#icon=ic_content_copy&search=copy and https://icons.googleplex.com/#icon=ic_visibility_off&search=eye so I just downloaded the .png files from there and included them in the specified directories.
Sorry, disregard the last message, it was meant for a different CL.
The CQ bit was checked by dozsa@google.com 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 checked by dozsa@google.com 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/2092723002/diff/920001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2092723002/diff/920001/build/android/test_run... build/android/test_runner.py:61: '--debug', action='store_const', const='Default', dest='build_type', On 2016/07/20 14:56:19, Bernhard Bauer wrote: > Why this change? I have to make that change in order to be able to run tests, however the file will be unchanged when I submit the CL. https://codereview.chromium.org/2092723002/diff/920001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java (right): https://codereview.chromium.org/2092723002/diff/920001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:76: /* On 2016/07/20 14:56:19, Bernhard Bauer wrote: > Javadoc-style comment. Done. https://codereview.chromium.org/2092723002/diff/920001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:92: private static class FakeProfileSyncService extends ProfileSyncService { On 2016/07/20 14:56:19, Bernhard Bauer wrote: > You could make this an anonymous class in overrideProfileSyncService() so you > could leave out the constructor and the member -- up to you. Thank you for the suggestion. Done. https://codereview.chromium.org/2092723002/diff/920001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:94: private boolean mUsingPassphrase; On 2016/07/20 14:56:19, Bernhard Bauer wrote: > Make this final? Removed so no longer necessary. https://codereview.chromium.org/2092723002/diff/920001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:136: public void testUserRedirectSyncSettings() throws InterruptedException { On 2016/07/20 14:16:44, vabr (Chromium) wrote: > Since tests now pass, and you know that the sync set-up works, maybe you don't > need this particular test case any more? Or if you keep it, perhaps you could > pull out the common set-up code shared with testSyncingNativePasswordView into a > helper method? I've kept the test but made a helper method for turning syncability on/off. https://codereview.chromium.org/2092723002/diff/920001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/preferences/PasswordViewingTypeTest.java:142: //First sync On 2016/07/20 14:56:19, Bernhard Bauer wrote: > Add a space at the beginning of the comment (also in other places in this file). Done.
LGTM https://codereview.chromium.org/2092723002/diff/920001/build/android/test_run... File build/android/test_runner.py (right): https://codereview.chromium.org/2092723002/diff/920001/build/android/test_run... build/android/test_runner.py:61: '--debug', action='store_const', const='Default', dest='build_type', On 2016/07/21 08:51:33, dozsa wrote: > On 2016/07/20 14:56:19, Bernhard Bauer wrote: > > Why this change? > > I have to make that change in order to be able to run tests, however the file > will be unchanged when I submit the CL. Is that for running tests locally? You could keep this change in a separate branch and merge the CL for upload in, for example. Or don't commit the change at all and use `git stash` and `git stash pop` to remove the change before uploading and reapply it afterwards.
The CQ bit was unchecked by dozsa@google.com
The CQ bit was checked by dozsa@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org Link to the patchset: https://codereview.chromium.org/2092723002/#ps960001 (title: "Make helper method for sync settings")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by dozsa@google.com 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 dozsa@google.com
The CQ bit was checked by dozsa@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2092723002/#ps980001 (title: "Fix presubmit errors")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Redirect syncing users without sync passphrase to passwords.google.com This CL redirects syncing users without a sync passphrase to https://passwords.google.com instead of natively displaying passwords. The changes created in this CL are hidden behind the ViewPasswords feature. BUG=623488 ========== to ========== Redirect syncing users without sync passphrase to passwords.google.com This CL redirects syncing users without a sync passphrase to https://passwords.google.com instead of natively displaying passwords. The changes created in this CL are hidden behind the ViewPasswords feature. BUG=623488 ==========
Message was sent while issue was closed.
Committed patchset #50 (id:980001)
Message was sent while issue was closed.
Description was changed from ========== Redirect syncing users without sync passphrase to passwords.google.com This CL redirects syncing users without a sync passphrase to https://passwords.google.com instead of natively displaying passwords. The changes created in this CL are hidden behind the ViewPasswords feature. BUG=623488 ========== to ========== Redirect syncing users without sync passphrase to passwords.google.com This CL redirects syncing users without a sync passphrase to https://passwords.google.com instead of natively displaying passwords. The changes created in this CL are hidden behind the ViewPasswords feature. BUG=623488 Committed: https://crrev.com/13085d446efc3b8cd2c26338738c04a7516953a8 Cr-Commit-Position: refs/heads/master@{#406823} ==========
Message was sent while issue was closed.
Patchset 50 (id:??) landed as https://crrev.com/13085d446efc3b8cd2c26338738c04a7516953a8 Cr-Commit-Position: refs/heads/master@{#406823}
Message was sent while issue was closed.
A revert of this CL (patchset #50 id:980001) has been created in https://codereview.chromium.org/2165333002/ by tedchoc@chromium.org. The reason for reverting is: The tests are checking their expectations on the wrong thread. https://uberchromegw.corp.google.com/i/chromium.linux/builders/Android%20Test... I 703.521s run_tests_on_device(0ca41989032f9f23) java.lang.AssertionError I 703.521s run_tests_on_device(0ca41989032f9f23) at org.chromium.base.ThreadUtils.assertOnUiThread(ThreadUtils.java:192) I 703.521s run_tests_on_device(0ca41989032f9f23) at org.chromium.chrome.browser.sync.ProfileSyncService.get(ProfileSyncService.java:104) I 703.521s run_tests_on_device(0ca41989032f9f23) at org.chromium.chrome.browser.preferences.PasswordViewingTypeTest.testUserRedirectSyncSettings(PasswordViewingTypeTest.java:133) I 703.521s run_tests_on_device(0ca41989032f9f23) at java.lang.reflect.Method.invokeNative(Native Method) I 703.521s run_tests_on_device(0ca41989032f9f23) at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) I 703.521s run_tests_on_device(0ca41989032f9f23) at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) I 703.521s run_tests_on_device(0ca41989032f9f23) at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:129) I 703.521s run_tests_on_device(0ca41989032f9f23) at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) I 703.521s run_tests_on_device(0ca41989032f9f23) at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) I 703.521s run_tests_on_device(0ca41989032f9f23) at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) I 703.521s run_tests_on_device(0ca41989032f9f23) at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701). |
