Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2064793003:
Remove old requests from queue, start new requests if any, add test. (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. |
DescriptionRemove old requests from queue, start new requests if any, add test.
When offline page requests are completed, remove them from the queue of outstanding requests, and start the next request (if any)
This changelist also adds a test for the new code.
BUG=610521
Committed: https://crrev.com/ec96ac8f3893d0fbea151a1750ff15df5b5f8b9f
Cr-Commit-Position: refs/heads/master@{#399914}
Patch Set 1 #
Total comments: 10
Patch Set 2 : CR feedback per DewittJ and DougArnett #Patch Set 3 : Update comment #
Total comments: 2
Patch Set 4 : Fix unit test #
Messages
Total messages: 25 (11 generated)
petewil@chromium.org changed reviewers: + chili@chromium.org, dougarnett@chromium.org
dewittj@chromium.org changed reviewers: + dewittj@chromium.org
Please make the cl description a little more understandable for sheriffs. https://codereview.chromium.org/2064793003/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2064793003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:60: void OfflinerDoneCallback(const SavePageRequest& request, nit: make this private and make the test harness a friend class.
Description was changed from ========== Remove old requests from queue, start new requests if any, add test. BUG=610521 ========== to ========== Remove old requests from queue, start new requests if any, add test. When offline page requests are completed, remove them from the queue of outstanding requests, and start the next request (if any) This changelist also adds a test for the new code. BUG=610521 ==========
https://codereview.chromium.org/2064793003/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2064793003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:72: RequestQueue::UpdateRequestResult result) {} Where do we re-schedule and/or unschedule depending on any more items in queue? https://codereview.chromium.org/2064793003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:86: scheduler_callback_.Run(true); unschedule() here too? https://codereview.chromium.org/2064793003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:90: const base::Callback<void(bool)> callback) { couldn't this still be a reference here if copying below anyway? https://codereview.chromium.org/2064793003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:97: return false; should return true if caller should expect a callback, right? Consider TODO to check if already running, then return false. We might also return false for above TODO if we can do a synchronous check on device conditions.
https://codereview.chromium.org/2064793003/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2064793003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:72: RequestQueue::UpdateRequestResult result) {} On 2016/06/14 18:12:52, dougarnett wrote: > Where do we re-schedule and/or unschedule depending on any more items in queue? OfflinerDoneCallback calls TryNextRequest() to schedule the next item in the queue. The proper place to unschedule is RequestQueueEmpty(). https://codereview.chromium.org/2064793003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:86: scheduler_callback_.Run(true); On 2016/06/14 18:12:52, dougarnett wrote: > unschedule() here too? Actually, this is the main place to add unschedule. Added (now that unschedule has landed). https://codereview.chromium.org/2064793003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:90: const base::Callback<void(bool)> callback) { On 2016/06/14 18:12:52, dougarnett wrote: > couldn't this still be a reference here if copying below anyway? Done. https://codereview.chromium.org/2064793003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:97: return false; On 2016/06/14 18:12:52, dougarnett wrote: > should return true if caller should expect a callback, right? > > Consider TODO to check if already running, then return false. We might also > return false for above TODO if we can do a synchronous check on device > conditions. Done. https://codereview.chromium.org/2064793003/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2064793003/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:60: void OfflinerDoneCallback(const SavePageRequest& request, On 2016/06/14 16:16:36, dewittj wrote: > nit: make this private and make the test harness a friend class. Done.
lgtm lgtm with comment to fix test return value check from StartProcessing() with other thoughts optional https://codereview.chromium.org/2064793003/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2064793003/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:172: EXPECT_FALSE(coordinator()->StartProcessing(callback)); EXPECT_TRUE at this point since don't know if there are any requests synchronously - so caller should expect callback. Future thoughts: ideally there is a bit more should be verified here wrt request queue query happening (or ultimately processing done callback gets called). Also when you add support for single active request, this maybe forks into two tests - where one does expect false.
https://codereview.chromium.org/2064793003/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2064793003/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:172: EXPECT_FALSE(coordinator()->StartProcessing(callback)); On 2016/06/14 22:53:33, dougarnett wrote: > EXPECT_TRUE at this point since don't know if there are any requests > synchronously - so caller should expect callback. > > Future thoughts: ideally there is a bit more should be verified here wrt request > queue query happening (or ultimately processing done callback gets called). Also > when you add support for single active request, this maybe forks into two tests > - where one does expect false. Changed expectations. Saving future thoughts for when we enforce a single request.
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 Link to the patchset: https://codereview.chromium.org/2064793003/#ps60001 (title: "Fix unit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2064793003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
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/2064793003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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/2064793003/60001
Message was sent while issue was closed.
Description was changed from ========== Remove old requests from queue, start new requests if any, add test. When offline page requests are completed, remove them from the queue of outstanding requests, and start the next request (if any) This changelist also adds a test for the new code. BUG=610521 ========== to ========== Remove old requests from queue, start new requests if any, add test. When offline page requests are completed, remove them from the queue of outstanding requests, and start the next request (if any) This changelist also adds a test for the new code. BUG=610521 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Remove old requests from queue, start new requests if any, add test. When offline page requests are completed, remove them from the queue of outstanding requests, and start the next request (if any) This changelist also adds a test for the new code. BUG=610521 ========== to ========== Remove old requests from queue, start new requests if any, add test. When offline page requests are completed, remove them from the queue of outstanding requests, and start the next request (if any) This changelist also adds a test for the new code. BUG=610521 Committed: https://crrev.com/ec96ac8f3893d0fbea151a1750ff15df5b5f8b9f Cr-Commit-Position: refs/heads/master@{#399914} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ec96ac8f3893d0fbea151a1750ff15df5b5f8b9f Cr-Commit-Position: refs/heads/master@{#399914} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
