Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2627093009:
Fetch Finch seed during restore (Closed)
DescriptionFetch Finch seed during restore
Chrome will fetch a Finch seed during a normal first run, but since
may take some time it is only done if the FRE is displayed (hence
hiding the delay from the user. The Finch data is not backed up since
it would probably be out of date when restored. As a result there was
no Finch seed set on the first run following a restore.
Fix this by fetching the finch seed during restore.
This also moves fetching the Finch seed from an Android Service to a background thread (actually an AsyncTask) and gets rid of the no longer used ToS Service.
BUG=679386
BUG=680829
Review-Url: https://codereview.chromium.org/2627093009
Cr-Commit-Position: refs/heads/master@{#445436}
Committed: https://chromium.googlesource.com/chromium/src/+/6b91ed876fd20ba96e4c1c008d056a0ed98788b6
Patch Set 1 #
Total comments: 31
Patch Set 2 : Rebase to fix merge problems #Patch Set 3 : Respond to review comments - tests still needed #
Total comments: 12
Patch Set 4 : Now with tests! #
Total comments: 12
Patch Set 5 : Remove VariationsSeedService, change everything to AsyncTasks #Patch Set 6 : Fix findbugs problem #
Total comments: 36
Patch Set 7 : Fix nits and simplify VariationsSeedFetcher tests #
Total comments: 5
Patch Set 8 : Fix nits and tests #Messages
Total messages: 52 (29 generated)
aberent@chromium.org changed reviewers: + asvitkine@chromium.org, bauerb@chromium.org
Initial review. I think the code is correct and complete, but I will investigate adding to the tests before I land this.
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/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...)
Looks good! https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:60: // network access, but must be less than the restore timeout to be useful. What is the restore timeout? https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:61: private static final long BACKGROUND_TASK_TIMEOUT = 20; Add a unit? https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:278: // Ignore timeouts. Problems with the variation seed can be ignored, and other Nit: If this should time out, await() is not going to throw an InterruptedException. That happens if something signals the current thread (via Thread.interrupt(); see https://docs.oracle.com/javase/tutorial/essential/concurrency/interrupt.html), which to my knowledge we never do in Chrome. If it does timeout, the method is going to return false, which we already ignore, but the comment is a bit misleading if it's in the catch clause. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:290: })) { Nit: this looks really awkward. Can we maybe extract the return value if a local variable to break up the two blocks?
Thanks for working on this! https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:273: }.startBackgroundTasks(false, true); Can you add /* */ comments documenting the params or add vars for them above so it's clear what you're passing? https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java (right): https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:33: Nit: Remove empty line. You can add it after TAG if you'd like. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:39: private static boolean shouldFetchVariationsSeedDuringFirstRun() { This logic changed on TOT in the meantime - this function was deleted and replace with a call to ChromeVersionInfo.isOfficialBuild() directly. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:48: * Start the background tasks. Nit: Javadoc for params. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:51: final boolean initVariationSeed) { Nit: No need for final on the second var. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:56: boolean variationsInitialized = prefs.getBoolean(VARIATIONS_INITIALIZED, false); I don't understand how this will work. If this Java pref will be restored by B&R, then it will have a value of true while the native side would have been lost. For this to work, we would need to make sure this pref is cleared in the case of B&R. Is that done somewhere - or if not, how would this work? https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:64: Nit: Remove empty line. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java (left): https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:98: if (shouldFetchVariationsSeedBeforeFRE()) { You need to rebase as this was updated on TOT. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java (right): https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:32: Nit: Remove empty line. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:83: Intent initialIntent = mActivityDelegate.getInitialIntent(); Nit: Inline these two into the call below since they're not used elsewhere in this function? https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:84: boolean initVariationSeed = FirstRunFlowSequencer.checkIfFirstRunIsNecessary(context, Nit: I'd call this var fetchVariationsSeed.
The CQ bit was checked by aberent@chromium.org to run a CQ dry run
https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:60: // network access, but must be less than the restore timeout to be useful. On 2017/01/12 15:56:02, Bernhard Bauer wrote: > What is the restore timeout? Done. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:61: private static final long BACKGROUND_TASK_TIMEOUT = 20; On 2017/01/12 15:56:02, Bernhard Bauer wrote: > Add a unit? Done. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:273: }.startBackgroundTasks(false, true); On 2017/01/12 16:06:28, Alexei Svitkine (slow) wrote: > Can you add /* */ comments documenting the params or add vars for them above so > it's clear what you're passing? Done. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:278: // Ignore timeouts. Problems with the variation seed can be ignored, and other On 2017/01/12 15:56:02, Bernhard Bauer wrote: > Nit: If this should time out, await() is not going to throw an > InterruptedException. That happens if something signals the current thread (via > Thread.interrupt(); see > https://docs.oracle.com/javase/tutorial/essential/concurrency/interrupt.html), > which to my knowledge we never do in Chrome. If it does timeout, the method is > going to return false, which we already ignore, but the comment is a bit > misleading if it's in the catch clause. Done. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:290: })) { On 2017/01/12 15:56:02, Bernhard Bauer wrote: > Nit: this looks really awkward. Can we maybe extract the return value if a local > variable to break up the two blocks? Done. Also similar change in onBackup. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java (right): https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:33: On 2017/01/12 16:06:28, Alexei Svitkine (slow) wrote: > Nit: Remove empty line. You can add it after TAG if you'd like. Done. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:39: private static boolean shouldFetchVariationsSeedDuringFirstRun() { On 2017/01/12 16:06:28, Alexei Svitkine (slow) wrote: > This logic changed on TOT in the meantime - this function was deleted and > replace with a call to ChromeVersionInfo.isOfficialBuild() directly. Done. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:48: * Start the background tasks. On 2017/01/12 16:06:28, Alexei Svitkine (slow) wrote: > Nit: Javadoc for params. Done. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:51: final boolean initVariationSeed) { On 2017/01/12 16:06:28, Alexei Svitkine (slow) wrote: > Nit: No need for final on the second var. Done. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:56: boolean variationsInitialized = prefs.getBoolean(VARIATIONS_INITIALIZED, false); On 2017/01/12 16:06:28, Alexei Svitkine (slow) wrote: > I don't understand how this will work. > > If this Java pref will be restored by B&R, then it will have a value of true > while the native side would have been lost. > > For this to work, we would need to make sure this pref is cleared in the case of > B&R. Is that done somewhere - or if not, how would this work? Backup only saves and restores a very limited subset of the both the native and the Java prefs (see the list in ChromeBackupAgent), not including this one, so this will not be restored by B&R. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:64: On 2017/01/12 16:06:28, Alexei Svitkine (slow) wrote: > Nit: Remove empty line. Done. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java (left): https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:98: if (shouldFetchVariationsSeedBeforeFRE()) { On 2017/01/12 16:06:28, Alexei Svitkine (slow) wrote: > You need to rebase as this was updated on TOT. Done. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java (right): https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:32: On 2017/01/12 16:06:28, Alexei Svitkine (slow) wrote: > Nit: Remove empty line. Done. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:83: Intent initialIntent = mActivityDelegate.getInitialIntent(); On 2017/01/12 16:06:28, Alexei Svitkine (slow) wrote: > Nit: Inline these two into the call below since they're not used elsewhere in > this function? Done. https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:84: boolean initVariationSeed = FirstRunFlowSequencer.checkIfFirstRunIsNecessary(context, On 2017/01/12 16:06:28, Alexei Svitkine (slow) wrote: > Nit: I'd call this var fetchVariationsSeed. Done.
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/2627093009/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java (right): https://codereview.chromium.org/2627093009/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:56: boolean variationsInitialized = prefs.getBoolean(VARIATIONS_INITIALIZED, false); On 2017/01/12 17:31:50, aberent wrote: > On 2017/01/12 16:06:28, Alexei Svitkine (slow) wrote: > > I don't understand how this will work. > > > > If this Java pref will be restored by B&R, then it will have a value of true > > while the native side would have been lost. > > > > For this to work, we would need to make sure this pref is cleared in the case > of > > B&R. Is that done somewhere - or if not, how would this work? > > Backup only saves and restores a very limited subset of the both the native and > the Java prefs (see the list in ChromeBackupAgent), not including this one, so > this will not be restored by B&R. Ah, I did not realise this! It means that my fix for crbug.com/669543 was not sufficient - sending another CL now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:240: ThreadUtils.runOnUiThreadBlocking(new Runnable() { Seems a lot of following code could be simplified if it was all run on the UI thread. For example, if we guarantee that AsyncInitTaskRunner.startBackgroundTasks() is run on the UI thread, then we can remove the special strict mode stuff in it when it's manipulating prefs. But I realise we also want *this* thread to wait on that stuff being complete. Could we restructure this so that the flow is something like: // Do initial onRestore reading on this thread // Then run path utils & async task runner on UI thread with a given countdown latch. // Wait on latch here. // Now that startup init is done, finish all the initiatilzation on UI thread. https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java (right): https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:34: private static final String VARIATIONS_INITIALIZED = "variations_initialized"; Nit: It's not obvious this is a pref from this declaration - maybe name it _PREF? https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:35: private boolean mWaitingForVariationsFetch; Nit: Add a blank line before the instance variables. https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:46: boolean initVariationSeed) { Nit: Rename to fetchVariationsSeed https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:47: Nit: No blank line https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java (left): https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:88: // TODO(asvitkine): Consider moving this logic to a singleton, like You're removing the TODO but not actually addressing this, as far as I can tell. Specifically, each NativeInitializationController (each owned by an AsyncInitializationActivity) will still try to perform this work separately which is wasteful. Can your refactoring also address this TODO? i.e. have a singleton that does the async tasks and remember their state and keeps track of callers that are initiating this - i.e. basically the singleton would keep a list of objects to notify ... and if a new object wants to be added after everything has already been done, it can be notified right away of the success/failure.
Now ready for full review: torne@ - Please take a look, as Owner, at my addition to LibraryLoader.java bauberb@, advitkine@ - please review the tests, and the other changes I have made (mainly to make the code testable) since your last review.
aberent@chromium.org changed reviewers: + torne@chromium.org
base/ LGTM
https://codereview.chromium.org/2627093009/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java (right): https://codereview.chromium.org/2627093009/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:133: public abstract void onSuccess(); Add Javadoc. Also, can this be protected? https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java (right): https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:335: AsyncInitTaskRunner createMockInitTaskRunner() { Can this be private? https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:337: when(mAgent.createAsyncInitTaskRunner(any(CountDownLatch.class))).thenAnswer(new Answer() { Can you use a subclass of ChromeBackupAgent that overrides createAsyncInitTaskRunner()? https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:383: // complete. If it isn't completed before the test exists Robolectric crashes with a null Nit: s/exists/exits/. Also, maybe move this to teardown? https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/init/AsyncInitTaskRunnerTest.java (right): https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/init/AsyncInitTaskRunnerTest.java:79: Thread.sleep(THREAD_WAIT_TIME_MS); Ugh. I don't want to block a fix for the bug too much, but this is really not great for testability. There is this TODO in AsyncInitTaskRunner to consider using AsyncTask; maybe now would be the right time for that? :)
aberent@chromium.org changed reviewers: + dpranke@chromium.org
dpranke@chromium.org: Please review changes in chromium.linux.json as OWNER. One new Junit test set added. Other reviewers PTAL. If possible I would like to land this before the branch point. https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:240: ThreadUtils.runOnUiThreadBlocking(new Runnable() { On 2017/01/13 16:33:29, Alexei Svitkine (slow) wrote: > Seems a lot of following code could be simplified if it was all run on the UI > thread. > > For example, if we guarantee that AsyncInitTaskRunner.startBackgroundTasks() is > run on the UI thread, then we can remove the special strict mode stuff in it Actually the strict mode stuff is only needed if *are* running on the UI thread. The problem is that the first time the Android preferences are read they do a file read, which isn't allowed on the UI thread if we have full strict mode enforcement. AsyncInitTaskRunner is likely to be the first thing to read them during normal startup. > when it's manipulating prefs. > > But I realise we also want *this* thread to wait on that stuff being complete. > > Could we restructure this so that the flow is something like: > > // Do initial onRestore reading on this thread > // Then run path utils & async task runner on UI thread with a given countdown > latch. > // Wait on latch here. > // Now that startup init is done, finish all the initiatilzation on UI thread. Done. It doesn't currently simplify AsyncInitTaskRunner, but might do in future. In particular, guaranteeing it runs on the UI thread will allow us to use Android AsyncTasks instead of threads and services. Switching to AsyncTasks is probably, however, best done in a separate CL. https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java (right): https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:34: private static final String VARIATIONS_INITIALIZED = "variations_initialized"; On 2017/01/13 16:33:29, Alexei Svitkine (slow) wrote: > Nit: It's not obvious this is a pref from this declaration - maybe name it > _PREF? Done. https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:35: private boolean mWaitingForVariationsFetch; On 2017/01/13 16:33:29, Alexei Svitkine (slow) wrote: > Nit: Add a blank line before the instance variables. Done. https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:46: boolean initVariationSeed) { On 2017/01/13 16:33:29, Alexei Svitkine (slow) wrote: > Nit: Rename to fetchVariationsSeed Done. https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:47: On 2017/01/13 16:33:29, Alexei Svitkine (slow) wrote: > Nit: No blank line Done. https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java (left): https://codereview.chromium.org/2627093009/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:88: // TODO(asvitkine): Consider moving this logic to a singleton, like On 2017/01/13 16:33:29, Alexei Svitkine (slow) wrote: > You're removing the TODO but not actually addressing this, as far as I can tell. > > Specifically, each NativeInitializationController (each owned by an > AsyncInitializationActivity) will still try to perform this work separately > which is wasteful. > > Can your refactoring also address this TODO? > > i.e. have a singleton that does the async tasks and remember their state and > keeps track of callers that are initiating this - i.e. basically the singleton > would keep a list of objects to notify ... and if a new object wants to be added > after everything has already been done, it can be notified right away of the > success/failure. I didn't actually mean to remove the TODO (although maybe I should have)! It was a cut and paste error. In fact I don't think significant work will happen twice: 1. The library loader is already a singleton, LibraryLoader.ensureInitialized() is synchronised, so the library will only load once. 2. The variations seed will only be fetched once, because the pref will prevent it being fetched a second time. In my latest patchset I have converted the fetcher from a service to singleton that runs within an AsyncTask. In addition, as with the LibraryLoader, the fetch is (with my latest patchset) actually synchronised, so if a second fetch is requested while the first one is in progress it will wait, and then use the result of the first fetch. In fact I don't think this can ever happen (it would need two simultaneous restores or first runs requiring FREs) but the logic is there to handle it if it does. https://codereview.chromium.org/2627093009/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java (right): https://codereview.chromium.org/2627093009/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:133: public abstract void onSuccess(); On 2017/01/17 13:52:13, Bernhard Bauer wrote: > Add Javadoc. Also, can this be protected? Done. https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java (right): https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:335: AsyncInitTaskRunner createMockInitTaskRunner() { On 2017/01/17 13:52:13, Bernhard Bauer wrote: > Can this be private? Actually it has gone away, and its code has moved to setUp. https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:337: when(mAgent.createAsyncInitTaskRunner(any(CountDownLatch.class))).thenAnswer(new Answer() { On 2017/01/17 13:52:13, Bernhard Bauer wrote: > Can you use a subclass of ChromeBackupAgent that overrides > createAsyncInitTaskRunner()? I could, but I don't see the advantage. I would still need to spy on the agent to set results at various points in both the backup tests and the restore tests. I have, however, moved this code setUp, which is, I think, a bit cleaner. https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:383: // complete. If it isn't completed before the test exists Robolectric crashes with a null On 2017/01/17 13:52:13, Bernhard Bauer wrote: > Nit: s/exists/exits/. Also, maybe move this to teardown? Nit fixed. I can't move this to teardown because only the restore tests initialise PathUtils, so the backup tests crash if this is called. https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/init/AsyncInitTaskRunnerTest.java (right): https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/init/AsyncInitTaskRunnerTest.java:79: Thread.sleep(THREAD_WAIT_TIME_MS); On 2017/01/17 13:52:13, Bernhard Bauer wrote: > Ugh. I don't want to block a fix for the bug too much, but this is really not > great for testability. There is this TODO in AsyncInitTaskRunner to consider > using AsyncTask; maybe now would be the right time for that? :) 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/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fetch Finch seed during restore Chrome will fetch a Finch seed during a normal first run, but since may take some time it is only done if the FRE is displayed (hence hiding the delay from the user. The Finch data is not backed up since it would probably be out of date when restored. As a result there was no Finch seed set on the first run following a restore. Fix this by fetching the finch seed during restore. BUG=679386 ========== to ========== Fetch Finch seed during restore Chrome will fetch a Finch seed during a normal first run, but since may take some time it is only done if the FRE is displayed (hence hiding the delay from the user. The Finch data is not backed up since it would probably be out of date when restored. As a result there was no Finch seed set on the first run following a restore. Fix this by fetching the finch seed during restore. This also moves fetching the Finch seed from an Android Service to a background thread (actually an AsyncTask) and gets rid of the no longer used ToS Service. BUG=679386 ==========
Description was changed from ========== Fetch Finch seed during restore Chrome will fetch a Finch seed during a normal first run, but since may take some time it is only done if the FRE is displayed (hence hiding the delay from the user. The Finch data is not backed up since it would probably be out of date when restored. As a result there was no Finch seed set on the first run following a restore. Fix this by fetching the finch seed during restore. This also moves fetching the Finch seed from an Android Service to a background thread (actually an AsyncTask) and gets rid of the no longer used ToS Service. BUG=679386 ========== to ========== Fetch Finch seed during restore Chrome will fetch a Finch seed during a normal first run, but since may take some time it is only done if the FRE is displayed (hence hiding the delay from the user. The Finch data is not backed up since it would probably be out of date when restored. As a result there was no Finch seed set on the first run following a restore. Fix this by fetching the finch seed during restore. This also moves fetching the Finch seed from an Android Service to a background thread (actually an AsyncTask) and gets rid of the no longer used ToS Service. BUG=679386 BUG=680829 ==========
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...)
//testing lgtm.
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/v2/patch-status/codereview.chromium.or...
Some initial comments now - but have to run to a meeting. Will review more after. https://codereview.chromium.org/2627093009/diff/100001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2627093009/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:66: public void fetchSeed() { Needs javadoc. https://codereview.chromium.org/2627093009/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:151: protected URL getServerUrl() throws MalformedURLException { Nit: Ordering seems strange - i.e. this method is in the middle of a bunch of private ones. Can this one and the set* method for testing below be together - maybe near the top? https://codereview.chromium.org/2627093009/diff/100001/components/variations/... File components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java (right): https://codereview.chromium.org/2627093009/diff/100001/components/variations/... components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java:41: private static MyURLStreamHandler sHandler; Why is this a static? It seems it's only there so that it could be returned by MyURLStreamHandlerFactory - why not have the factory be in an instance variable and just set the handler on the factory object in the test? Same comment about sResponseCode - can it be part of an instance scoped object and be retrieved with a getter? Otherwise, having things static will leave over those objects around after the test is done which may have side effects. Better to have everything be instance state.
Looks great - just some more nits. Thanks again for working on this! https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:241: // 1. Fetching the seed involves starting a service and waiting for a broadcast. This is Update comment since we're not longer using a service. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java (right): https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:22: * Class for running the Asynchronous startup tasks that need to be run before the native side is Nit: "Class for" seems redundant. Also no need to capitalize asynchronous. How about: "Runs asynchronous startup tasks..." https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:25: * - Fetching the variations seed on first run. Nit: Remove . since you don't have it in the bullet point above. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:32: Nit: Remove empty line. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:88: * Start the background tasks Nit: Full sentence. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:95: final boolean allocateChildConnection, boolean fetchVariationSeed) { Nit: allocateChildConnection doesn't need to be final now. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:96: ThreadUtils.assertOnUiThread(); Maybe assert that this is not called multiple times? e.g. assert mLoadTask is null? https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:104: Nit: Remove blank line. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:113: if (mFetchSeedTask != null) mFetchSeedTask.cancel(true); Should this set mFetchingVariations = false as well? https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java (right): https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:97: protected void onFailure() { Should this set mBackgroundTasksComplete = true? If not, then document that in a comment above the variable declaration. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java (right): https://codereview.chromium.org/2627093009/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:95: Nit: Remove blank line
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java (right): https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:337: when(mAgent.createAsyncInitTaskRunner(any(CountDownLatch.class))).thenAnswer(new Answer() { On 2017/01/18 22:43:58, aberent wrote: > On 2017/01/17 13:52:13, Bernhard Bauer wrote: > > Can you use a subclass of ChromeBackupAgent that overrides > > createAsyncInitTaskRunner()? > > I could, but I don't see the advantage. I would still need to spy on the agent > to set results at various points in both the backup tests and the restore tests. > > I have, however, moved this code setUp, which is, I think, a bit cleaner. Mostly I don't like the way this implements createAsyncInitTaskRunner() using Answer, which defeats the purpose of having a type system, and smells a bit like we're having too much logic in what is supposed to be a simple mock object. Alternatively, you could use an ArgumentCaptor to get the passed CountDownLatch out of an invocation and manually call countDown() on it, which seems a bit more idiomatic for Mockito. https://codereview.chromium.org/2627093009/diff/100001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2627093009/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:28: * Background service that fetches the variations seed before the actual first run of Chrome. Nit: This is not a service anymore. https://codereview.chromium.org/2627093009/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:66: public void fetchSeed() { These methods are meant to be called on a background thread, right? Can you check that? https://codereview.chromium.org/2627093009/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:102: private boolean downloadContent(Context context) { Existing code, but the return value appears unused. https://codereview.chromium.org/2627093009/diff/100001/components/variations/... File components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java (right): https://codereview.chromium.org/2627093009/diff/100001/components/variations/... components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java:48: // TODO(aberent): Auto-generated method stub :)
In addition to answering the comments I have considerably simplified VariationsSeedFetcherTests by overriding fetching the connection instead of simply overriding fetching the URL. PTAL. https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java (right): https://codereview.chromium.org/2627093009/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:337: when(mAgent.createAsyncInitTaskRunner(any(CountDownLatch.class))).thenAnswer(new Answer() { On 2017/01/19 16:49:47, Bernhard Bauer wrote: > On 2017/01/18 22:43:58, aberent wrote: > > On 2017/01/17 13:52:13, Bernhard Bauer wrote: > > > Can you use a subclass of ChromeBackupAgent that overrides > > > createAsyncInitTaskRunner()? > > > > I could, but I don't see the advantage. I would still need to spy on the agent > > to set results at various points in both the backup tests and the restore > tests. > > > > I have, however, moved this code setUp, which is, I think, a bit cleaner. > > Mostly I don't like the way this implements createAsyncInitTaskRunner() using > Answer, which defeats the purpose of having a type system, and smells a bit like > we're having too much logic in what is supposed to be a simple mock object. I have fixed the typing (now using Answer<AsyncInitTaskRunner>). I don't like using sub > > Alternatively, you could use an ArgumentCaptor to get the passed CountDownLatch > out of an invocation and manually call countDown() on it, which seems a bit more > idiomatic for Mockito. On further consideration I agree. For consistency I have removed all mocking from the class; and have instead created a subclass which overrides both this and the native methods. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:241: // 1. Fetching the seed involves starting a service and waiting for a broadcast. This is On 2017/01/19 15:57:27, Alexei Svitkine (slow) wrote: > Update comment since we're not longer using a service. Done. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java (right): https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:22: * Class for running the Asynchronous startup tasks that need to be run before the native side is On 2017/01/19 15:57:27, Alexei Svitkine (slow) wrote: > Nit: "Class for" seems redundant. Also no need to capitalize asynchronous. > > How about: "Runs asynchronous startup tasks..." Done. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:25: * - Fetching the variations seed on first run. On 2017/01/19 15:57:28, Alexei Svitkine (slow) wrote: > Nit: Remove . since you don't have it in the bullet point above. Done. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:32: On 2017/01/19 15:57:28, Alexei Svitkine (slow) wrote: > Nit: Remove empty line. Done. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:88: * Start the background tasks On 2017/01/19 15:57:28, Alexei Svitkine (slow) wrote: > Nit: Full sentence. Changed to "Starts the background tasks." which is not strictly a grammatically full sentence, but is consistent with the style of many of our other function descriptions. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:95: final boolean allocateChildConnection, boolean fetchVariationSeed) { On 2017/01/19 15:57:28, Alexei Svitkine (slow) wrote: > Nit: allocateChildConnection doesn't need to be final now. Done. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:96: ThreadUtils.assertOnUiThread(); On 2017/01/19 15:57:28, Alexei Svitkine (slow) wrote: > Maybe assert that this is not called multiple times? e.g. assert mLoadTask is > null? Done. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:104: On 2017/01/19 15:57:28, Alexei Svitkine (slow) wrote: > Nit: Remove blank line. Done. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java:113: if (mFetchSeedTask != null) mFetchSeedTask.cancel(true); On 2017/01/19 15:57:28, Alexei Svitkine (slow) wrote: > Should this set mFetchingVariations = false as well? No! Consider a possible future change where the fetching variations task can fail. The load task may have already posted its onPostExecute to the IOThread queue and cancel doesn't remove it. If we set mFetchingVariations to false then when the loadTask's onPostExecute() calls this function it will (incorrectly) think that both tasks have completed successfully and call onSuccess(). https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java (right): https://codereview.chromium.org/2627093009/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java:97: protected void onFailure() { On 2017/01/19 15:57:28, Alexei Svitkine (slow) wrote: > Should this set mBackgroundTasksComplete = true? > > If not, then document that in a comment above the variable declaration. No point, once the background tasks fail nothing will work, and there is no recovery path, so onStartupFailure is expected to show an error and exit. I have added a comment explaining this, Note that the old code similarly didn't set mLibraryLoaded on failure. https://codereview.chromium.org/2627093009/diff/100001/chrome/android/junit/s... File chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java (right): https://codereview.chromium.org/2627093009/diff/100001/chrome/android/junit/s... chrome/android/junit/src/org/chromium/chrome/browser/ChromeBackupAgentTest.java:95: On 2017/01/19 15:57:28, Alexei Svitkine (slow) wrote: > Nit: Remove blank line Done. https://codereview.chromium.org/2627093009/diff/100001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2627093009/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:28: * Background service that fetches the variations seed before the actual first run of Chrome. On 2017/01/19 16:49:47, Bernhard Bauer wrote: > Nit: This is not a service anymore. Done. https://codereview.chromium.org/2627093009/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:66: public void fetchSeed() { On 2017/01/19 15:29:50, Alexei Svitkine (slow) wrote: > Needs javadoc. Done. https://codereview.chromium.org/2627093009/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:66: public void fetchSeed() { On 2017/01/19 16:49:47, Bernhard Bauer wrote: > These methods are meant to be called on a background thread, right? Can you > check that? Done. https://codereview.chromium.org/2627093009/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:102: private boolean downloadContent(Context context) { On 2017/01/19 16:49:47, Bernhard Bauer wrote: > Existing code, but the return value appears unused. Done. https://codereview.chromium.org/2627093009/diff/100001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:151: protected URL getServerUrl() throws MalformedURLException { On 2017/01/19 15:29:50, Alexei Svitkine (slow) wrote: > Nit: Ordering seems strange - i.e. this method is in the middle of a bunch of > private ones. Can this one and the set* method for testing below be together - > maybe near the top? Done. Although this function has now been replaced by getServerConnection() to simplify the tests. https://codereview.chromium.org/2627093009/diff/100001/components/variations/... File components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java (right): https://codereview.chromium.org/2627093009/diff/100001/components/variations/... components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java:41: private static MyURLStreamHandler sHandler; On 2017/01/19 15:29:50, Alexei Svitkine (slow) wrote: > Why is this a static? > > It seems it's only there so that it could be returned by > MyURLStreamHandlerFactory - why not have the factory be in an instance variable > and just set the handler on the factory object in the test? > > Same comment about sResponseCode - can it be part of an instance scoped object > and be retrieved with a getter? > > Otherwise, having things static will leave over those objects around after the > test is done which may have side effects. Better to have everything be instance > state. Gone away. https://codereview.chromium.org/2627093009/diff/100001/components/variations/... components/variations/android/junit/src/org/chromium/components/variations/firstrun/VariationsSeedFetcherTest.java:48: // TODO(aberent): Auto-generated method stub On 2017/01/19 16:49:47, Bernhard Bauer wrote: > :) Gone away. https://codereview.chromium.org/2627093009/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java (right): https://codereview.chromium.org/2627093009/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackupAgent.java:215: // testing difficult since the test code runs on the UI thread. The problem is that I can't simply set up a dummy UI thread in the tests (as I do in VariationsSeedFetcherTest), since the code posts things onto the real UI thread, and waits for them to complete.
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/v2/patch-status/codereview.chromium.or...
lgtm % a couple more comments thanks! https://codereview.chromium.org/2627093009/diff/120001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2627093009/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:80: HttpURLConnection connection; Nit: Just return line 82 below and remove this line and the extra separate return. https://codereview.chromium.org/2627093009/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:98: if (prefs.getBoolean(VARIATIONS_INITIALIZED_PREF, false) Can you preserve a previous bit of the comment here: "or a different Java pref is set that indicates that that the seed exists in the native prefs." I think it's useful to call this out explicitly, otherwise hasNativePref() looks like it's checking a native pref which would be bad to do at this point.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm
https://codereview.chromium.org/2627093009/diff/120001/components/variations/... File components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java (right): https://codereview.chromium.org/2627093009/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:80: HttpURLConnection connection; On 2017/01/19 21:28:55, Alexei Svitkine (slow) wrote: > Nit: Just return line 82 below and remove this line and the extra separate > return. Done. https://codereview.chromium.org/2627093009/diff/120001/components/variations/... components/variations/android/java/src/org/chromium/components/variations/firstrun/VariationsSeedFetcher.java:98: if (prefs.getBoolean(VARIATIONS_INITIALIZED_PREF, false) On 2017/01/19 21:28:55, Alexei Svitkine (slow) wrote: > Can you preserve a previous bit of the comment here: > > "or a different Java pref is set that indicates that that the seed exists in the > native prefs." > > I think it's useful to call this out explicitly, otherwise hasNativePref() looks > like it's checking a native pref which would be bad to do at this point. Done, although expressed slightly differently.
The CQ bit was checked by aberent@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org, dpranke@chromium.org, bauerb@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2627093009/#ps140001 (title: "Fix nits and tests")
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": 140001, "attempt_start_ts": 1485195338494780,
"parent_rev": "d5be401a40c154be41657992cb271a6b3a128f32", "commit_rev":
"6b91ed876fd20ba96e4c1c008d056a0ed98788b6"}
Message was sent while issue was closed.
Description was changed from ========== Fetch Finch seed during restore Chrome will fetch a Finch seed during a normal first run, but since may take some time it is only done if the FRE is displayed (hence hiding the delay from the user. The Finch data is not backed up since it would probably be out of date when restored. As a result there was no Finch seed set on the first run following a restore. Fix this by fetching the finch seed during restore. This also moves fetching the Finch seed from an Android Service to a background thread (actually an AsyncTask) and gets rid of the no longer used ToS Service. BUG=679386 BUG=680829 ========== to ========== Fetch Finch seed during restore Chrome will fetch a Finch seed during a normal first run, but since may take some time it is only done if the FRE is displayed (hence hiding the delay from the user. The Finch data is not backed up since it would probably be out of date when restored. As a result there was no Finch seed set on the first run following a restore. Fix this by fetching the finch seed during restore. This also moves fetching the Finch seed from an Android Service to a background thread (actually an AsyncTask) and gets rid of the no longer used ToS Service. BUG=679386 BUG=680829 Review-Url: https://codereview.chromium.org/2627093009 Cr-Commit-Position: refs/heads/master@{#445436} Committed: https://chromium.googlesource.com/chromium/src/+/6b91ed876fd20ba96e4c1c008d05... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/6b91ed876fd20ba96e4c1c008d05...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2648383002/ by jbudorick@chromium.org. The reason for reverting is: Breaking analyze step on linux_android_rel_ng..
Message was sent while issue was closed.
On 2017/01/23 20:24:27, jbudorick wrote: > A revert of this CL (patchset #8 id:140001) has been created in > https://codereview.chromium.org/2648383002/ by mailto:jbudorick@chromium.org. > > The reason for reverting is: Breaking analyze step on linux_android_rel_ng.. Interesting ... analyze needed the gn_isolate_map.pyl entry, but since the test isn't actually swarmed, it's the *only* thing that needed that entry. Since the whitelist caused us to bypass analyze, we didn't actually catch this in the tryjob. That seems like a bug. |
