Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2233493003:
[Offline Pages] Remove expired requests from the queue. (Closed)
|
Created:
4 years, 4 months ago by romax Modified:
4 years, 4 months ago Reviewers:
Pete Williamson 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. |
Description[Offline Pages] Remove expired requests from the queue.
Added logic to remove expired requests from the queue when picking next
requests. Also added a placeholder for notification through coordinator.
BUG=636185
Committed: https://crrev.com/91f93ff1742c76686413531ec1f6a5214b833bd5
Cr-Commit-Position: refs/heads/master@{#412926}
Patch Set 1 #Patch Set 2 : Fixing patching error. #
Total comments: 6
Patch Set 3 : fixing build. #Patch Set 4 : Merging. #
Total comments: 6
Patch Set 5 : Pulling out the notifier as an interface. #
Total comments: 6
Patch Set 6 : Comments from Pete. #Patch Set 7 : fixing build. #
Messages
Total messages: 39 (26 generated)
romax@chromium.org changed reviewers: + petewil@chromium.org
PTAL thanks!
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Overall looks good. It needs to merge with a couple of changelists that are still in progress (https://codereview.chromium.org/2228813003 and https://codereview.chromium.org/2219393004) https://codereview.chromium.org/2233493003/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2233493003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:85: virtual void NotifyRequestsExpired( We will use the existing NotifyFailed for this. No need to hook it up until the observer patch lands. https://codereview.chromium.org/2233493003/diff/20001/components/offline_page... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2233493003/diff/20001/components/offline_page... components/offline_pages/background/request_picker.cc:258: coordinator_->NotifyRequestsExpired(expired_requests, result); When the observer changelist lands, this should be changed to call NotifyFailed for each request. https://codereview.chromium.org/2233493003/diff/20001/components/offline_page... File components/offline_pages/background/request_queue.h (right): https://codereview.chromium.org/2233493003/diff/20001/components/offline_page... components/offline_pages/background/request_queue.h:81: void RemoveRequestsById(std::vector<int64_t> request_ids, You can remove this method. RemoveRequest() is changing into RemoveRequests() in a change list that is already out for review (but you may need to wait for it before checkin)
Pete PTAL. I'm not sure the next steps for Notify* but seems like I need to make them somehow usable in tests... suggestions? Thanks! https://codereview.chromium.org/2233493003/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2233493003/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:85: virtual void NotifyRequestsExpired( On 2016/08/10 17:15:11, Pete Williamson wrote: > We will use the existing NotifyFailed for this. No need to hook it up until the > observer patch lands. Done. https://codereview.chromium.org/2233493003/diff/20001/components/offline_page... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2233493003/diff/20001/components/offline_page... components/offline_pages/background/request_picker.cc:258: coordinator_->NotifyRequestsExpired(expired_requests, result); On 2016/08/10 17:15:11, Pete Williamson wrote: > When the observer changelist lands, this should be changed to call NotifyFailed > for each request. Done. https://codereview.chromium.org/2233493003/diff/20001/components/offline_page... File components/offline_pages/background/request_queue.h (right): https://codereview.chromium.org/2233493003/diff/20001/components/offline_page... components/offline_pages/background/request_queue.h:81: void RemoveRequestsById(std::vector<int64_t> request_ids, On 2016/08/10 17:15:11, Pete Williamson wrote: > You can remove this method. RemoveRequest() is changing into RemoveRequests() > in a change list that is already out for review (but you may need to wait for it > before checkin) Done.
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
If I understand your question about testing correctly, I think the answer is to add a test in request_coordinator_unittest.cc that puts an expired request in the queue, calls get, and observes that we get a notification for the expired request. Look at the PauseAndResumeObserverTest at the bottom of the file for an example of how to add requests and call get, and check that the notification appears. https://codereview.chromium.org/2233493003/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2233493003/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:137: void NotifyRemoved(const SavePageRequest& request); FYI: You'll need to do another merge soon, these are changing: See https://codereview.chromium.org/2249303002/ https://codereview.chromium.org/2233493003/diff/60001/components/offline_page... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2233493003/diff/60001/components/offline_page... components/offline_pages/background/request_picker.cc:262: coordinator_->NotifyFailed(request, This will change to coordinator_->NotifiyCompleted() when my patch in the CQ lands... https://codereview.chromium.org/2233493003/diff/60001/components/offline_page... File components/offline_pages/background/request_picker.h (right): https://codereview.chromium.org/2233493003/diff/60001/components/offline_page... components/offline_pages/background/request_picker.h:25: RequestCoordinator* coordinator); This builds a circular dependency - RP depends on RC, RC depends on RC. To break the dependency, we should have a new interface that both depend on, and pass the interface down to RP. It's OK to just add a TODO for now and do it in a future change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Pete PTAL! I pulled out the notifier as an interface. (also the SavePageStatus which seems to be only used here). Thanks! https://codereview.chromium.org/2233493003/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2233493003/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:137: void NotifyRemoved(const SavePageRequest& request); On 2016/08/16 23:40:46, Pete Williamson wrote: > FYI: You'll need to do another merge soon, these are changing: See > https://codereview.chromium.org/2249303002/ Done. https://codereview.chromium.org/2233493003/diff/60001/components/offline_page... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2233493003/diff/60001/components/offline_page... components/offline_pages/background/request_picker.cc:262: coordinator_->NotifyFailed(request, On 2016/08/16 23:40:46, Pete Williamson wrote: > This will change to coordinator_->NotifiyCompleted() when my patch in the CQ > lands... Done. https://codereview.chromium.org/2233493003/diff/60001/components/offline_page... File components/offline_pages/background/request_picker.h (right): https://codereview.chromium.org/2233493003/diff/60001/components/offline_page... components/offline_pages/background/request_picker.h:25: RequestCoordinator* coordinator); On 2016/08/16 23:40:46, Pete Williamson wrote: > This builds a circular dependency - RP depends on RC, RC depends on RC. To > break the dependency, we should have a new interface that both depend on, and > pass the interface down to RP. It's OK to just add a TODO for now and do it in > a future change. Done.
Looks good, just a few suggestions. Thanks for making a fresh interface for the notifier. I noticed that it seems to make your unit testing easier too, since you can use a stub notifier. https://codereview.chromium.org/2233493003/diff/80001/components/offline_page... File components/offline_pages/background/request_picker_unittest.cc (right): https://codereview.chromium.org/2233493003/diff/80001/components/offline_page... components/offline_pages/background/request_picker_unittest.cc:43: const SavePageRequest kDefaultRequest(0UL, kEmptyRequest might be a better name than kDefaultRequest. https://codereview.chromium.org/2233493003/diff/80001/components/offline_page... components/offline_pages/background/request_picker_unittest.cc:62: total_expired_requests_++; I don't see any code to check the number of expired requests, maybe drop this variable? https://codereview.chromium.org/2233493003/diff/80001/components/offline_page... components/offline_pages/background/request_picker_unittest.cc:315: base::TimeDelta::FromSeconds(kRequestExpirationTimeInSeconds + 60); How does this compile? I thought that kReqeustExpriationTimeInSeconds was a private member of offliner_policy. Maybe instead use policy_->GetRequestExpirationTimeInSeconds().
Addressed comments. Thanks for bringing up the details! And IMO, having interfaces is always helpful for writing tests I guess, since it's possible to mock/override a set of APIs. https://codereview.chromium.org/2233493003/diff/80001/components/offline_page... File components/offline_pages/background/request_picker_unittest.cc (right): https://codereview.chromium.org/2233493003/diff/80001/components/offline_page... components/offline_pages/background/request_picker_unittest.cc:43: const SavePageRequest kDefaultRequest(0UL, On 2016/08/18 17:14:46, Pete Williamson wrote: > kEmptyRequest might be a better name than kDefaultRequest. Done. https://codereview.chromium.org/2233493003/diff/80001/components/offline_page... components/offline_pages/background/request_picker_unittest.cc:62: total_expired_requests_++; On 2016/08/18 17:14:46, Pete Williamson wrote: > I don't see any code to check the number of expired requests, maybe drop this > variable? Ah i meant to use that to check how many requests and I'd like to keep it, but adding a check for it :) https://codereview.chromium.org/2233493003/diff/80001/components/offline_page... components/offline_pages/background/request_picker_unittest.cc:315: base::TimeDelta::FromSeconds(kRequestExpirationTimeInSeconds + 60); On 2016/08/18 17:14:46, Pete Williamson wrote: > How does this compile? I thought that kReqeustExpriationTimeInSeconds was a > private member of offliner_policy. Maybe instead use > policy_->GetRequestExpirationTimeInSeconds(). hmm it's not private but those constants are defined within a namespace... But changed to the getter :)
lgtm
The CQ bit was checked by romax@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org Link to the patchset: https://codereview.chromium.org/2233493003/#ps120001 (title: "fixing build.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Remove expired requests from the queue. Added logic to remove expired requests from the queue when picking next requests. Also added a placeholder for notification through coordinator. BUG=636185 ========== to ========== [Offline Pages] Remove expired requests from the queue. Added logic to remove expired requests from the queue when picking next requests. Also added a placeholder for notification through coordinator. BUG=636185 Committed: https://crrev.com/91f93ff1742c76686413531ec1f6a5214b833bd5 Cr-Commit-Position: refs/heads/master@{#412926} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/91f93ff1742c76686413531ec1f6a5214b833bd5 Cr-Commit-Position: refs/heads/master@{#412926} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
