Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2066593003:
Add a general purpose facility for letting background tasks wait until (Closed)
|
Created:
4 years, 6 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, 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 general purpose facility for letting background tasks wait until
the task is done before returning control to GCM Network Manager.
To minimize concurrent network activity, the background offlining task
wants to make sure we hold the thread for the GCM Network Manager until
we are done using the network instead of letting the task start right
away. The idea here is to reduce network contention during the Marshmallow
maintenance window when the network becomes available.
While this is only needed for BackgroundOfflining at the moment, we wanted
to make the feature more generally accessible, any task with a bundle
containing "WaitNeeded" set to anything other than 0 will start a wait.
BUG=612325
patch from issue 1985923002 at patchset 200001 (http://crrev.com/1985923002#ps200001)
Committed: https://crrev.com/71ef97e9c3815152a269b37b67990a7bbd641e1c
Cr-Commit-Position: refs/heads/master@{#400180}
Patch Set 1 #
Total comments: 28
Patch Set 2 : CR feedback per DougArnett #
Total comments: 14
Patch Set 3 : CR feedback per BauerB #Patch Set 4 : rebase #
Total comments: 4
Patch Set 5 : CR fixes per BauerB (round 2) #
Total comments: 4
Patch Set 6 : Last CR feedback from BauerB #Patch Set 7 : merge fix #Patch Set 8 : merge fix #Patch Set 9 : logical merge fix #
Messages
Total messages: 46 (18 generated)
petewil@chromium.org changed reviewers: + dougarnett@chromium.org
To help various consumers and maintainers of ChromeBackgroundService, we may need to document more about the onRunTask called on a work thread and that holds wakelock until the call returns and thus blocking it returns keeps the device awake for processing work (on UI thread). https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:39: /** Bundle value to use to specify we should wait before returning */ Bundle key actually https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:40: public static final String WAIT_NEEDED = "WaitNeeded"; I wonder about something like HOLD_WAKELOCK here as this is kind of the api to the requester. Think we need to be more clear with naming and/or documentation about the intent with this - eg, applies to blocking the onRunTask thread while processing proceed on main UI thread and is responsible for signalling when done. https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:81: // The BackgroundOffliner needs to hold the thread until the operation is complete, Comment not clear wrt waht holding the thread achieve (or what threads are involved)? https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:142: private void handleOfflinePageBackgroundLoad(Context context, Bundle bundle) { remove above annotation if private now https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:143: Log.d(TAG, "handleOfflinePageBackgroundLoad"); remove before landing? https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:158: private void setWaitLatchIfNeeded(Bundle bundle) { actually this method and the next one having javadoc is maybe more important than the call sites having comments https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:160: if (bundle != null && bundle.getLong(WAIT_NEEDED) != 0) { why getLong()? Do you expect to plumb wait time value? If so, should be documented. If not, suggest using putBoolean/getBoolean https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:169: boolean waitSucceeded = mLatch.await(4, TimeUnit.MINUTES); maybe define constant for the 4 value https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:181: mLatch.countDown(); // Release the onRunTask thread to return so that the wakelock being hed for it is cleared. ? https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:204: private BackgroundOfflinerTask mBackgroundOfflinerTask; members before methods for Java classes https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java (right): https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:82: private ChromeBackgroundServiceWaiter mWaiter; members before methods https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java (right): https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:21: bundle.putLong(ChromeBackgroundService.WAIT_NEEDED, 1); putBoolean
Added a lot more doc, PTAL and let me know if you think I should add more. https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:39: /** Bundle value to use to specify we should wait before returning */ On 2016/06/13 21:53:35, dougarnett wrote: > Bundle key actually Done. https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:40: public static final String WAIT_NEEDED = "WaitNeeded"; On 2016/06/13 21:53:34, dougarnett wrote: > I wonder about something like HOLD_WAKELOCK here as this is kind of the api to > the requester. > > Think we need to be more clear with naming and/or documentation about the intent > with this - eg, applies to blocking the onRunTask thread while processing > proceed on main UI thread and is responsible for signalling when done. > Constant changed, I added lots of doc, PTAL. https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:81: // The BackgroundOffliner needs to hold the thread until the operation is complete, On 2016/06/13 21:53:35, dougarnett wrote: > Comment not clear wrt waht holding the thread achieve (or what threads are > involved)? Done. https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:142: private void handleOfflinePageBackgroundLoad(Context context, Bundle bundle) { On 2016/06/13 21:53:35, dougarnett wrote: > remove above annotation if private now Done. https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:143: Log.d(TAG, "handleOfflinePageBackgroundLoad"); On 2016/06/13 21:53:35, dougarnett wrote: > remove before landing? Done. https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:158: private void setWaitLatchIfNeeded(Bundle bundle) { On 2016/06/13 21:53:35, dougarnett wrote: > actually this method and the next one having javadoc is maybe more important > than the call sites having comments Done. https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:160: if (bundle != null && bundle.getLong(WAIT_NEEDED) != 0) { On 2016/06/13 21:53:35, dougarnett wrote: > why getLong()? Do you expect to plumb wait time value? If so, should be > documented. If not, suggest using putBoolean/getBoolean Added doc. There are no get/put Boolean methods for bundle. https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:169: boolean waitSucceeded = mLatch.await(4, TimeUnit.MINUTES); On 2016/06/13 21:53:35, dougarnett wrote: > maybe define constant for the 4 value Done. https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:181: mLatch.countDown(); On 2016/06/13 21:53:34, dougarnett wrote: > // Release the onRunTask thread to return so that the wakelock being hed for it > is cleared. ? Doc added. https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:204: private BackgroundOfflinerTask mBackgroundOfflinerTask; On 2016/06/13 21:53:35, dougarnett wrote: > members before methods for Java classes Done. https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java (right): https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:82: private ChromeBackgroundServiceWaiter mWaiter; On 2016/06/13 21:53:35, dougarnett wrote: > members before methods Done. https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java (right): https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:21: bundle.putLong(ChromeBackgroundService.WAIT_NEEDED, 1); On 2016/06/13 21:53:35, dougarnett wrote: > putBoolean There is no putBoolean in bundle. I kept this a long. I can make it short if you want, but then I'd have to cast the literals "(short)1" instead of "1".
Added doc looks really helpful. Want to try to get Bernard looped in on the review now? https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:160: if (bundle != null && bundle.getLong(WAIT_NEEDED) != 0) { On 2016/06/13 23:23:10, Pete Williamson wrote: > On 2016/06/13 21:53:35, dougarnett wrote: > > why getLong()? Do you expect to plumb wait time value? If so, should be > > documented. If not, suggest using putBoolean/getBoolean > > Added doc. There are no get/put Boolean methods for bundle. yes, there is putBoolean and getBoolean (2 forms for getBoolean, one with default value) https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java (right): https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:21: bundle.putLong(ChromeBackgroundService.WAIT_NEEDED, 1); On 2016/06/13 23:23:10, Pete Williamson wrote: > On 2016/06/13 21:53:35, dougarnett wrote: > > putBoolean > > There is no putBoolean in bundle. I kept this a long. I can make it short if > you want, but then I'd have to cast the literals "(short)1" instead of "1". https://developer.android.com/reference/android/os/BaseBundle.html#putBoolean..., boolean) https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:39: * start another task in place of the one just started. nice! https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:48: private static final int WAKELOCK_TIMEOUT = 4; WAKELOCK_TIMEOUT_MINUTES ?
petewil@chromium.org changed reviewers: + bauerb@chromium.org
BauerB: Please review changes to ChromeBackgroundService and the new interface for waiters. Thanks! (I'll address Doug's other comments tomorrow, but I was hoping you could start taking a look at this)
https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:170: private void setWaitLatchIfNeeded(Bundle bundle) { For testing, maybe make this and waitFor* visible for testing and set up onWaitDone() to be called from another thread. If ThreadUtils.runOnUiThread() works in the junit environment, that would be attractive as that is what is used in the code. Otherwise, some candidates might be creating a Thread or use an Executor or maybe even Android AsyncTask. https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:199: mLatch.countDown(); might want to be sure mLatch is not null here before countDown and then can set to null afterwards
https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:92: // If needed, block the GcmNetworkManager thread until the UI thread has finished its work. Just to check: you know that GcmNetworkManager is not using a thread pool for this and therefore might run a different task on another thread? https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiter.java (right): https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiter.java:12: public void onWaitDone(); Hm, just in terms of naming and comments, this seems very tied to ChromeBackgroundService. Do you need this be an interface (e.g. so you can mock it out in a test)? Otherwise you could just have clients use ChromeBackgroundService directly. Or if you do want this to be an interface, you could move it into ChromeBackgroundService. ChromeBackgroundService wouldn't even have to directly implement the interface, but could do that in an inner class that could hold the latch, so you wouldn't need to keep state in ChromeBackgroundService at all. https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java (right): https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:21: bundle.putLong(ChromeBackgroundService.HOLD_WAKELOCK, 1); Why not use a boolean?
Addressed feedback from BauerB and DougArnett https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:160: if (bundle != null && bundle.getLong(WAIT_NEEDED) != 0) { On 2016/06/14 01:42:45, dougarnett wrote: > On 2016/06/13 23:23:10, Pete Williamson wrote: > > On 2016/06/13 21:53:35, dougarnett wrote: > > > why getLong()? Do you expect to plumb wait time value? If so, should be > > > documented. If not, suggest using putBoolean/getBoolean > > > > Added doc. There are no get/put Boolean methods for bundle. > > yes, there is putBoolean and getBoolean (2 forms for getBoolean, one with > default value) Done. Huh, I didn't see it on the doc for Bundle. https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java (right): https://codereview.chromium.org/2066593003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:21: bundle.putLong(ChromeBackgroundService.WAIT_NEEDED, 1); On 2016/06/14 01:42:45, dougarnett wrote: > On 2016/06/13 23:23:10, Pete Williamson wrote: > > On 2016/06/13 21:53:35, dougarnett wrote: > > > putBoolean > > > > There is no putBoolean in bundle. I kept this a long. I can make it short if > > you want, but then I'd have to cast the literals "(short)1" instead of "1". > > https://developer.android.com/reference/android/os/BaseBundle.html#putBoolean..., > boolean) Done. https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:48: private static final int WAKELOCK_TIMEOUT = 4; On 2016/06/14 01:42:45, dougarnett wrote: > WAKELOCK_TIMEOUT_MINUTES ? Done. https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:92: // If needed, block the GcmNetworkManager thread until the UI thread has finished its work. On 2016/06/14 16:07:32, Bernhard Bauer wrote: > Just to check: you know that GcmNetworkManager is not using a thread pool for > this and therefore might run a different task on another thread? Done. (We now have different waiters on different incoming threads.) https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:170: private void setWaitLatchIfNeeded(Bundle bundle) { On 2016/06/14 16:01:20, dougarnett wrote: > For testing, maybe make this and waitFor* visible for testing and set up > onWaitDone() to be called from another thread. If ThreadUtils.runOnUiThread() > works in the junit environment, that would be attractive as that is what is used > in the code. Otherwise, some candidates might be creating a Thread or use an > Executor or maybe even Android AsyncTask. Done. https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:199: mLatch.countDown(); On 2016/06/14 16:01:20, dougarnett wrote: > might want to be sure mLatch is not null here before countDown and then can set > to null afterwards Done. https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiter.java (right): https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiter.java:12: public void onWaitDone(); On 2016/06/14 16:07:32, Bernhard Bauer wrote: > Hm, just in terms of naming and comments, this seems very tied to > ChromeBackgroundService. Do you need this be an interface (e.g. so you can mock > it out in a test)? Otherwise you could just have clients use > ChromeBackgroundService directly. > > Or if you do want this to be an interface, you could move it into > ChromeBackgroundService. ChromeBackgroundService wouldn't even have to directly > implement the interface, but could do that in an inner class that could hold the > latch, so you wouldn't need to keep state in ChromeBackgroundService at all. OK, changed the waiter from an interface to a class to encapsulate the wait, was that what you intended to suggest? https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java (right): https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:21: bundle.putLong(ChromeBackgroundService.HOLD_WAKELOCK, 1); On 2016/06/14 16:07:32, Bernhard Bauer wrote: > Why not use a boolean? Done. (Because I looked at the documentation for Bundle, not BundleBase for the method, and didn't see it).
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/2066593003/40001
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/2066593003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng 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/2066593003/60001
https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java (right): https://codereview.chromium.org/2066593003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:21: bundle.putLong(ChromeBackgroundService.HOLD_WAKELOCK, 1); On 2016/06/15 00:25:19, Pete Williamson wrote: > On 2016/06/14 16:07:32, Bernhard Bauer wrote: > > Why not use a boolean? > > Done. (Because I looked at the documentation for Bundle, not BundleBase for the > method, and didn't see it). Ah, yup. I made the same mistake initially :) https://codereview.chromium.org/2066593003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/2066593003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:51: private CountDownLatch mLatch; This is now unused, right? https://codereview.chromium.org/2066593003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiter.java (right): https://codereview.chromium.org/2066593003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiter.java:48: if (mLatch != null) { Uh, sorry to be contrary here, but at least with this approach, I would not reset the latch, but rather make the latch final (which implies creating it in the constructor), and basically make the class one-shot only -- that's how it's used anyway. It would also avoid multithreading issues (for example, right now you would be accessing and modifying |mLatch| on different threads, which might lead to a race condition).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2066593003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/2066593003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:51: private CountDownLatch mLatch; On 2016/06/15 15:24:29, Bernhard Bauer wrote: > This is now unused, right? Done. https://codereview.chromium.org/2066593003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiter.java (right): https://codereview.chromium.org/2066593003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiter.java:48: if (mLatch != null) { On 2016/06/15 15:24:29, Bernhard Bauer wrote: > Uh, sorry to be contrary here, but at least with this approach, I would not > reset the latch, but rather make the latch final (which implies creating it in > the constructor), and basically make the class one-shot only -- that's how it's > used anyway. It would also avoid multithreading issues (for example, right now > you would be accessing and modifying |mLatch| on different threads, which might > lead to a race condition). Done.
lgtm - with recommendation to drop waitForTaskIfNeeded() method https://codereview.chromium.org/2066593003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/2066593003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:187: public void waitForTaskIfNeeded(ChromeBackgroundServiceWaiter waiter) { Do you have tests written that call this? Not clear to me yet that it needs to be visible but maybe you already have a test using it. Otherwise, I wonder about not having this method and just putting this simple code above as it only adds a null check.
lgtm https://codereview.chromium.org/2066593003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiter.java (right): https://codereview.chromium.org/2066593003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiter.java:48: if (mLatch != null) { This check is unnecessary -- the latch will never be null.
https://codereview.chromium.org/2066593003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/2066593003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:187: public void waitForTaskIfNeeded(ChromeBackgroundServiceWaiter waiter) { On 2016/06/15 16:22:35, dougarnett wrote: > Do you have tests written that call this? > Not clear to me yet that it needs to be visible but maybe you already have a > test using it. Otherwise, I wonder about not having this method and just putting > this simple code above as it only adds a null check. I think that it's a matter of encapsulation. Yes, I know that it is small, but I think that getWaiter... and waitFor... work well together to put all the details of waiting out of the mainline. I'd prefer to leave this code as is. https://codereview.chromium.org/2066593003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiter.java (right): https://codereview.chromium.org/2066593003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundServiceWaiter.java:48: if (mLatch != null) { On 2016/06/15 16:32:45, Bernhard Bauer wrote: > This check is unnecessary -- the latch will never be null. Done.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, dougarnett@chromium.org Link to the patchset: https://codereview.chromium.org/2066593003/#ps100001 (title: "Last CR feedback from BauerB")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066593003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, dougarnett@chromium.org Link to the patchset: https://codereview.chromium.org/2066593003/#ps140001 (title: "merge fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066593003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, dougarnett@chromium.org Link to the patchset: https://codereview.chromium.org/2066593003/#ps160001 (title: "logical merge fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066593003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by petewil@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066593003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Add a general purpose facility for letting background tasks wait until the task is done before returning control to GCM Network Manager. To minimize concurrent network activity, the background offlining task wants to make sure we hold the thread for the GCM Network Manager until we are done using the network instead of letting the task start right away. The idea here is to reduce network contention during the Marshmallow maintenance window when the network becomes available. While this is only needed for BackgroundOfflining at the moment, we wanted to make the feature more generally accessible, any task with a bundle containing "WaitNeeded" set to anything other than 0 will start a wait. BUG=612325 patch from issue 1985923002 at patchset 200001 (http://crrev.com/1985923002#ps200001) ========== to ========== Add a general purpose facility for letting background tasks wait until the task is done before returning control to GCM Network Manager. To minimize concurrent network activity, the background offlining task wants to make sure we hold the thread for the GCM Network Manager until we are done using the network instead of letting the task start right away. The idea here is to reduce network contention during the Marshmallow maintenance window when the network becomes available. While this is only needed for BackgroundOfflining at the moment, we wanted to make the feature more generally accessible, any task with a bundle containing "WaitNeeded" set to anything other than 0 will start a wait. BUG=612325 patch from issue 1985923002 at patchset 200001 (http://crrev.com/1985923002#ps200001) Committed: https://crrev.com/71ef97e9c3815152a269b37b67990a7bbd641e1c Cr-Commit-Position: refs/heads/master@{#400180} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/71ef97e9c3815152a269b37b67990a7bbd641e1c Cr-Commit-Position: refs/heads/master@{#400180} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
