Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1990233002:
[Android backup] Restore nothing if signed in user isn't valid (Closed)
|
Created:
4 years, 7 months ago by aberent Modified:
4 years, 6 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[Android backup] Restore nothing if signed in user isn't valid
If the user was previously not signed in, or if the account doesn't
exist on the new device, then restore nothing.
BUG=613147
Committed: https://crrev.com/074761b1a1d52efdccd1188841a821fb91249d9e
Cr-Commit-Position: refs/heads/master@{#397135}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Allow downstream override of account fetching #
Total comments: 8
Patch Set 3 : Respond to comments, fix integration test #
Total comments: 6
Patch Set 4 : Respond to more comments #
Messages
Total messages: 30 (11 generated)
aberent@chromium.org changed reviewers: + bauerb@chromium.org
The CQ bit was checked by aberent@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/1990233002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1990233002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1990233002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/1990233002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:66: AccountManager manager = (AccountManager) getSystemService(ACCOUNT_SERVICE); Isn't this going to fail if we don't have the CONTACTS permission? I think you want to use AccountManagerHelper (which is going to go through GMSCore to avoid requiring the permission).
https://codereview.chromium.org/1990233002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/1990233002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:66: AccountManager manager = (AccountManager) getSystemService(ACCOUNT_SERVICE); On 2016/05/19 13:58:38, Bernhard Bauer wrote: > Isn't this going to fail if we don't have the CONTACTS permission? I think you > want to use AccountManagerHelper (which is going to go through GMSCore to avoid > requiring the permission). Done.
The CQ bit was checked by aberent@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/1990233002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1990233002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1990233002/diff/20001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1990233002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:107: {% block extra_application_attributes %} Using extra_application_attributes for this feels a bit brittle to me -- it requires that the template extending this does *not* call super(), otherwise things will break. We could just use a new, dedicated block for this. https://codereview.chromium.org/1990233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/1990233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:95: Log.d(TAG, "onRestoreFinished complete, nothing restored; commit result = " Use a format string for logging. (Or just remove these log statements completely.) https://codereview.chromium.org/1990233002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java (right): https://codereview.chromium.org/1990233002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:69: public void testOnRestoreFinished_noUser() { Use camelCase throughout the test name.
https://codereview.chromium.org/1990233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/1990233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:72: private boolean accountExistsOnDevice(String userName) { I would also mention that we're not using AccountManagerHelper here because we don't have a custom application class to set it up.
https://codereview.chromium.org/1990233002/diff/20001/chrome/android/java/And... File chrome/android/java/AndroidManifest.xml (right): https://codereview.chromium.org/1990233002/diff/20001/chrome/android/java/And... chrome/android/java/AndroidManifest.xml:107: {% block extra_application_attributes %} On 2016/05/23 12:40:15, Bernhard Bauer wrote: > Using extra_application_attributes for this feels a bit brittle to me -- it > requires that the template extending this does *not* call super(), otherwise > things will break. We could just use a new, dedicated block for this. Done. https://codereview.chromium.org/1990233002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/1990233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:72: private boolean accountExistsOnDevice(String userName) { On 2016/05/23 12:41:24, Bernhard Bauer wrote: > I would also mention that we're not using AccountManagerHelper here because we > don't have a custom application class to set it up. Done. I have also put a comment at the top of the class explaining that nothing in it can depend on ChromeApplication. https://codereview.chromium.org/1990233002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:95: Log.d(TAG, "onRestoreFinished complete, nothing restored; commit result = " On 2016/05/23 12:40:15, Bernhard Bauer wrote: > Use a format string for logging. (Or just remove these log statements > completely.) Done. https://codereview.chromium.org/1990233002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java (right): https://codereview.chromium.org/1990233002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:69: public void testOnRestoreFinished_noUser() { On 2016/05/23 12:40:15, Bernhard Bauer wrote: > Use camelCase throughout the test name. Done.
The CQ bit was checked by aberent@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/1990233002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1990233002/40001
https://codereview.chromium.org/1990233002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/1990233002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:47: // should restore all non-device specific aspects of the user's state. This will involve both This isn't up to date anymore, no? https://codereview.chromium.org/1990233002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackupIntegrationTest.java (right): https://codereview.chromium.org/1990233002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackupIntegrationTest.java:64: Account[] accounts = new Account[1]; You can initialize this inline. https://codereview.chromium.org/1990233002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackupIntegrationTest.java:125: AccountManagerHelper.overrideAccountManagerHelperForTests(targetContext, accountManager); Wait, is this actually still used?
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 aberent@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/1990233002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1990233002/60001
https://codereview.chromium.org/1990233002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/1990233002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:47: // should restore all non-device specific aspects of the user's state. This will involve both On 2016/06/01 12:44:31, Bernhard Bauer wrote: > This isn't up to date anymore, no? Re-written. This will go away in my next CL, which will restore the sync preferences. https://codereview.chromium.org/1990233002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackupIntegrationTest.java (right): https://codereview.chromium.org/1990233002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackupIntegrationTest.java:64: Account[] accounts = new Account[1]; On 2016/06/01 12:44:31, Bernhard Bauer wrote: > You can initialize this inline. Done. https://codereview.chromium.org/1990233002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackupIntegrationTest.java:125: AccountManagerHelper.overrideAccountManagerHelperForTests(targetContext, accountManager); On 2016/06/01 12:44:31, Bernhard Bauer wrote: > Wait, is this actually still used? Yes. Not by ChromeBackupAgent, but starting the Chrome activity, which I do to test the result, will use it, because it checks whether the account exists on the device. I have now moved this to setUp, since it is common to both tests (and probably all future tests).
lgtm
The CQ bit was unchecked by aberent@chromium.org
The CQ bit was checked by aberent@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1990233002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1990233002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [Android backup] Restore nothing if signed in user isn't valid If the user was previously not signed in, or if the account doesn't exist on the new device, then restore nothing. BUG=613147 ========== to ========== [Android backup] Restore nothing if signed in user isn't valid If the user was previously not signed in, or if the account doesn't exist on the new device, then restore nothing. BUG=613147 Committed: https://crrev.com/074761b1a1d52efdccd1188841a821fb91249d9e Cr-Commit-Position: refs/heads/master@{#397135} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/074761b1a1d52efdccd1188841a821fb91249d9e Cr-Commit-Position: refs/heads/master@{#397135} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
