Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2266323002:
Cancel prerendering when a request is paused or removed. (Closed)
|
Created:
4 years, 4 months ago by Pete Williamson Modified:
4 years, 4 months ago Reviewers:
dougarnett 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. |
DescriptionCancel prerendering when a request is paused or removed.
A request might be paused or removed while being pre-rendered. If
that happens, we should cancel the request that is inflight.
As a follow up, we still need to remember that we cancelled it, and
remove any save page that might appear after cancelling it. (there
is already a TODO in the code for that)
BUG=610521
Committed: https://crrev.com/e59d91c72518cf5a820978a46fe24672a5386ea0
Cr-Commit-Position: refs/heads/master@{#413882}
Patch Set 1 #
Total comments: 12
Patch Set 2 : CR feedback per DougArnett #
Total comments: 4
Patch Set 3 : CR feedback per DougArnett #
Total comments: 2
Patch Set 4 : rename flag #
Messages
Total messages: 19 (7 generated)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
petewil@chromium.org changed reviewers: + dougarnett@chromium.org
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.
https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:115: void RequestCoordinator::CancelActiveRequestIfItMatches( CancelRequestIfActive()? a bit shorter https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:123: offliner_->Cancel(); consider calling StopProcessing() instead or check its logic to see what else we should be doing here if we call offliner directly. Actually, we may want this and StopProcessing to both call a helper method to take care of bookkeeping (last_offlining_status, is_canceled, active_request) but might support both stopping and just canceling current and trying next one. https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:125: // TryNextRequest(); even though return value is PRERENDER_CANCELLED. hmm, the Offliner interface says it will not call the completion callback so we should discuss that. But if it does call it, then we may want it to return REQUEST_COORDINATOR_CANCELED https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:338: // TODO: Will this be PRERENDERING_CANCELED or REQUEST_COORDINATOR_CANCELED as noted above, I wasn't expecting there to be a callback currently (so we need to discuss if there does need to be one)
https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:284: active_request_.reset(new SavePageRequest(request)); We may want to move this down and set with updated_request which has been MarkAttemptStarted
https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:115: void RequestCoordinator::CancelActiveRequestIfItMatches( On 2016/08/23 16:32:13, dougarnett wrote: > CancelRequestIfActive()? a bit shorter That's shorter, but not really accurate, since we have a list of requests. How about CancelActiveRequestIfNeeded? https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:123: offliner_->Cancel(); On 2016/08/23 16:32:13, dougarnett wrote: > consider calling StopProcessing() instead or check its logic to see what else we > should be doing here if we call offliner directly. Actually, we may want this > and StopProcessing to both call a helper method to take care of bookkeeping > (last_offlining_status, is_canceled, active_request) but might support both > stopping and just canceling current and trying next one. Added StopPrerendering() to handle common tasks. https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:125: // TryNextRequest(); even though return value is PRERENDER_CANCELLED. On 2016/08/23 16:32:13, dougarnett wrote: > hmm, the Offliner interface says it will not call the completion callback so we > should discuss that. But if it does call it, then we may want it to return > REQUEST_COORDINATOR_CANCELED Ah, I had forgotten that. I don't need to modify the offliner, I'll just keep track of the fact that I need to call TryNextRequest(), and call it after removing/pausing. https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:284: active_request_.reset(new SavePageRequest(request)); On 2016/08/23 17:00:39, dougarnett wrote: > We may want to move this down and set with updated_request which has been > MarkAttemptStarted Done. https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:338: // TODO: Will this be PRERENDERING_CANCELED or REQUEST_COORDINATOR_CANCELED On 2016/08/23 16:32:13, dougarnett wrote: > as noted above, I wasn't expecting there to be a callback currently (so we need > to discuss if there does need to be one) We don't need one, this change reverted.
https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:284: active_request_.reset(new SavePageRequest(request)); On 2016/08/23 17:00:39, dougarnett wrote: > We may want to move this down and set with updated_request which has been > MarkAttemptStarted not the current problem though, as the MarkAbortAttempted is not currently performed on the active_request_ (done the request passed into the callback).
https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2266323002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:115: void RequestCoordinator::CancelActiveRequestIfItMatches( On 2016/08/23 17:48:43, Pete Williamson wrote: > On 2016/08/23 16:32:13, dougarnett wrote: > > CancelRequestIfActive()? a bit shorter > > That's shorter, but not really accurate, since we have a list of requests. How > about CancelActiveRequestIfNeeded? ugh, seems no more accurate than mine. I guess stick with what you have - I am reading it differently now and makes more sense to me. ;-) https://codereview.chromium.org/2266323002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2266323002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:117: // TODO(dougarnett): Find current request and mark attempt aborted. maybe actually DO MarkAttemptAborted now on active_request_ https://codereview.chromium.org/2266323002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:216: is_canceled_ = true; if this flag only applies to StopProcessing (and not to CancelActive*), then maybe should be renamed to is_stopped_ or processing_stopped_ or something. https://codereview.chromium.org/2266323002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2266323002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:640: SendOfflinerDoneCallback(request1, as discussed, we may seeing that SendToOffliner was never called - which is a clue maybe to do another PumpLoop before the Remove if we want to exercise the path where it is called
https://codereview.chromium.org/2266323002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2266323002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:630: PumpLoop(); More PumpLoop() to get SendToOffliner to run (eg, so that is_busy_ and active_request_ are set)
https://codereview.chromium.org/2266323002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2266323002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.cc:216: is_canceled_ = true; On 2016/08/23 20:59:45, dougarnett wrote: > if this flag only applies to StopProcessing (and not to CancelActive*), then > maybe should be renamed to is_stopped_ or processing_stopped_ or something. Done. https://codereview.chromium.org/2266323002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2266323002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:630: PumpLoop(); On 2016/08/23 21:50:24, dougarnett wrote: > More PumpLoop() to get SendToOffliner to run (eg, so that is_busy_ and > active_request_ are set) A single PumpLoop() will run until the task queue is empty (idle), so we should not need any more, and in fact the test is passing.
lgtm (as long as trybots happy ;-)
The CQ bit was checked by petewil@chromium.org
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 #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Cancel prerendering when a request is paused or removed. A request might be paused or removed while being pre-rendered. If that happens, we should cancel the request that is inflight. As a follow up, we still need to remember that we cancelled it, and remove any save page that might appear after cancelling it. (there is already a TODO in the code for that) BUG=610521 ========== to ========== Cancel prerendering when a request is paused or removed. A request might be paused or removed while being pre-rendered. If that happens, we should cancel the request that is inflight. As a follow up, we still need to remember that we cancelled it, and remove any save page that might appear after cancelling it. (there is already a TODO in the code for that) BUG=610521 Committed: https://crrev.com/e59d91c72518cf5a820978a46fe24672a5386ea0 Cr-Commit-Position: refs/heads/master@{#413882} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e59d91c72518cf5a820978a46fe24672a5386ea0 Cr-Commit-Position: refs/heads/master@{#413882} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
