Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1985923002:
Wireframe scheduler implementation (Closed)
|
Created:
4 years, 7 months ago by Pete Williamson Modified:
4 years, 6 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, asvitkine+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a rough scheduler implementation.
This also hooks up the scheduler to ChromeBackgroundService
to start processing when our task fires.
BUG=612325
Committed: https://crrev.com/953b7e62de75f61f4589553c263dc456349eb842
Cr-Commit-Position: refs/heads/master@{#396873}
Patch Set 1 #Patch Set 2 : Remove UMA and context variable, clean up #Patch Set 3 : Merge with latest #Patch Set 4 : fix merge (it added an unintended file) #Patch Set 5 : Fix dtor crash #Patch Set 6 : rebase-update #Patch Set 7 : rebase-update #
Total comments: 26
Patch Set 8 : Address BackgroundTask CR feedback from DougArnett #
Total comments: 5
Patch Set 9 : More CR feedback per DougArnett #
Total comments: 20
Patch Set 10 : CR feedback per DewittJ and DougArnett #
Total comments: 14
Patch Set 11 : Removed countdownlatch for blocking. #
Total comments: 7
Patch Set 12 : CR feedback per DougArnett #
Total comments: 8
Patch Set 13 : CR feedback per DewittJ #
Total comments: 25
Patch Set 14 : CR feedback per BauerB #
Total comments: 2
Patch Set 15 : More CR feedback per BauerB #Messages
Total messages: 65 (21 generated)
The CQ bit was checked by petewil@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/1985923002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985923002/20001
Description was changed from ========== Add code to install the BackgroundSchedulerBridge hooking up the C++ and Java sides. This also adds calling across the bridge to send a menu item click down to the request coordinator, to start background offlining. BUG=612325 ========== to ========== Add code to install the BackgroundSchedulerBridge hooking up the C++ and Java sides. This also adds calling across the bridge to send a menu item click down to the request coordinator, to start background offlining. BUG=612325 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
petewil@chromium.org changed reviewers: + dewittj@chromium.org, dougarnett@chromium.org
Dougarnett: Please review everything DewittJ: Please check the Bridge creation and initialization code.
The CQ bit was checked by petewil@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/1985923002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985923002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by petewil@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/1985923002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985923002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Some comments on BackgroundTask https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:62: * Removes references to the native BackgroundSchedulerBridge when it is being destroyed. comment doesn't seem to match code https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:16: * The background task class is for receiving calls from the GCM Network Manager. Consider if better: Handles servicing of background offlining requests when triggered by GcmNetworkManager. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:21: * Handler for a GCM Network Manager activation. This gets called when we have the network and Consider if better: Triggers processing of background offlining requests. This is called when system conditions (such as device idleness, network connection, power conditions) are appropriate for background processing. [if you want to be specific about the mechanism:] This is intended to be called from a GcmTaskService onRunTask() method. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:23: * task processing by passing the call along to the C+ RequestCoordinator. C++ https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:25: public void incomingTask(Context context, Bundle bundle) { some alternative name ideas: process(), runTask(), processBackgroundRequests() Think I would prefer process() as it fits with the startProcessing() bridge call and the onProcessingDone() callback. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:26: // Get notification from GCM Network Manager I don't understand this comment applying to the latch. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:31: // Pass the activation on to the bridge to the c++ RequestCoordinator. c++ => C++ https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:46: mLatch.await(); I expect we will want a timeout in the await at some point (with UMA if it times out). Consider a TODO for now https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:48: Log.e(TAG, "IncomingTask interrupted, returning to GcmNetworkManager"); less severe than Log.e? https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:48: Log.e(TAG, "IncomingTask interrupted, returning to GcmNetworkManager"); Should we be returning at least a boolean so that the GcmTaskService caller could decide to return appropriate return code to GcmNetorkManager (eg, RESULT_RESCHEDULE instead of RESULT_SUCCESS)? Perhaps at least a TODO somewhere. Also, we might consider UMA here for visibility. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:60: private CountDownLatch mLatch; mProcessingDoneLatch ?
https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:32: // TODO(petewil): We only have a context. Is this a reasonable way to get a profile? We might want to at least check the type of profile. If it is incognito, we may want to getOriginalProfile() from it. The C++ Profile also has some other profile types (not sure they apply to Clank though or what could be returned from getLastProfile() - eg, system, guest).
https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:62: * Removes references to the native BackgroundSchedulerBridge when it is being destroyed. On 2016/05/20 16:17:17, dougarnett wrote: > comment doesn't seem to match code Comment is correct, but in this case "reference" is a long that can be cast to a pointer. Comment clarified, let me know if it makes more sense now. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:16: * The background task class is for receiving calls from the GCM Network Manager. On 2016/05/20 16:17:17, dougarnett wrote: > Consider if better: Handles servicing of background offlining requests when > triggered by GcmNetworkManager. Done. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:21: * Handler for a GCM Network Manager activation. This gets called when we have the network and On 2016/05/20 16:17:17, dougarnett wrote: > Consider if better: Triggers processing of background offlining requests. This > is called > when system conditions (such as device idleness, network connection, power > conditions) are > appropriate for background processing. [if you want to be specific about the > mechanism:] This > is intended to be called from a GcmTaskService onRunTask() method. Merged your comment with what I had, see if you like it now. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:23: * task processing by passing the call along to the C+ RequestCoordinator. On 2016/05/20 16:17:17, dougarnett wrote: > C++ Done. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:25: public void incomingTask(Context context, Bundle bundle) { On 2016/05/20 16:17:17, dougarnett wrote: > some alternative name ideas: process(), runTask(), processBackgroundRequests() > Think I would prefer process() as it fits with the startProcessing() bridge call > and the onProcessingDone() callback. Changed to processBackgroundRequests. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:26: // Get notification from GCM Network Manager On 2016/05/20 16:17:17, dougarnett wrote: > I don't understand this comment applying to the latch. Old comment, removed. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:31: // Pass the activation on to the bridge to the c++ RequestCoordinator. On 2016/05/20 16:17:17, dougarnett wrote: > c++ => C++ Done. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:32: // TODO(petewil): We only have a context. Is this a reasonable way to get a profile? On 2016/05/20 16:45:11, dougarnett wrote: > We might want to at least check the type of profile. If it is incognito, we may > want to getOriginalProfile() from it. The C++ Profile also has some other > profile types (not sure they apply to Clank though or what could be returned > from getLastProfile() - eg, system, guest). > Done. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:46: mLatch.await(); On 2016/05/20 16:17:18, dougarnett wrote: > I expect we will want a timeout in the await at some point (with UMA if it times > out). Consider a TODO for now Done. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:48: Log.e(TAG, "IncomingTask interrupted, returning to GcmNetworkManager"); On 2016/05/20 16:17:17, dougarnett wrote: > Should we be returning at least a boolean so that the GcmTaskService caller > could decide to return appropriate return code to GcmNetorkManager (eg, > RESULT_RESCHEDULE instead of RESULT_SUCCESS)? Perhaps at least a TODO somewhere. > Also, we might consider UMA here for visibility. boolean returned, TODO added in ChromeBackgroundService.java https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:48: Log.e(TAG, "IncomingTask interrupted, returning to GcmNetworkManager"); On 2016/05/20 16:17:17, dougarnett wrote: > less severe than Log.e? Changed to Log.w https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:60: private CountDownLatch mLatch; On 2016/05/20 16:17:17, dougarnett wrote: > mProcessingDoneLatch ? Done.
I don't yet see why we need to create bridge instance for downward call. You are calling static method on instance and I don't see the call dispatching the ProcessingDoneCallback in upward direction (so not sure why that can't just be direction invocation from static CalledByNative method). https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:36: bridge.startProcessing(context, this); This is static method, why not just BackgroundSchedulerBridge.startProcessing(context, this)? https://codereview.chromium.org/1985923002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/1985923002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:84: Context context, ProcessingDoneCallback callback) { Are we missing CalledByNative static method that receives this callback and calls it?
Why I create a bridge instance: I need for the bridge to have context and profile variables on the Java side in place when the callback arrives, so it can be dispatched properly. These variables are also needed for the Schedule and Unschedule call later. To make sure the bridge has them, I do the JS side initialization by calling getForProfile(), which saves these variables, and causes the C++ side of the bridge to be created if not already. I've commented further down about where the callback gets called from, please let me know if my explanation is not good, or if the code needs better commenting so the next reviewer won't have the same questions. https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:36: bridge.startProcessing(context, this); On 2016/05/20 18:09:56, dougarnett wrote: > This is static method, why not just > BackgroundSchedulerBridge.startProcessing(context, this)? Done. https://codereview.chromium.org/1985923002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/1985923002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:84: Context context, ProcessingDoneCallback callback) { On 2016/05/20 18:09:56, dougarnett wrote: > Are we missing CalledByNative static method that receives this callback and > calls it? onProcessingDone receives the callback on the Java Side. ProcessingDoneCallback in background_scheduler_bridge.cc:l25 calls the onProcessingDone callback via JNI. Does that answer your question, or are you asking something different?
The CQ bit was checked by petewil@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/1985923002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985923002/160001
On 2016/05/20 19:42:24, Pete Williamson wrote: > Why I create a bridge instance: > I need for the bridge to have context and profile variables on the Java side in > place when the callback arrives, so it can be dispatched properly. These > variables are also needed for the Schedule and Unschedule call later. To make > sure the bridge has them, I do the JS side initialization by calling > getForProfile(), which saves these variables, and causes the C++ side of the > bridge to be created if not already. > > I've commented further down about where the callback gets called from, please > let me know if my explanation is not good, or if the code needs better > commenting so the next reviewer won't have the same questions. > > https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java > (right): > > https://codereview.chromium.org/1985923002/diff/120001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:36: > bridge.startProcessing(context, this); > On 2016/05/20 18:09:56, dougarnett wrote: > > This is static method, why not just > > BackgroundSchedulerBridge.startProcessing(context, this)? > > Done. > > https://codereview.chromium.org/1985923002/diff/140001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java > (right): > > https://codereview.chromium.org/1985923002/diff/140001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:84: > Context context, ProcessingDoneCallback callback) { > On 2016/05/20 18:09:56, dougarnett wrote: > > Are we missing CalledByNative static method that receives this callback and > > calls it? > > onProcessingDone receives the callback on the Java Side. > > ProcessingDoneCallback in background_scheduler_bridge.cc:l25 calls the > onProcessingDone callback via JNI. > > Does that answer your question, or are you asking something different? Excellent, you did get rid of the bridge instance being needed for the schedule event startProcessing() and callback path. So you only need the instance when doing the schedule/unschedule upcalls - so wonder if that makes lifetime management easier of the Java objects (creation/deletion only from C++ side). I haven't really looked at that yet.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1985923002/diff/140001/chrome/browser/android... File chrome/browser/android/offline_pages/background_scheduler_bridge.cc (right): https://codereview.chromium.org/1985923002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/background_scheduler_bridge.cc:44: RequestCoordinatorFactory::GetForBrowserContext(profile); please add a nullptr check since we don't support all profiles. https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:31: sContext = context; This should not be stored here, it's probably better to just use the application context. https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:57: private static BackgroundSchedulerBridge create(long nativeBackgroundSchedulerBridge) { It's not clear that you really /need/ a bridge at this point. It seems like everything you need can be captured by the relevant ProcessingDoneCallback. https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:90: BackgroundScheduler.schedule(sContext); Instead, pass a Context in here or use the application context (ContextUtils.getApplicationContext()) https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:95: BackgroundScheduler.unschedule(sContext); same. https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:53: mProcessingDoneLatch.await(); Based on the call flow, this will block the UI thread: onRunTask -> runOnUiThread -> handleOfflinePageBackgroundLoad -> task.processBackgroundReqeusts Needs refactoring to avoid that. https://codereview.chromium.org/1985923002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/background_scheduler_bridge.cc (right): https://codereview.chromium.org/1985923002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/background_scheduler_bridge.cc:44: RequestCoordinatorFactory::GetForBrowserContext(profile); needs a null check here, we don't support incognito profiles.
Looks like my draft comments didn't go out with previous reply ... https://codereview.chromium.org/1985923002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/1985923002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:84: Context context, ProcessingDoneCallback callback) { On 2016/05/20 19:42:24, Pete Williamson wrote: > On 2016/05/20 18:09:56, dougarnett wrote: > > Are we missing CalledByNative static method that receives this callback and > > calls it? > > onProcessingDone receives the callback on the Java Side. > > ProcessingDoneCallback in background_scheduler_bridge.cc:l25 calls the > onProcessingDone callback via JNI. > > Does that answer your question, or are you asking something different? Yes, great. So indeed no need for Java instance for calling callback. https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:16: * Handles servicing of background offlining requests coming via the GCMNetworkManager. nit: GcmNetworkManager is the actual capitalization https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:28: public boolean processBackgroundReqeusts(Context context, Bundle bundle) { spelling of Requests
https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:53: mProcessingDoneLatch.await(); On 2016/05/20 21:52:31, dewittj wrote: > Based on the call flow, this will block the UI thread: > > onRunTask -> runOnUiThread -> handleOfflinePageBackgroundLoad -> > task.processBackgroundReqeusts > > Needs refactoring to avoid that. Nice catch! We might want a thread check entering this method if we expect/need it to not be UI thread.
https://codereview.chromium.org/1985923002/diff/140001/chrome/browser/android... File chrome/browser/android/offline_pages/background_scheduler_bridge.cc (right): https://codereview.chromium.org/1985923002/diff/140001/chrome/browser/android... chrome/browser/android/offline_pages/background_scheduler_bridge.cc:44: RequestCoordinatorFactory::GetForBrowserContext(profile); On 2016/05/20 21:52:30, dewittj wrote: > please add a nullptr check since we don't support all profiles. Done. https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:31: sContext = context; On 2016/05/20 21:52:30, dewittj wrote: > This should not be stored here, it's probably better to just use the application > context. Done, but then I deleted the function. https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:57: private static BackgroundSchedulerBridge create(long nativeBackgroundSchedulerBridge) { On 2016/05/20 21:52:31, dewittj wrote: > It's not clear that you really /need/ a bridge at this point. It seems like > everything you need can be captured by the relevant ProcessingDoneCallback. Good point, now that you showed me how to get a context, I can get rid of a lot of the extra bridge mechanism. Thanks, DewittJ! https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:90: BackgroundScheduler.schedule(sContext); On 2016/05/20 21:52:30, dewittj wrote: > Instead, pass a Context in here or use the application context > (ContextUtils.getApplicationContext()) Done. https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:95: BackgroundScheduler.unschedule(sContext); On 2016/05/20 21:52:30, dewittj wrote: > same. Done. https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:16: * Handles servicing of background offlining requests coming via the GCMNetworkManager. On 2016/05/20 22:17:32, dougarnett wrote: > nit: GcmNetworkManager is the actual capitalization Done. https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:28: public boolean processBackgroundReqeusts(Context context, Bundle bundle) { On 2016/05/20 22:17:32, dougarnett wrote: > spelling of Requests Done. https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:53: mProcessingDoneLatch.await(); On 2016/05/20 21:52:31, dewittj wrote: > Based on the call flow, this will block the UI thread: > > onRunTask -> runOnUiThread -> handleOfflinePageBackgroundLoad -> > task.processBackgroundReqeusts > > Needs refactoring to avoid that. The intent is for this to block the incoming thread. However, GCM Network Manager promises us that we will not be called on the UI thread. Added a check that we don't get called on the UI thread. https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:53: mProcessingDoneLatch.await(); On 2016/05/20 22:30:18, dougarnett wrote: > On 2016/05/20 21:52:31, dewittj wrote: > > Based on the call flow, this will block the UI thread: > > > > onRunTask -> runOnUiThread -> handleOfflinePageBackgroundLoad -> > > task.processBackgroundReqeusts > > > > Needs refactoring to avoid that. > > Nice catch! > > We might want a thread check entering this method if we expect/need it to not be > UI thread. Check addded. https://codereview.chromium.org/1985923002/diff/160001/chrome/browser/android... File chrome/browser/android/offline_pages/background_scheduler_bridge.cc (right): https://codereview.chromium.org/1985923002/diff/160001/chrome/browser/android... chrome/browser/android/offline_pages/background_scheduler_bridge.cc:44: RequestCoordinatorFactory::GetForBrowserContext(profile); On 2016/05/20 21:52:31, dewittj wrote: > needs a null check here, we don't support incognito profiles. Done. (but the check is further down, not here)
The CQ bit was checked by petewil@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/1985923002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985923002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yeah, great to see the simplifications. We do need to discuss what to do about ChromeBackgroundService dispatching on UI thread and how we should proceed. https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:53: mProcessingDoneLatch.await(); On 2016/05/23 19:36:31, Pete Williamson wrote: > On 2016/05/20 21:52:31, dewittj wrote: > > Based on the call flow, this will block the UI thread: > > > > onRunTask -> runOnUiThread -> handleOfflinePageBackgroundLoad -> > > task.processBackgroundReqeusts > > > > Needs refactoring to avoid that. > > The intent is for this to block the incoming thread. However, GCM Network > Manager promises us that we will not be called on the UI thread. Added a check > that we don't get called on the UI thread. Added comment to the runOnUiThread call to make this issue more clear. Let's discuss this afternoon. https://codereview.chromium.org/1985923002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/1985923002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:39: ThreadUtils.runOnUiThread(new Runnable() { Pete, we still have the UI thread issue here for our code to block on latch. This onRunTask is returning immediately with RESULT_SUCCESS after queueing the dispatch on the UI thread. We need to revisit our blocking plan (especially if we need to spin up the browser in UI thread) - do we add latch here or do we let it return and try to manage the rescheduling for more work? https://codereview.chromium.org/1985923002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:31: assert (!ThreadUtils.runningOnUiThread()); Will assert as ChromeBackgroundService is currently written
https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:53: mProcessingDoneLatch.await(); > GCM Network Manager promises us that we will not be called on the UI thread. Can you point me to where this promise is made? I haven't seen any reference to which thread we'll be called on in the docs. https://codereview.chromium.org/1985923002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:31: assert (!ThreadUtils.runningOnUiThread()); On 2016/05/24 16:27:58, dougarnett wrote: > Will assert as ChromeBackgroundService is currently written also nit: this should be the first line of the function https://codereview.chromium.org/1985923002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:47: mProcessingDoneLatch.await(); why not just use the two-argument await with a timeout instead of adding a TODO https://codereview.chromium.org/1985923002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/background_scheduler_bridge.h (right): https://codereview.chromium.org/1985923002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/background_scheduler_bridge.h:24: void Schedule(const TriggerCondition& trigger_condition) override; These should be static now, right? https://codereview.chromium.org/1985923002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/1985923002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/request_coordinator_factory.cc:45: // TODO(petewil) Add support for server based offliner when it is ready. I don't think this comment is really necessary here. https://codereview.chromium.org/1985923002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/request_coordinator_factory.cc:49: // Create the Bridge from the C++ side (and it will initialize from the Java I think this is out of date now.
https://codereview.chromium.org/1985923002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/1985923002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:39: ThreadUtils.runOnUiThread(new Runnable() { On 2016/05/24 16:27:58, dougarnett wrote: > Pete, we still have the UI thread issue here for our code to block on latch. > This onRunTask is returning immediately with RESULT_SUCCESS after queueing the > dispatch on the UI thread. We need to revisit our blocking plan (especially if > we need to spin up the browser in UI thread) - do we add latch here or do we let > it return and try to manage the rescheduling for more work? I'v voting for adding the latch here - in the case of an offline task, we will not switch to the UI thread (at least not here, we may need to do it before the JNI bridge crossing). I'll discuss with you this afternoon. https://codereview.chromium.org/1985923002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:31: assert (!ThreadUtils.runningOnUiThread()); On 2016/05/24 16:56:19, dewittj wrote: > On 2016/05/24 16:27:58, dougarnett wrote: > > Will assert as ChromeBackgroundService is currently written > > also nit: this should be the first line of the function Done. https://codereview.chromium.org/1985923002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:31: assert (!ThreadUtils.runningOnUiThread()); On 2016/05/24 16:27:58, dougarnett wrote: > Will assert as ChromeBackgroundService is currently written Noted, I'll discuss the resolution with you (possibly rewriting ChromeBackgroundService for our needs). https://codereview.chromium.org/1985923002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:47: mProcessingDoneLatch.await(); On 2016/05/24 16:56:19, dewittj wrote: > why not just use the two-argument await with a timeout instead of adding a TODO Done. https://codereview.chromium.org/1985923002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/background_scheduler_bridge.h (right): https://codereview.chromium.org/1985923002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/background_scheduler_bridge.h:24: void Schedule(const TriggerCondition& trigger_condition) override; On 2016/05/24 16:56:19, dewittj wrote: > These should be static now, right? If I continue to use the Scheduler interface, I can't make them static (static functions don't override). I'd like to keep the interface because I expect more logic to go into the scheduler in the future. I'm open to discussion if you think the scheduler interface is not worth making these functions static for. https://codereview.chromium.org/1985923002/diff/180001/chrome/browser/android... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/1985923002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/request_coordinator_factory.cc:45: // TODO(petewil) Add support for server based offliner when it is ready. On 2016/05/24 16:56:19, dewittj wrote: > I don't think this comment is really necessary here. As I recall, I was asked to add it in a previous code review so we don't forget about a server based offliner. Since we aren't developing the server based offliner, I could still remove it. https://codereview.chromium.org/1985923002/diff/180001/chrome/browser/android... chrome/browser/android/offline_pages/request_coordinator_factory.cc:49: // Create the Bridge from the C++ side (and it will initialize from the Java On 2016/05/24 16:56:19, dewittj wrote: > I think this is out of date now. Done.
After talking it over with DougArnett, I've decided to remove the CountDownLatch and rework our waiting strategy in a following changelist.
Description was changed from ========== Add code to install the BackgroundSchedulerBridge hooking up the C++ and Java sides. This also adds calling across the bridge to send a menu item click down to the request coordinator, to start background offlining. BUG=612325 ========== to ========== Add a rough scheduler implementation. This also adds calling across the bridge to send a menu item click down to the request coordinator, to start background offlining. BUG=612325 ==========
Description was changed from ========== Add a rough scheduler implementation. This also adds calling across the bridge to send a menu item click down to the request coordinator, to start background offlining. BUG=612325 ========== to ========== Add a rough scheduler implementation. This also hooks up the scheduler to ChromeBackgroundService to start processing when our task fires. BUG=612325 ==========
https://codereview.chromium.org/1985923002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/1985923002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:133: // Call BackgroundTask, provide context. Synch call, will not return for awhile Update comment - do not want to suggest blocking call on UI thread here https://codereview.chromium.org/1985923002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:44: * @param result - TODO(petewil): What is this for again? What does true mean? Yes, we need to define. I believe the idea was for this to indicate whether some work was actually done or not (in case useful for deciding to capture UMA here) https://codereview.chromium.org/1985923002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:47: TODO here for releasing wakelock (once one is set)
https://codereview.chromium.org/1985923002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/1985923002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:133: // Call BackgroundTask, provide context. Synch call, will not return for awhile On 2016/05/25 18:38:29, dougarnett wrote: > Update comment - do not want to suggest blocking call on UI thread here Done. https://codereview.chromium.org/1985923002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:44: * @param result - TODO(petewil): What is this for again? What does true mean? On 2016/05/25 18:38:29, dougarnett wrote: > Yes, we need to define. I believe the idea was for this to indicate whether some > work was actually done or not (in case useful for deciding to capture UMA here) Done. https://codereview.chromium.org/1985923002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:47: On 2016/05/25 18:38:29, dougarnett wrote: > TODO here for releasing wakelock (once one is set) I'd rather leave the TODO out. I already have code to release the wakelock in another changelist (which is backed up behind this one).
lgtm https://codereview.chromium.org/1985923002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:47: On 2016/05/25 21:47:46, Pete Williamson wrote: > On 2016/05/25 18:38:29, dougarnett wrote: > > TODO here for releasing wakelock (once one is set) > > I'd rather leave the TODO out. I already have code to release the wakelock in > another changelist (which is backed up behind this one). To avoid merge conflict? Seems like this CL would stand by itself better with this foreshadowing of how this callback might be used and TODOs are lightweight. But not blocking.
lgtm with comments https://codereview.chromium.org/1985923002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/1985923002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:128: private void handleOfflinePageBackgroundLoad(Context context, Bundle bundle) { All the other methods are @VisibleForTesting - is there a test you could hook into here? https://codereview.chromium.org/1985923002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java (right): https://codereview.chromium.org/1985923002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:23: public static final String DATE_TAG = "Date"; nit: private https://codereview.chromium.org/1985923002/diff/220001/chrome/browser/android... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/1985923002/diff/220001/chrome/browser/android... chrome/browser/android/offline_pages/request_coordinator_factory.cc:45: // TODO(petewil) Add support for server based offliner when it is ready. I still prefer removing this comment. https://codereview.chromium.org/1985923002/diff/220001/components/offline_pag... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1985923002/diff/220001/components/offline_pag... components/offline_pages/background/request_coordinator.h:61: Scheduler* GetScheduler() { return scheduler_.get(); } I think that any functions that are inlined like this should be lower_case(), so s/GetScheduler/scheduler/ Also should do GetQueue() -> queue()
https://codereview.chromium.org/1985923002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/1985923002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:128: private void handleOfflinePageBackgroundLoad(Context context, Bundle bundle) { On 2016/05/25 23:16:44, dewittj wrote: > All the other methods are @VisibleForTesting - is there a test you could hook > into here? Tests for this patch are coming in another patch I have which is waiting on this changelist, including a test to ChromeBackgroundService for this behavior. https://codereview.chromium.org/1985923002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java (right): https://codereview.chromium.org/1985923002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:23: public static final String DATE_TAG = "Date"; On 2016/05/25 23:16:45, dewittj wrote: > nit: private Done. https://codereview.chromium.org/1985923002/diff/220001/chrome/browser/android... File chrome/browser/android/offline_pages/request_coordinator_factory.cc (right): https://codereview.chromium.org/1985923002/diff/220001/chrome/browser/android... chrome/browser/android/offline_pages/request_coordinator_factory.cc:45: // TODO(petewil) Add support for server based offliner when it is ready. On 2016/05/25 23:16:45, dewittj wrote: > I still prefer removing this comment. Done. https://codereview.chromium.org/1985923002/diff/220001/components/offline_pag... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/1985923002/diff/220001/components/offline_pag... components/offline_pages/background/request_coordinator.h:61: Scheduler* GetScheduler() { return scheduler_.get(); } On 2016/05/25 23:16:45, dewittj wrote: > I think that any functions that are inlined like this should be lower_case(), so > s/GetScheduler/scheduler/ > > Also should do GetQueue() -> queue() Done.
petewil@chromium.org changed reviewers: + bauerb@chromium.org
bauerb@chromium.org: Please review changes in ChromeBackgroundService.java. You reviewed the original add of the BackgroundOfflining code to ChromeBackgroundService, (https://codereview.chromium.org/1956633002) and more info is available here: go/chrome-background-offlining. Thanks!
https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:134: BackgroundTask task = new BackgroundTask(); It's a little bit unfortunate to use a class plainly named BackgroundTask here, as ChromeBackgroundService refers to doing arbitrary things in the background, but BackgroundTask is just about offlining. Could we maybe rename that to BackgroundOffliningTask? https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:135: task.processBackgroundRequests(context, bundle); You're not doing anything with |task| afterwards -- you could just make the method static. https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:33: // TODO(petewil): Today this puts timestamp, add triggering conditions into bundle. Nit: I can't parse this sentence :) https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:37: Date now = new Date(); I don't exactly know what you are going to use this for, but note that wall clock time might change. It looks like your task is not persisting across reboots, so if you just want to get elapsed time when the task is run, you could use a guaranteed monotonic clock (e.g. SystemClock.elapsedRealtime). https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:38: taskExtras.putLong(DATE_TAG, now.getTime()); Hm... this creates the bundle, but BackgroundSchedulerBridge pulls information out of it (or at least is supposed to). It would probably make more sense to do both in the same class? https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:40: // Create a task. Nit: some of these comments are bordering on no informational content that can't be gathered from the code :) https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:33: BackgroundSchedulerBridge.startProcessing(context, this); Do you actually need to pass the context to the native side? It's not going to be able to do a lot with it. https://codereview.chromium.org/1985923002/diff/240001/chrome/browser/android... File chrome/browser/android/offline_pages/background_scheduler_bridge.cc (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/background_scheduler_bridge.cc:38: j_callback_ref.Reset(env, j_callback_obj); You can directly initialize |j_callback_ref|.
https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:134: BackgroundTask task = new BackgroundTask(); On 2016/05/26 11:17:44, Bernhard Bauer wrote: > It's a little bit unfortunate to use a class plainly named BackgroundTask here, > as ChromeBackgroundService refers to doing arbitrary things in the background, > but BackgroundTask is just about offlining. Could we maybe rename that to > BackgroundOffliningTask? Renamed to BackgroundOfflinerTask (we use Offliner in other places). https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:135: task.processBackgroundRequests(context, bundle); On 2016/05/26 11:17:44, Bernhard Bauer wrote: > You're not doing anything with |task| afterwards -- you could just make the > method static. The method needs to be a class method, since it passes a this pointer to the background scheduler bridge so the brige can get to a callback. https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:33: // TODO(petewil): Today this puts timestamp, add triggering conditions into bundle. On 2016/05/26 11:17:44, Bernhard Bauer wrote: > Nit: I can't parse this sentence :) OK, try again, I rephrased it,hopefully with better grammar. https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:37: Date now = new Date(); On 2016/05/26 11:17:44, Bernhard Bauer wrote: > I don't exactly know what you are going to use this for, but note that wall > clock time might change. It looks like your task is not persisting across > reboots, so if you just want to get elapsed time when the task is run, you could > use a guaranteed monotonic clock (e.g. SystemClock.elapsedRealtime). This is to mark the starting point of an elapsed time calculation we will make when the task runs. I'm definitely open to using a different kind of time class, but I'm not sure which is best here, and I'd welcome any feedback. https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:38: taskExtras.putLong(DATE_TAG, now.getTime()); On 2016/05/26 11:17:44, Bernhard Bauer wrote: > Hm... this creates the bundle, but BackgroundSchedulerBridge pulls information > out of it (or at least is supposed to). It would probably make more sense to do > both in the same class? We want to keep BackgroundSchedulerBridge at thin as possible because it is hard to test across the JNI boundary, so we wanted to keep that out of BackgroundSchedulerBridge. BackgroundTask (Now BackgroundOfflinerTask) will extract this date in the future to make an elapsed time computation for UMA purposes.) https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:40: // Create a task. On 2016/05/26 11:17:44, Bernhard Bauer wrote: > Nit: some of these comments are bordering on no informational content that can't > be gathered from the code :) Comment removed. Yep, sorry, that's my personal style clashing with the style guidelines. I've always thought that it is faster to read comments to see what is going on than read the code, so you can scan past the code faster looking for relevant parts. https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundTask.java:33: BackgroundSchedulerBridge.startProcessing(context, this); On 2016/05/26 11:17:44, Bernhard Bauer wrote: > Do you actually need to pass the context to the native side? It's not going to > be able to do a lot with it. Right, I actually need the profile. I removed the context for now, I'll add the profile in a following changelist. The context was leftover from a previous approach and should have been removed already, good catch. https://codereview.chromium.org/1985923002/diff/240001/chrome/browser/android... File chrome/browser/android/offline_pages/background_scheduler_bridge.cc (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/browser/android... chrome/browser/android/offline_pages/background_scheduler_bridge.cc:38: j_callback_ref.Reset(env, j_callback_obj); On 2016/05/26 11:17:44, Bernhard Bauer wrote: > You can directly initialize |j_callback_ref|. Coming in a future changelist (I already have it coded, but it is coming along with other changes, and I wanted to keep this changelist small).
The CQ bit was checked by petewil@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/1985923002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985923002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:135: task.processBackgroundRequests(context, bundle); On 2016/05/26 17:02:44, Pete Williamson wrote: > On 2016/05/26 11:17:44, Bernhard Bauer wrote: > > You're not doing anything with |task| afterwards -- you could just make the > > method static. > > The method needs to be a class method, since it passes a this pointer to the > background scheduler bridge so the brige can get to a callback. Yeah, but you could create an instance inside of the static method, and then make the constructor private, to reduce the public API surface. https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:37: Date now = new Date(); On 2016/05/26 17:02:45, Pete Williamson wrote: > On 2016/05/26 11:17:44, Bernhard Bauer wrote: > > I don't exactly know what you are going to use this for, but note that wall > > clock time might change. It looks like your task is not persisting across > > reboots, so if you just want to get elapsed time when the task is run, you > could > > use a guaranteed monotonic clock (e.g. SystemClock.elapsedRealtime). > > This is to mark the starting point of an elapsed time calculation we will make > when the task runs. I'm definitely open to using a different kind of time > class, but I'm not sure which is best here, and I'd welcome any feedback. OK, yeah, I would use SystemClock.elapsedRealtime() for that. https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:38: taskExtras.putLong(DATE_TAG, now.getTime()); On 2016/05/26 17:02:45, Pete Williamson wrote: > On 2016/05/26 11:17:44, Bernhard Bauer wrote: > > Hm... this creates the bundle, but BackgroundSchedulerBridge pulls information > > out of it (or at least is supposed to). It would probably make more sense to > do > > both in the same class? > > We want to keep BackgroundSchedulerBridge at thin as possible because it is hard > to test across the JNI boundary, so we wanted to keep that out of > BackgroundSchedulerBridge. BackgroundTask (Now BackgroundOfflinerTask) will > extract this date in the future to make an elapsed time computation for UMA > purposes.) Yeah, it doesn't have to be in BackgroundSchedulerBridge, but it might be a good idea to have bundle encoding and decoding in the same class (once you actually do that). FTR, if you want to ease testing, you could actually move the native methods into a non-static class, where you could override them in a subclass for testing. Otherwise even with a thin BackgroundSchedulerBridge it will be difficult to stub them out in a test (you'll just have moved the problem from avoiding a nativeFoo() call to avoiding a static BackgroundSchedulerBridge.foo() call). https://codereview.chromium.org/1985923002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java (right): https://codereview.chromium.org/1985923002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:46: public void onProcessingDone(boolean result) { Should this have @Override?
https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:135: task.processBackgroundRequests(context, bundle); On 2016/05/27 12:36:15, Bernhard Bauer wrote: > On 2016/05/26 17:02:44, Pete Williamson wrote: > > On 2016/05/26 11:17:44, Bernhard Bauer wrote: > > > You're not doing anything with |task| afterwards -- you could just make the > > > method static. > > > > The method needs to be a class method, since it passes a this pointer to the > > background scheduler bridge so the brige can get to a callback. > > Yeah, but you could create an instance inside of the static method, and then > make the constructor private, to reduce the public API surface. Done. https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:37: Date now = new Date(); On 2016/05/27 12:36:15, Bernhard Bauer wrote: > On 2016/05/26 17:02:45, Pete Williamson wrote: > > On 2016/05/26 11:17:44, Bernhard Bauer wrote: > > > I don't exactly know what you are going to use this for, but note that wall > > > clock time might change. It looks like your task is not persisting across > > > reboots, so if you just want to get elapsed time when the task is run, you > > could > > > use a guaranteed monotonic clock (e.g. SystemClock.elapsedRealtime). > > > > This is to mark the starting point of an elapsed time calculation we will make > > when the task runs. I'm definitely open to using a different kind of time > > class, but I'm not sure which is best here, and I'd welcome any feedback. > > OK, yeah, I would use SystemClock.elapsedRealtime() for that. Done. I'll need to be careful to check for negative elapsed time in case the device reboots, then. https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:38: taskExtras.putLong(DATE_TAG, now.getTime()); On 2016/05/27 12:36:15, Bernhard Bauer wrote: > On 2016/05/26 17:02:45, Pete Williamson wrote: > > On 2016/05/26 11:17:44, Bernhard Bauer wrote: > > > Hm... this creates the bundle, but BackgroundSchedulerBridge pulls > information > > > out of it (or at least is supposed to). It would probably make more sense to > > do > > > both in the same class? > > > > We want to keep BackgroundSchedulerBridge at thin as possible because it is > hard > > to test across the JNI boundary, so we wanted to keep that out of > > BackgroundSchedulerBridge. BackgroundTask (Now BackgroundOfflinerTask) will > > extract this date in the future to make an elapsed time computation for UMA > > purposes.) > > Yeah, it doesn't have to be in BackgroundSchedulerBridge, but it might be a good > idea to have bundle encoding and decoding in the same class (once you actually > do that). > > FTR, if you want to ease testing, you could actually move the native methods > into a non-static class, where you could override them in a subclass for > testing. Otherwise even with a thin BackgroundSchedulerBridge it will be > difficult to stub them out in a test (you'll just have moved the problem from > avoiding a nativeFoo() call to avoiding a static BackgroundSchedulerBridge.foo() > call). Good idea. I'm working on a test changelist right now, and I'll make a non-static class that I can mock which calls out to the static bridge class. The non-static class might be a good place for the packing/unpacking logic. TODO added. https://codereview.chromium.org/1985923002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java (right): https://codereview.chromium.org/1985923002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:46: public void onProcessingDone(boolean result) { On 2016/05/27 12:36:16, Bernhard Bauer wrote: > Should this have @Override? Done.
lgtm https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:37: Date now = new Date(); On 2016/05/27 17:57:58, Pete Williamson wrote: > On 2016/05/27 12:36:15, Bernhard Bauer wrote: > > On 2016/05/26 17:02:45, Pete Williamson wrote: > > > On 2016/05/26 11:17:44, Bernhard Bauer wrote: > > > > I don't exactly know what you are going to use this for, but note that > wall > > > > clock time might change. It looks like your task is not persisting across > > > > reboots, so if you just want to get elapsed time when the task is run, you > > > could > > > > use a guaranteed monotonic clock (e.g. SystemClock.elapsedRealtime). > > > > > > This is to mark the starting point of an elapsed time calculation we will > make > > > when the task runs. I'm definitely open to using a different kind of time > > > class, but I'm not sure which is best here, and I'd welcome any feedback. > > > > OK, yeah, I would use SystemClock.elapsedRealtime() for that. > > Done. I'll need to be careful to check for negative elapsed time in case the > device reboots, then. AFAICT, your tasks are currently not persisted across reboots, so that couldn't happen.
Thanks, BauerB! https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:37: Date now = new Date(); On 2016/05/31 14:15:26, Bernhard Bauer wrote: > On 2016/05/27 17:57:58, Pete Williamson wrote: > > On 2016/05/27 12:36:15, Bernhard Bauer wrote: > > > On 2016/05/26 17:02:45, Pete Williamson wrote: > > > > On 2016/05/26 11:17:44, Bernhard Bauer wrote: > > > > > I don't exactly know what you are going to use this for, but note that > > wall > > > > > clock time might change. It looks like your task is not persisting > across > > > > > reboots, so if you just want to get elapsed time when the task is run, > you > > > > could > > > > > use a guaranteed monotonic clock (e.g. SystemClock.elapsedRealtime). > > > > > > > > This is to mark the starting point of an elapsed time calculation we will > > make > > > > when the task runs. I'm definitely open to using a different kind of time > > > > class, but I'm not sure which is best here, and I'd welcome any feedback. > > > > > > OK, yeah, I would use SystemClock.elapsedRealtime() for that. > > > > Done. I'll need to be careful to check for negative elapsed time in case the > > device reboots, then. > > AFAICT, your tasks are currently not persisted across reboots, so that couldn't > happen. FYI: Our tasks are persisted across reboots. We put the date into the bundle that we give the GCM network manager to give back to us when it wakes up Chrome. There is a chance Chrome will have gotten kicked out of memory before it wakes up. The android device could reboot before the network comes back. So, we are using the bundle to persist the task start time, even across reboots (Assuming the GCM Network Manager still remembers to wake us up across reboots.)
https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java (right): https://codereview.chromium.org/1985923002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:37: Date now = new Date(); On 2016/05/31 14:30:15, Pete Williamson wrote: > On 2016/05/31 14:15:26, Bernhard Bauer wrote: > > On 2016/05/27 17:57:58, Pete Williamson wrote: > > > On 2016/05/27 12:36:15, Bernhard Bauer wrote: > > > > On 2016/05/26 17:02:45, Pete Williamson wrote: > > > > > On 2016/05/26 11:17:44, Bernhard Bauer wrote: > > > > > > I don't exactly know what you are going to use this for, but note that > > > wall > > > > > > clock time might change. It looks like your task is not persisting > > across > > > > > > reboots, so if you just want to get elapsed time when the task is run, > > you > > > > > could > > > > > > use a guaranteed monotonic clock (e.g. SystemClock.elapsedRealtime). > > > > > > > > > > This is to mark the starting point of an elapsed time calculation we > will > > > make > > > > > when the task runs. I'm definitely open to using a different kind of > time > > > > > class, but I'm not sure which is best here, and I'd welcome any > feedback. > > > > > > > > OK, yeah, I would use SystemClock.elapsedRealtime() for that. > > > > > > Done. I'll need to be careful to check for negative elapsed time in case > the > > > device reboots, then. > > > > AFAICT, your tasks are currently not persisted across reboots, so that > couldn't > > happen. > > FYI: Our tasks are persisted across reboots. We put the date into the bundle > that we give the GCM network manager to give back to us when it wakes up Chrome. > There is a chance Chrome will have gotten kicked out of memory before it wakes > up. The android device could reboot before the network comes back. So, we are > using the bundle to persist the task start time, even across reboots (Assuming > the GCM Network Manager still remembers to wake us up across reboots.) Uh, the default for one-off tasks is to be _not_ persisted (see https://developers.google.com/android/reference/com/google/android/gms/gcm/Ta...).
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougarnett@chromium.org, dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/1985923002/#ps280001 (title: "More CR feedback per BauerB")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1985923002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985923002/280001
Message was sent while issue was closed.
Description was changed from ========== Add a rough scheduler implementation. This also hooks up the scheduler to ChromeBackgroundService to start processing when our task fires. BUG=612325 ========== to ========== Add a rough scheduler implementation. This also hooks up the scheduler to ChromeBackgroundService to start processing when our task fires. BUG=612325 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Add a rough scheduler implementation. This also hooks up the scheduler to ChromeBackgroundService to start processing when our task fires. BUG=612325 ========== to ========== Add a rough scheduler implementation. This also hooks up the scheduler to ChromeBackgroundService to start processing when our task fires. BUG=612325 Committed: https://crrev.com/953b7e62de75f61f4589553c263dc456349eb842 Cr-Commit-Position: refs/heads/master@{#396873} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/953b7e62de75f61f4589553c263dc456349eb842 Cr-Commit-Position: refs/heads/master@{#396873} |
