Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2825783003:
[Android] Native initialization task for BackgroundTaskScheduler (Closed)
Description[Android] Native initialization task for BackgroundTaskScheduler
Provides an abstract class implementing BackgroundTask with capability
to ensure that native portion of Chrome is initialized.
BUG=710630
R=nyquist@chromium.org
Review-Url: https://codereview.chromium.org/2825783003
Cr-Commit-Position: refs/heads/master@{#474385}
Committed: https://chromium.googlesource.com/chromium/src/+/acd0e4ec71292ce486b7a5d9ca5ecece6712bfa3
Patch Set 1 #
Total comments: 4
Patch Set 2 : Adding a few tests to NativeBackgroundTask #Patch Set 3 : Rebasing' #Patch Set 4 : Udpating the native task based on comments on design document #
Total comments: 13
Patch Set 5 : Addressing CR feedback #
Dependent Patchsets: Messages
Total messages: 35 (22 generated)
The CQ bit was checked by fgorski@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...
PTAL
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/2825783003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java (right): https://codereview.chromium.org/2825783003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:25: public abstract class NativeBackgroundTask implements BackgroundTask { How would you feel about writing a test to ensure that when the subclass gets a call to onStartTaskWithNative, it does in fact always have native? If you want to run on the host, maybe just create a shadow for BrowserStartupController and ChromeBrowserInitializer? https://codereview.chromium.org/2825783003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:48: protected void ensureNativeInitialized(Context context) { Nit: I think you want @SuppressFBWarnings("DM_EXIT") here.
The CQ bit was checked by fgorski@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...
PTAL https://codereview.chromium.org/2825783003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java (right): https://codereview.chromium.org/2825783003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:25: public abstract class NativeBackgroundTask implements BackgroundTask { On 2017/04/18 22:48:43, nyquist wrote: > How would you feel about writing a test to ensure that when the subclass gets a > call to onStartTaskWithNative, it does in fact always have native? > > If you want to run on the host, maybe just create a shadow for > BrowserStartupController and ChromeBrowserInitializer? Here is what I thought 3 hours ago: hold my beer... It's done now, but you know... lot's of tears :) https://codereview.chromium.org/2825783003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:48: protected void ensureNativeInitialized(Context context) { On 2017/04/18 22:48:44, nyquist wrote: > Nit: I think you want @SuppressFBWarnings("DM_EXIT") here. Actually I thought of a different approach, in which we return success status. If we are not able to start native, onStartTask will return without calling implementation further. This means no sad surprises from the component and no exception while invoked from the background. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
thanks for the test!! lgtm
The CQ bit was checked by fgorski@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...
fgorski@chromium.org changed reviewers: + tedchoc@chromium.org
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
still lgtm https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java (right): https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:48: private boolean mTaskStopped; Nit: Could you add a comment that this must only ever be accessed on the UI thread? https://codereview.chromium.org/2825783003/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTaskTest.java (right): https://codereview.chromium.org/2825783003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTaskTest.java:233: return latch.await(1, TimeUnit.SECONDS); Our devices are sometimes incredibly slow for weird reasons. Could we increase this to something like 5 seconds? It shouldn't slow down the tests in the default case.
https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java (right): https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:30: * #onStartTask(Context, TaskParameters, TaskFinishedCallback)}. I'd argue the last sentence isn't necessary, onStartTaskWithNative is abstract and onStartTask is final, so it should be pretty self explanatory what a embedder should do. https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:112: ChromeBrowserInitializer.getInstance(context).handlePreNativeStartup(parts); Should this do: if (mTaskStopped) return; https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:160: callback.taskFinished(true); Same question as above. Should this do: if (mTaskStopped) return; Should we do ThreadUtil.assertOnUiThread here? https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:171: onStartTaskWithNative(context, taskParameters, callback); same about mTaskStopped?
The CQ bit was checked by fgorski@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...
All accepted. PTAL https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java (right): https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:30: * #onStartTask(Context, TaskParameters, TaskFinishedCallback)}. On 2017/05/23 20:52:55, Ted C wrote: > I'd argue the last sentence isn't necessary, onStartTaskWithNative is abstract > and onStartTask is final, so it should be pretty self explanatory what a > embedder should do. Done. https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:48: private boolean mTaskStopped; On 2017/05/23 20:14:28, nyquist (nychthemeron ping) wrote: > Nit: Could you add a comment that this must only ever be accessed on the UI > thread? Done. https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:112: ChromeBrowserInitializer.getInstance(context).handlePreNativeStartup(parts); On 2017/05/23 20:52:55, Ted C wrote: > Should this do: > > if (mTaskStopped) return; I am putting it before the try, presuming you didn't mean for that check to happen between pre-native and post-native. Let me know if that's incorrect. https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:160: callback.taskFinished(true); On 2017/05/23 20:52:55, Ted C wrote: > Same question as above. Should this do: > > if (mTaskStopped) return; > > Should we do ThreadUtil.assertOnUiThread here? Done. Good point. I went ahead and added checks in both runnables, making sure they are posted to or invoked from UI thread only. https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:171: onStartTaskWithNative(context, taskParameters, callback); On 2017/05/23 20:52:55, Ted C wrote: > same about mTaskStopped? Done. https://codereview.chromium.org/2825783003/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTaskTest.java (right): https://codereview.chromium.org/2825783003/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTaskTest.java:233: return latch.await(1, TimeUnit.SECONDS); On 2017/05/23 20:14:29, nyquist (nychthemeron ping) wrote: > Our devices are sometimes incredibly slow for weird reasons. Could we increase > this to something like 5 seconds? It shouldn't slow down the tests in the > default case. Done.
lgtm https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java (right): https://codereview.chromium.org/2825783003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/background_task_scheduler/NativeBackgroundTask.java:112: ChromeBrowserInitializer.getInstance(context).handlePreNativeStartup(parts); On 2017/05/23 21:35:02, fgorski wrote: > On 2017/05/23 20:52:55, Ted C wrote: > > Should this do: > > > > if (mTaskStopped) return; > > I am putting it before the try, presuming you didn't mean for that check to > happen between pre-native and post-native. Let me know if that's incorrect. Nope, that's right
The CQ bit was unchecked by fgorski@chromium.org
The CQ bit was checked by fgorski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/2825783003/#ps80001 (title: "Addressing CR feedback")
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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by fgorski@chromium.org
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": 80001, "attempt_start_ts": 1495642914827620,
"parent_rev": "a8852f2672df3b8c9f1daeadbcee5243191de8f5", "commit_rev":
"acd0e4ec71292ce486b7a5d9ca5ecece6712bfa3"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Native initialization task for BackgroundTaskScheduler Provides an abstract class implementing BackgroundTask with capability to ensure that native portion of Chrome is initialized. BUG=710630 R=nyquist@chromium.org ========== to ========== [Android] Native initialization task for BackgroundTaskScheduler Provides an abstract class implementing BackgroundTask with capability to ensure that native portion of Chrome is initialized. BUG=710630 R=nyquist@chromium.org Review-Url: https://codereview.chromium.org/2825783003 Cr-Commit-Position: refs/heads/master@{#474385} Committed: https://chromium.googlesource.com/chromium/src/+/acd0e4ec71292ce486b7a5d9ca5e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/acd0e4ec71292ce486b7a5d9ca5e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
