Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2030773002:
Add unit tests for the Background Scheduler. (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 unit tests for the Background Scheduler.
Adding the unit tests required a lot of related changes to be able to do dependency injection, and adding some stub classes. The SchedulerBridgeInterface allows us to stub out the BackgroundSchedulerBridge, and we added a shadow class for GcmNetworkManager.
This work is described at go/chrome-background-offlining.
BUG=612325
Committed: https://crrev.com/36efc4e2983393cb9a92fc35c7656a31e932da59
Cr-Commit-Position: refs/heads/master@{#398738}
Patch Set 1 #
Total comments: 25
Patch Set 2 : CR feedback per DougArnett #
Total comments: 7
Patch Set 3 : CR feedback per DougArnett and chili #
Total comments: 12
Patch Set 4 : CR feedback per DougArnett #
Total comments: 22
Patch Set 5 : Split out persistence and time changes. #Patch Set 6 : CR fixes per DougArnett #
Total comments: 6
Patch Set 7 : CR fixes per BauerB #
Total comments: 2
Patch Set 8 : Rename SchedulerRequestProcessor to BackgroundSchedulerProcessor #Patch Set 9 : Callback<Boolean> and BackgroundSchedulerProcessor #
Total comments: 4
Patch Set 10 : Address BauerB nits #Patch Set 11 : attempt to fix flaky shadow in test. #Patch Set 12 : Disable flaky test #Messages
Total messages: 68 (27 generated)
petewil@chromium.org changed reviewers: + dougarnett@chromium.org, jianli@chromium.org
Description was changed from ========== Add unit tests for the Background Scheduler. BUG=612325 ========== to ========== Add unit tests for the Background Scheduler. Adding the unit tests required a lot of related changes to be able to do dependency injection, and adding some stub classes. The SchedulerBridgeInterface allows us to stub out the BackgroundSchedulerBridge, and we added a shadow class for GcmNetworkManager. This work is described at go/chrome-background-offlining. BUG=612325 ==========
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/2030773002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2030773002/1
Few preliminary comments - I haven't gotten very far into it yet. https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:129: @VisibleForTesting ? but the following method is private so confused by this annotation https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:46: // TODO(petewil): Change back to private when UMA works in the test, and test So @VisibleForTesting here then? https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:47: // startBackgroundRequests insteasd of this method. spellign https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:45: public interface ProcessingDoneCallback { hmm, confusing to have same name for two interface definitions - hard to see connection/use https://codereview.chromium.org/2030773002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java:194: public void testBackgroundOfflinerLaunchBrowserWhenInstanceDoesNotExist() { Does this method name apply? Instance of what?
chili@chromium.org changed reviewers: + chili@chromium.org
Sorry for jumping in with comments. I'm trying to keep on top of changes for the internals page, and also using this as style guide exercise :) https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/SchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/SchedulerBridge.java:16: SchedulerBridgeInterface.ProcessingDoneCallback callback) { nit - this fits in previous line? https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridgeInterface.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridgeInterface.java:10: public abstract interface SchedulerBridgeInterface { why are we declaring an interface abstract? Aren't interfaces by definition abstract? Also, can we drop the Interface in SchedulerBridgeInterface? We declare it as an interface already, so that seems redundant (we can rename SchedulerBridge to SchedulerBridgeImpl?). Feel free to ignore if this is for consistency
https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:129: @VisibleForTesting On 2016/06/02 00:14:45, dougarnett wrote: > ? but the following method is private so confused by this annotation I meant to mark it as testable since the test now calls it. Not sure why it compiled, but I changed it to protected like the others. https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:46: // TODO(petewil): Change back to private when UMA works in the test, and test On 2016/06/02 00:14:45, dougarnett wrote: > So @VisibleForTesting here then? Done. https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:47: // startBackgroundRequests insteasd of this method. On 2016/06/02 00:14:45, dougarnett wrote: > spellign Done. https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:45: public interface ProcessingDoneCallback { On 2016/06/02 00:14:45, dougarnett wrote: > hmm, confusing to have same name for two interface definitions - hard to see > connection/use Agreed, I'm playing tricks with JNI. JNI needs to be able to see the definition to be able for the C++ code to call it. It turns out I can only include one XXX_jni.h file in the c++ code, so all the things needed to call into the bridge need to be contained in the bridge file. I made the declaration as identical as possible to the SchedulerBridgeInterface definition so that a call to this can act as a call to the other (duck typing). If you'd like me to put that in a comment, let me know. If you see a flaw in my reasoning, let me know. https://codereview.chromium.org/2030773002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java:194: public void testBackgroundOfflinerLaunchBrowserWhenInstanceDoesNotExist() { On 2016/06/02 00:14:45, dougarnett wrote: > Does this method name apply? Instance of what? In this case, the instance is of chrome. Notice that this follows the same convention as other tests in the file. When the ChromeBackgroundService is awoken, chrome might or might not be running. "WhenInstanceDoesNotExist" covers the case where chrome is not already running. "WhenInstanceExists" covers the case where chrome is already running.
few more comments from bouncy shuttle ... https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:45: public interface ProcessingDoneCallback { On 2016/06/02 00:42:06, Pete Williamson wrote: > On 2016/06/02 00:14:45, dougarnett wrote: > > hmm, confusing to have same name for two interface definitions - hard to see > > connection/use > > Agreed, I'm playing tricks with JNI. JNI needs to be able to see the definition > to be able for the C++ code to call it. It turns out I can only include one > XXX_jni.h file in the c++ code, so all the things needed to call into the bridge > need to be contained in the bridge file. I made the declaration as identical as > possible to the SchedulerBridgeInterface definition so that a call to this can > act as a call to the other (duck typing). > > If you'd like me to put that in a comment, let me know. If you see a flaw in my > reasoning, let me know. Yes, please try to document here. https://codereview.chromium.org/2030773002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java:194: public void testBackgroundOfflinerLaunchBrowserWhenInstanceDoesNotExist() { On 2016/06/02 00:42:06, Pete Williamson wrote: > On 2016/06/02 00:14:45, dougarnett wrote: > > Does this method name apply? Instance of what? > > In this case, the instance is of chrome. Notice that this follows the same > convention as other tests in the file. When the ChromeBackgroundService is > awoken, chrome might or might not be running. "WhenInstanceDoesNotExist" covers > the case where chrome is not already running. "WhenInstanceExists" covers the > case where chrome is already running. Yeah, the other examples I looked at were deleting domain specific instances (eg, deletePrecacheInstance()) so that threw me initially. https://codereview.chromium.org/2030773002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:48: task.onProcessingDone(true); why calling done callback first? https://codereview.chromium.org/2030773002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:57: public void testOnProcessingDone() { TODO here or instead since this isn't actually testing anything yet https://codereview.chromium.org/2030773002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:45: // Also assert that the date we see is what we expected TODO? https://codereview.chromium.org/2030773002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:49: @Feature({"OfflinePages"}) TODO instead here too https://codereview.chromium.org/2030773002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java (right): https://codereview.chromium.org/2030773002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java:19: public class ShadowGcmNetworkManager { can include this shadow with BackgroundSchedulerTest if that is all it will be used for. https://codereview.chromium.org/2030773002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubOfflinePageUtils.java (right): https://codereview.chromium.org/2030773002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubOfflinePageUtils.java:12: public class StubOfflinePageUtils extends OfflinePageUtils { also one to consider locating with test class that uses - can be better context for robolectric test
I removed the ChromeBackgroundService test, which seems to be failing when I call SystemClock.elapsedRealtime() when the test runs on the device while I investigate (I intend to reintroduce it in another changelist once I find the problem). https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:45: public interface ProcessingDoneCallback { On 2016/06/02 16:31:47, dougarnett wrote: > On 2016/06/02 00:42:06, Pete Williamson wrote: > > On 2016/06/02 00:14:45, dougarnett wrote: > > > hmm, confusing to have same name for two interface definitions - hard to see > > > connection/use > > > > Agreed, I'm playing tricks with JNI. JNI needs to be able to see the > definition > > to be able for the C++ code to call it. It turns out I can only include one > > XXX_jni.h file in the c++ code, so all the things needed to call into the > bridge > > need to be contained in the bridge file. I made the declaration as identical > as > > possible to the SchedulerBridgeInterface definition so that a call to this can > > act as a call to the other (duck typing). > > > > If you'd like me to put that in a comment, let me know. If you see a flaw in > my > > reasoning, let me know. > > Yes, please try to document here. Comment expanded. https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/SchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/SchedulerBridge.java:16: SchedulerBridgeInterface.ProcessingDoneCallback callback) { On 2016/06/02 00:37:25, chili wrote: > nit - this fits in previous line? Yep, it does. Fixed (this function got a lot of name changes...) https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridgeInterface.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridgeInterface.java:10: public abstract interface SchedulerBridgeInterface { On 2016/06/02 00:37:25, chili wrote: > why are we declaring an interface abstract? Aren't interfaces by definition > abstract? > > Also, can we drop the Interface in SchedulerBridgeInterface? We declare it as > an interface already, so that seems redundant (we can rename SchedulerBridge to > SchedulerBridgeImpl?). Feel free to ignore if this is for consistency Done. https://codereview.chromium.org/2030773002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:48: task.onProcessingDone(true); On 2016/06/02 16:31:47, dougarnett wrote: > why calling done callback first? Originally it was to release the latch and let the call proceed. The CountDownLatch moved to another changelist, but this stayed here, and at least makes sure onProcessingDone() gets called from the tests. I can remove it if you like (but it may have to come back later) https://codereview.chromium.org/2030773002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:57: public void testOnProcessingDone() { On 2016/06/02 16:31:47, dougarnett wrote: > TODO here or instead since this isn't actually testing anything yet Done. https://codereview.chromium.org/2030773002/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java (right): https://codereview.chromium.org/2030773002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:45: // Also assert that the date we see is what we expected On 2016/06/02 16:31:47, dougarnett wrote: > TODO? TODO added https://codereview.chromium.org/2030773002/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerTest.java:49: @Feature({"OfflinePages"}) On 2016/06/02 16:31:47, dougarnett wrote: > TODO instead here too Done. https://codereview.chromium.org/2030773002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java (right): https://codereview.chromium.org/2030773002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java:19: public class ShadowGcmNetworkManager { On 2016/06/02 16:31:47, dougarnett wrote: > can include this shadow with BackgroundSchedulerTest if that is all it will be > used for. In past experience, it is good to keep shadows in their own file, they end up getting used in several places. Also, we are supposed to have only one class per java file per coding standards, and I'm not sure this would work as a shadow if it is an embedded class. https://codereview.chromium.org/2030773002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubOfflinePageUtils.java (right): https://codereview.chromium.org/2030773002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubOfflinePageUtils.java:12: public class StubOfflinePageUtils extends OfflinePageUtils { On 2016/06/02 16:31:47, dougarnett wrote: > also one to consider locating with test class that uses - can be better context > for robolectric test As before, I like keeping classes in their own file, and we may be reusing this someday
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/2030773002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2030773002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2030773002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java (right): https://codereview.chromium.org/2030773002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java:19: public class ShadowGcmNetworkManager { On 2016/06/02 20:51:33, Pete Williamson wrote: > On 2016/06/02 16:31:47, dougarnett wrote: > > can include this shadow with BackgroundSchedulerTest if that is all it will be > > used for. > > In past experience, it is good to keep shadows in their own file, they end up > getting used in several places. Also, we are supposed to have only one class > per java file per coding standards, and I'm not sure this would work as a shadow > if it is an embedded class. Generally sounds good if we expect to reuse in other tests but not clear to me in this GcmNetworkManager case and also not clear to me if it is in the right directory for sharing to other uses. Also, I don't think the coding standard is prohibitive in this situation - but might be looking at wrong place - and I do find examples that use inner classes for shadows. From a quick check of robolectric shadow examples in chromium code base, I found these tests with inner shadow classes (including an OfflinePage test): - base/android/junit/src/org/chromium/base/BaseChromiumApplicationTest.java - chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java - chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java - chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java - net/android/junit/src/org/chromium/net/HttpNegotiateAuthenticatorTest.java And these examples of stand-alone shadows that appear to be in "test" or "testing" support packages: - base/test/android/junit/src/org/chromium/base/test/shadows/ShadowMultiDex.java - testing/android/junit/java/src/org/chromium/testing/local/BackgroundShadowAsyncTask.java - testing/android/junit/java/src/org/chromium/testing/local/CustomShadowAsyncTask.java So I still suggest you consider inner class for this one (as I think it is less conceptual baggage to find and relate) - until we have another use and then break it out and determine what shareable path makes sense for it. https://codereview.chromium.org/2030773002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubOfflinePageUtils.java (right): https://codereview.chromium.org/2030773002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubOfflinePageUtils.java:12: public class StubOfflinePageUtils extends OfflinePageUtils { On 2016/06/02 20:51:33, Pete Williamson wrote: > On 2016/06/02 16:31:47, dougarnett wrote: > > also one to consider locating with test class that uses - can be better > context > > for robolectric test > > As before, I like keeping classes in their own file, and we may be reusing this > someday Ok, I retract this one - it does seems more likely to be reused and also not constrained to robolectric. https://codereview.chromium.org/2030773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:43: * Callback used to determine when request processing is done. Note that this is the same as Let me try some alternative wording (which will at least reveal if I am understanding this): This interface defines the JNI callback available to native code. It must exactly match the {@link SchedulerBridge.ProcessingDoneCallback} interface (which is not tied to JNI and may be use for unit testing). ... <possibly more about single XXX_jni.h file and duck typing> https://codereview.chromium.org/2030773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/SchedulerBridgeImpl.java (right): https://codereview.chromium.org/2030773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/SchedulerBridgeImpl.java:10: * Interface to allow mocking out our BackgroundSchedulerBridge. fix comment https://codereview.chromium.org/2030773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java:15: public abstract boolean startProcessing( drop "public abstract" here https://codereview.chromium.org/2030773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java:22: void onProcessingDone(boolean result); javadoc for method? https://codereview.chromium.org/2030773002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java (right): https://codereview.chromium.org/2030773002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:37: Date now = new Date(); Why not System.currentTimeMillis() rather than Date? https://codereview.chromium.org/2030773002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:48: task.onProcessingDone(true); Please add comment for this line if you keep it as it is confusing.
Addressed most of DougArnett's feedback, we are still working on whether we need a new class for the ShadowGcmNetworkManager, or should use an embedded class. I also fixed a problem I discovered while testing in ChromeBackgroundService - We should be using LoadLibrary instead of hasPrecacheInstance - the hasPrecacheInstance function reports whether we have an instance of the precacher, which is not quite the same as if chrome has been launched. While I converted over the time to use System.currentTimeMillis, I also moved the time packing code into a separate class as requested by BauerB in the previous code review, so that is a new file with this version of the patch. https://codereview.chromium.org/2030773002/diff/20001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java (right): https://codereview.chromium.org/2030773002/diff/20001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java:19: public class ShadowGcmNetworkManager { On 2016/06/03 16:57:59, dougarnett wrote: > On 2016/06/02 20:51:33, Pete Williamson wrote: > > On 2016/06/02 16:31:47, dougarnett wrote: > > > can include this shadow with BackgroundSchedulerTest if that is all it will > be > > > used for. > > > > In past experience, it is good to keep shadows in their own file, they end up > > getting used in several places. Also, we are supposed to have only one class > > per java file per coding standards, and I'm not sure this would work as a > shadow > > if it is an embedded class. > > Generally sounds good if we expect to reuse in other tests but not clear to me > in this GcmNetworkManager case and also not clear to me if it is in the right > directory for sharing to other uses. Also, I don't think the coding standard is > prohibitive in this situation - but might be looking at wrong place - and I do > find examples that use inner classes for shadows. > > From a quick check of robolectric shadow examples in chromium code base, I found > these tests with inner shadow classes (including an OfflinePage test): > - base/android/junit/src/org/chromium/base/BaseChromiumApplicationTest.java > - > chrome/android/junit/src/org/chromium/chrome/browser/media/remote/AbstractMediaRouteControllerTest.java > - > chrome/android/junit/src/org/chromium/chrome/browser/media/router/ChromeMediaRouterTestBase.java > - > chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtilsTest.java > - net/android/junit/src/org/chromium/net/HttpNegotiateAuthenticatorTest.java > > And these examples of stand-alone shadows that appear to be in "test" or > "testing" support packages: > - > base/test/android/junit/src/org/chromium/base/test/shadows/ShadowMultiDex.java > - > testing/android/junit/java/src/org/chromium/testing/local/BackgroundShadowAsyncTask.java > - > testing/android/junit/java/src/org/chromium/testing/local/CustomShadowAsyncTask.java > > So I still suggest you consider inner class for this one (as I think it is less > conceptual baggage to find and relate) - until we have another use and then > break it out and determine what shareable path makes sense for it. I'd really prefer to keep this as a separate class. Let's take the discussion offline, and I'll update the code to whatever we agree upon. https://codereview.chromium.org/2030773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:43: * Callback used to determine when request processing is done. Note that this is the same as On 2016/06/03 16:58:00, dougarnett wrote: > Let me try some alternative wording (which will at least reveal if I am > understanding this): > > This interface defines the JNI callback available to native code. It must > exactly match the {@link SchedulerBridge.ProcessingDoneCallback} interface > (which is not tied to JNI and may be use for unit testing). ... <possibly more > about single XXX_jni.h file and duck typing> OK, Check current wording. Your understanding is correct. https://codereview.chromium.org/2030773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/SchedulerBridgeImpl.java (right): https://codereview.chromium.org/2030773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/SchedulerBridgeImpl.java:10: * Interface to allow mocking out our BackgroundSchedulerBridge. On 2016/06/03 16:58:00, dougarnett wrote: > fix comment Done. https://codereview.chromium.org/2030773002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java:15: public abstract boolean startProcessing( On 2016/06/03 16:58:00, dougarnett wrote: > drop "public abstract" here Done. https://codereview.chromium.org/2030773002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java:22: void onProcessingDone(boolean result); On 2016/06/03 16:58:00, dougarnett wrote: > javadoc for method? Done. https://codereview.chromium.org/2030773002/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java (right): https://codereview.chromium.org/2030773002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:37: Date now = new Date(); On 2016/06/03 16:58:00, dougarnett wrote: > Why not System.currentTimeMillis() rather than Date? Done. https://codereview.chromium.org/2030773002/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:48: task.onProcessingDone(true); On 2016/06/03 16:58:00, dougarnett wrote: > Please add comment for this line if you keep it as it is confusing. Done.
dewittj@chromium.org changed reviewers: + dewittj@chromium.org
drive-by! https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:43: .setPersisted(true) This should be done in a separate CL - not related to unit testing, right? https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:230: long milliNow = System.currentTimeMillis(); Same here, unrelated to tests so should be in a different cl. https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java:16: SchedulerBridge.ProcessingDoneCallback callback); nit: I think you can omit the SchedulerBridge. here. https://codereview.chromium.org/2030773002/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java:5: package org.chromium.chrome.browser.offlinepages; This seems more global than just an offline pages class, probably a chrome/browser/android reviewer can comment more here. https://codereview.chromium.org/2030773002/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java:20: private static final String TAG = "ShadowGcmNetworkManager"; This seems unused. https://codereview.chromium.org/2030773002/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubOfflinePageUtils.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubOfflinePageUtils.java:12: public class StubOfflinePageUtils extends OfflinePageUtils { Is this class used?
chili@chromium.org changed reviewers: - chili@chromium.org
https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java:12: // started and that caller should expect a callback (once processing has completed or should this be in javadoc instead?
Just some naming suggestions https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:39: long millisTaskStart = TaskExtrasPacker.unpackTimeFromBundle(bundle); usually see Millis as suffix. Also, isn't this the scheduled time? ie, taskScheduledTimeMillis? https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:208: public static void recordWakeupUMA(Context context, long millisTaskStart) { taskScheduledTimeMillis? https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:230: long milliNow = System.currentTimeMillis(); nowMillis https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:230: long milliNow = System.currentTimeMillis(); On 2016/06/03 21:21:49, dewittj wrote: > Same here, unrelated to tests so should be in a different cl. fwiw, I am happy with this followup here if you want to update the patch description to cover it (vs. separate patch) https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:14: public static final String DATE_TAG = "Date"; We may want to end up naming with more meaningful name like SCHEDULED_TIME_TAG or REQUESTED_TIME_TAG
CR feedback per DewittJ and Chili. (None for DougArnett yet) https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundScheduler.java:43: .setPersisted(true) On 2016/06/03 21:21:49, dewittj wrote: > This should be done in a separate CL - not related to unit testing, right? Done. https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:230: long milliNow = System.currentTimeMillis(); On 2016/06/03 21:21:49, dewittj wrote: > Same here, unrelated to tests so should be in a different cl. Done. https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java:12: // started and that caller should expect a callback (once processing has completed or On 2016/06/03 21:24:17, chili wrote: > should this be in javadoc instead? Done. https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java:16: SchedulerBridge.ProcessingDoneCallback callback); On 2016/06/03 21:21:49, dewittj wrote: > nit: I think you can omit the SchedulerBridge. here. Done. https://codereview.chromium.org/2030773002/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/ShadowGcmNetworkManager.java:20: private static final String TAG = "ShadowGcmNetworkManager"; On 2016/06/03 21:21:49, dewittj wrote: > This seems unused. Done. https://codereview.chromium.org/2030773002/diff/60001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubOfflinePageUtils.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/StubOfflinePageUtils.java:12: public class StubOfflinePageUtils extends OfflinePageUtils { On 2016/06/03 21:21:49, dewittj wrote: > Is this class used? Leftover from earlier attempt. Removed.
CR fixes per DougArnett https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTask.java:39: long millisTaskStart = TaskExtrasPacker.unpackTimeFromBundle(bundle); On 2016/06/03 21:51:53, dougarnett wrote: > usually see Millis as suffix. Also, isn't this the scheduled time? ie, > taskScheduledTimeMillis? Done. https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:208: public static void recordWakeupUMA(Context context, long millisTaskStart) { On 2016/06/03 21:51:53, dougarnett wrote: > taskScheduledTimeMillis? Done. https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:230: long milliNow = System.currentTimeMillis(); On 2016/06/03 21:51:53, dougarnett wrote: > nowMillis Done. https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java (right): https://codereview.chromium.org/2030773002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/TaskExtrasPacker.java:14: public static final String DATE_TAG = "Date"; On 2016/06/03 21:51:53, dougarnett wrote: > We may want to end up naming with more meaningful name like SCHEDULED_TIME_TAG > or REQUESTED_TIME_TAG Done.
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/2030773002/100001
lgtm
petewil@chromium.org changed reviewers: + bauerb@google.com, chili@chromium.org - dewittj@chromium.org, jianli@chromium.org
BauerB: Please review changes to ChromeBackgroundScheduler.java and how we set up the BackgroundSchedulerBridge for testing. We used a SchedulerBridge interface, and a concrete SchedulerBridgeImpl class to call out to the static BackgroundSchedulerBridge JNI interface. We did this to allow unit testing BackgroundOfflierTask without having to call across the JNI bridge while testing. This also adds the TaskExtrasPacker that you asked for during the previous changelist.(https://codereview.chromium.org/2030773002) To make the classes testable, I also made BackgroundOfflinerTask a non-static, so we need an instance of it now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bauerb@chromium.org changed reviewers: + bauerb@chromium.org - bauerb@google.com
https://codereview.chromium.org/2030773002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:52: @CalledByNative("ProcessingDoneCallback") So, the "proper" way to do this would be to add this annotation on the method on the real Callback, and add a new pair of C++ files to go with it. All these files would do is expose a static registration method and a static method to run the callback. Then you could include the header from anywhere you wanted run the callback from there. It's a bit more overhead, but way less brittle. You can look at base/android/callback_android.{cc,h} for an example. Actually, you might be able to use these directly without adding your own callback class! Looking at the CL that introduced it (https://codereview.chromium.org/1996193003), that might be preferred anyway. https://codereview.chromium.org/2030773002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java:10: public interface SchedulerBridge { I'm not super happy with this name, because it's not really clear just from looking at the name what the difference between a SchedulerBridge and a BackgroundSchedulerBridge is. One would be tempted to think that the latter only schedules things in the background, but that's obviously not the case. Maybe it would be better to name this interface after what it's supposed to do (and not the fact that the real implementation bridges to C++), so... RequestProcessor?
https://codereview.chromium.org/2030773002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/BackgroundSchedulerBridge.java:52: @CalledByNative("ProcessingDoneCallback") On 2016/06/06 15:43:32, Bernhard Bauer wrote: > So, the "proper" way to do this would be to add this annotation on the method on > the real Callback, and add a new pair of C++ files to go with it. All these > files would do is expose a static registration method and a static method to run > the callback. Then you could include the header from anywhere you wanted run the > callback from there. It's a bit more overhead, but way less brittle. You can > look at base/android/callback_android.{cc,h} for an example. > > Actually, you might be able to use these directly without adding your own > callback class! Looking at the CL that introduced it > (https://codereview.chromium.org/1996193003), that might be preferred anyway. Done. https://codereview.chromium.org/2030773002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java:10: public interface SchedulerBridge { On 2016/06/06 15:43:32, Bernhard Bauer wrote: > I'm not super happy with this name, because it's not really clear just from > looking at the name what the difference between a SchedulerBridge and a > BackgroundSchedulerBridge is. One would be tempted to think that the latter only > schedules things in the background, but that's obviously not the case. > > Maybe it would be better to name this interface after what it's supposed to do > (and not the fact that the real implementation bridges to C++), so... > RequestProcessor? Done. New name is SchedulerRequestProcessor (RequestProcessor seemed a bit too generic).
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/2030773002/120001
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...)
https://codereview.chromium.org/2030773002/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/background_scheduler_bridge.cc (right): https://codereview.chromium.org/2030773002/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/background_scheduler_bridge.cc:25: base::android::RunCallbackAndroid(j_callback_obj, result); Wait, does this work when |j_callback_obj| is a ProcessingDoneCallback? I think you're going to have to actually use org.chromium.base.Callback as the callback type here.
https://codereview.chromium.org/2030773002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java:10: public interface SchedulerBridge { On 2016/06/07 00:20:22, Pete Williamson wrote: > On 2016/06/06 15:43:32, Bernhard Bauer wrote: > > I'm not super happy with this name, because it's not really clear just from > > looking at the name what the difference between a SchedulerBridge and a > > BackgroundSchedulerBridge is. One would be tempted to think that the latter > only > > schedules things in the background, but that's obviously not the case. > > > > Maybe it would be better to name this interface after what it's supposed to do > > (and not the fact that the real implementation bridges to C++), so... > > RequestProcessor? > > Done. New name is SchedulerRequestProcessor (RequestProcessor seemed a bit too > generic). hmm, think BackgroundRequestProcessor would be better
Renamed the SchedulerRequestProcessor to BackgroundSchedulerProcessor per DougArnett. Replaced the ProcessingDoneCallback on the Java side with Callback<Boolean> everywhere. (Also replaced ProcessingDoneCallback in C++ with Callback<boolean> in a few places) https://codereview.chromium.org/2030773002/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/background_scheduler_bridge.cc (right): https://codereview.chromium.org/2030773002/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/background_scheduler_bridge.cc:25: base::android::RunCallbackAndroid(j_callback_obj, result); On 2016/06/07 08:43:42, Bernhard Bauer wrote: > Wait, does this work when |j_callback_obj| is a ProcessingDoneCallback? I think > you're going to have to actually use org.chromium.base.Callback as the callback > type here. Ah, good catch. I was thinking that since the argument and return type signatures matched that they would get seamlessly mapped, but looking at other examples shows that isn't so. I replaced the Java side ProcessingDoneCallback completely with Callback<Boolean>
https://codereview.chromium.org/2030773002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java (right): https://codereview.chromium.org/2030773002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java:10: public interface SchedulerBridge { On 2016/06/07 15:57:25, dougarnett wrote: > On 2016/06/07 00:20:22, Pete Williamson wrote: > > On 2016/06/06 15:43:32, Bernhard Bauer wrote: > > > I'm not super happy with this name, because it's not really clear just from > > > looking at the name what the difference between a SchedulerBridge and a > > > BackgroundSchedulerBridge is. One would be tempted to think that the latter > > only > > > schedules things in the background, but that's obviously not the case. > > > > > > Maybe it would be better to name this interface after what it's supposed to > do > > > (and not the fact that the real implementation bridges to C++), so... > > > RequestProcessor? > > > > Done. New name is SchedulerRequestProcessor (RequestProcessor seemed a bit > too > > generic). > > hmm, think BackgroundRequestProcessor would be better After a chat with Doug, we decided on "BackgroundSchedulerProcessor"
On 2016/06/07 22:10:34, Pete Williamson wrote: > https://codereview.chromium.org/2030773002/diff/100001/chrome/android/java/sr... > File > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java > (right): > > https://codereview.chromium.org/2030773002/diff/100001/chrome/android/java/sr... > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/interfaces/SchedulerBridge.java:10: > public interface SchedulerBridge { > On 2016/06/07 15:57:25, dougarnett wrote: > > On 2016/06/07 00:20:22, Pete Williamson wrote: > > > On 2016/06/06 15:43:32, Bernhard Bauer wrote: > > > > I'm not super happy with this name, because it's not really clear just > from > > > > looking at the name what the difference between a SchedulerBridge and a > > > > BackgroundSchedulerBridge is. One would be tempted to think that the > latter > > > only > > > > schedules things in the background, but that's obviously not the case. > > > > > > > > Maybe it would be better to name this interface after what it's supposed > to > > do > > > > (and not the fact that the real implementation bridges to C++), so... > > > > RequestProcessor? > > > > > > Done. New name is SchedulerRequestProcessor (RequestProcessor seemed a bit > > too > > > generic). > > > > hmm, think BackgroundRequestProcessor would be better > > After a chat with Doug, we decided on "BackgroundSchedulerProcessor" Definitely nicer with Callback<Boolean>
LGTM, just some small stuff: https://codereview.chromium.org/2030773002/diff/160001/components/offline_pag... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2030773002/diff/160001/components/offline_pag... components/offline_pages/background/request_coordinator.h:32: typedef base::Callback<void()> ProcessingDoneCallback; Is this now used anymore? https://codereview.chromium.org/2030773002/diff/160001/components/offline_pag... components/offline_pages/background/request_coordinator.h:51: bool StartProcessing(const base::Callback<void(bool)> callback); I would still pass the callback by (const) reference though.
https://codereview.chromium.org/2030773002/diff/160001/components/offline_pag... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2030773002/diff/160001/components/offline_pag... components/offline_pages/background/request_coordinator.h:32: typedef base::Callback<void()> ProcessingDoneCallback; On 2016/06/08 14:09:29, Bernhard Bauer wrote: > Is this now used anymore? Removed https://codereview.chromium.org/2030773002/diff/160001/components/offline_pag... components/offline_pages/background/request_coordinator.h:51: bool StartProcessing(const base::Callback<void(bool)> callback); On 2016/06/08 14:09:29, Bernhard Bauer wrote: > I would still pass the callback by (const) reference though. Done.
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, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2030773002/#ps180001 (title: "Address BauerB nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030773002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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/2030773002/180001
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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/2030773002/#ps200001 (title: "attempt to fix flaky shadow in test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030773002/200001
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/2030773002/#ps220001 (title: "Disable flaky test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2030773002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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/2030773002/220001
Message was sent while issue was closed.
Description was changed from ========== Add unit tests for the Background Scheduler. Adding the unit tests required a lot of related changes to be able to do dependency injection, and adding some stub classes. The SchedulerBridgeInterface allows us to stub out the BackgroundSchedulerBridge, and we added a shadow class for GcmNetworkManager. This work is described at go/chrome-background-offlining. BUG=612325 ========== to ========== Add unit tests for the Background Scheduler. Adding the unit tests required a lot of related changes to be able to do dependency injection, and adding some stub classes. The SchedulerBridgeInterface allows us to stub out the BackgroundSchedulerBridge, and we added a shadow class for GcmNetworkManager. This work is described at go/chrome-background-offlining. BUG=612325 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add unit tests for the Background Scheduler. Adding the unit tests required a lot of related changes to be able to do dependency injection, and adding some stub classes. The SchedulerBridgeInterface allows us to stub out the BackgroundSchedulerBridge, and we added a shadow class for GcmNetworkManager. This work is described at go/chrome-background-offlining. BUG=612325 ========== to ========== Add unit tests for the Background Scheduler. Adding the unit tests required a lot of related changes to be able to do dependency injection, and adding some stub classes. The SchedulerBridgeInterface allows us to stub out the BackgroundSchedulerBridge, and we added a shadow class for GcmNetworkManager. This work is described at go/chrome-background-offlining. BUG=612325 Committed: https://crrev.com/36efc4e2983393cb9a92fc35c7656a31e932da59 Cr-Commit-Position: refs/heads/master@{#398738} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/36efc4e2983393cb9a92fc35c7656a31e932da59 Cr-Commit-Position: refs/heads/master@{#398738} |
