Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2091473004:
Add the ability to cancel prerender reqeusts and tests (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 the ability to cancel prerender reqeusts and tests
This hooks up the existing Cancel() method to now call through
to the pre-renderer and cancel outstanding prerender attempts.
A unit test is added also.
BUG=610521
Committed: https://crrev.com/7fbbac693b162bcb6090a0b0613a3a807c48af70
Cr-Commit-Position: refs/heads/master@{#401695}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Merge with pre-render single instance changes. #Patch Set 3 : Merge with pre-render single instance changes. #
Total comments: 14
Patch Set 4 : CR fixes per DougArnett #
Total comments: 6
Patch Set 5 : Comment changes per DougArnett #
Messages
Total messages: 19 (6 generated)
petewil@chromium.org changed reviewers: + chili@chromium.org, dougarnett@chromium.org
https://codereview.chromium.org/2091473004/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2091473004/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:93: cancelled_ = false; FYI - there may be some overlap with the is_busy_ flag in another changelist (2078113002), I will be merging before this changelist gets submitted.
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/2091473004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2091473004/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2091473004/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:93: cancelled_ = false; nit - we seem to have different spellings of cancelled everywhere, though the concensus appears to be 'canceled'.... It was confusing with ArchiverResult::CANCELED and then SavePageResult::CANCELLED
Took a merge with the change to run only a single instance of the pre-renderer at a time, which had is_busy(). So, I replaced started_ with is_busy_ everywhere, and changed cancelled_ to is_canceled_ to match. https://codereview.chromium.org/2091473004/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2091473004/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:93: cancelled_ = false; On 2016/06/23 05:42:06, chili wrote: > nit - we seem to have different spellings of cancelled everywhere, though the > concensus appears to be 'canceled'.... > > It was confusing with ArchiverResult::CANCELED and then > SavePageResult::CANCELLED When I checked with a dictionary, both were legit, so I changed mine to 'canceled' for consistency with other code.
looking good - some comment and naming suggestions and the test stub idea I mentioned to consider https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:105: GetOffliner(); I wonder about this check here. Maybe you can say more about why it is here. Seems like we may want to get appropriate Offline once we have the next request (at least down the road if the request helps determine which offliner if more than one). https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:139: if (is_canceled_) // Be sure processing hasn't been canceled. https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.h:84: bool is_canceled() { or maybe was_canceled? https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.h:84: bool is_canceled() { // Tracks whether processing has been canceled. Reset upon next StartProcessing call. ? https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.h:109: void GetOffliner(); // Returns the appropriate Offliner to use. ? https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.h:119: // Unowned pointer to the offliner, if any. to the current offliner ? https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:162: void SkipOfflinerCallback() { consider whether changing to stub to not auto callback seems attractive - ie, explicitly control calling the callback with result value for the tests that are verifying callback values.
looking good - some comment and naming suggestions and the test stub idea I mentioned to consider
https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:105: GetOffliner(); On 2016/06/23 17:19:17, dougarnett wrote: > I wonder about this check here. Maybe you can say more about why it is here. > Seems like we may want to get appropriate Offline once we have the next request > (at least down the road if the request helps determine which offliner if more > than one). My thought here was that if we can't get an offliner that we should quit early instead of after some async steps. It can easily be removed so we can make the decision later. Removed the check. https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:139: if (is_canceled_) On 2016/06/23 17:19:17, dougarnett wrote: > // Be sure processing hasn't been canceled. Comment added (I worded it slightly differently). https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.h:84: bool is_canceled() { On 2016/06/23 17:19:17, dougarnett wrote: > or maybe was_canceled? I thought that is_canceled would work better with is_busy, and to me the state of cancellation seems to be ongoing until we actually cancel it, so I prefer is_canceled. What do you think? https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.h:84: bool is_canceled() { On 2016/06/23 17:19:17, dougarnett wrote: > // Tracks whether processing has been canceled. Reset upon next StartProcessing > call. ? Done. https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.h:109: void GetOffliner(); On 2016/06/23 17:19:17, dougarnett wrote: > // Returns the appropriate Offliner to use. ? The real purpose of this is not choosing the offliner, but getting one from the factory if needed. Comment now says both. https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.h:119: // Unowned pointer to the offliner, if any. On 2016/06/23 17:19:17, dougarnett wrote: > to the current offliner ? Done. https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2091473004/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:162: void SkipOfflinerCallback() { On 2016/06/23 17:19:17, dougarnett wrote: > consider whether changing to stub to not auto callback seems attractive - ie, > explicitly control calling the callback with result value for the tests that are > verifying callback values. Changed to EnableOfflinerCallback(bool enable); Any test that requires the callback we call it with true, any test that needs the callback to not happen we call it with false, any test that won't be exercising the callback doesn't call it at all.
lgtm with comments https://codereview.chromium.org/2091473004/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2091473004/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.cc:132: // Check that the pre-render didn't get cancelled while performing some async "pre-render" => "processing" here I think. This method is meant to send request to offliner so sounds weird to say the pre-render was canceled since it hasn't been started. https://codereview.chromium.org/2091473004/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2091473004/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:84: // Tracks whether the last prerender attempt got canceled. This is reset by "prerender" => "processing" or "offlining" https://codereview.chromium.org/2091473004/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:121: // True if a prerender request has been canceled. "prerender request" => "current request" ? or "processing request" or "offlining request"
https://codereview.chromium.org/2091473004/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2091473004/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.cc:132: // Check that the pre-render didn't get cancelled while performing some async On 2016/06/23 18:14:32, dougarnett wrote: > "pre-render" => "processing" here I think. This method is meant to send request > to offliner so sounds weird to say the pre-render was canceled since it hasn't > been started. Done. (I used offlining instead of processing to be consistent with another comment I changed in the .h file) https://codereview.chromium.org/2091473004/diff/60001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2091473004/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:84: // Tracks whether the last prerender attempt got canceled. This is reset by On 2016/06/23 18:14:33, dougarnett wrote: > "prerender" => "processing" or "offlining" Done. https://codereview.chromium.org/2091473004/diff/60001/components/offline_page... components/offline_pages/background/request_coordinator.h:121: // True if a prerender request has been canceled. On 2016/06/23 18:14:33, dougarnett wrote: > "prerender request" => "current request" ? or "processing request" or "offlining > request" Done.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chili@chromium.org, dougarnett@chromium.org Link to the patchset: https://codereview.chromium.org/2091473004/#ps80001 (title: "Comment changes per DougArnett")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2091473004/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add the ability to cancel prerender reqeusts and tests This hooks up the existing Cancel() method to now call through to the pre-renderer and cancel outstanding prerender attempts. A unit test is added also. BUG=610521 ========== to ========== Add the ability to cancel prerender reqeusts and tests This hooks up the existing Cancel() method to now call through to the pre-renderer and cancel outstanding prerender attempts. A unit test is added also. BUG=610521 Committed: https://crrev.com/7fbbac693b162bcb6090a0b0613a3a807c48af70 Cr-Commit-Position: refs/heads/master@{#401695} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/7fbbac693b162bcb6090a0b0613a3a807c48af70 Cr-Commit-Position: refs/heads/master@{#401695} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
