Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2856793002:
[Offline Prefetch] Prefetch request fetcher (Closed)
|
Created:
3 years, 7 months ago by jianli Modified:
3 years, 7 months ago Reviewers:
dewittj 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 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Prefetch] Prefetch request fetcher
BUG=717305
TEST=new tests
Review-Url: https://codereview.chromium.org/2856793002
Cr-Commit-Position: refs/heads/master@{#469110}
Committed: https://chromium.googlesource.com/chromium/src/+/997646dd86a04c4f08c6908eae431d6c13ec587c
Patch Set 1 #Patch Set 2 : Fix deps #
Total comments: 10
Patch Set 3 : Add 1 more test #Patch Set 4 : Address feedback #
Messages
Total messages: 26 (20 generated)
The CQ bit was checked by jianli@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...
jianli@chromium.org changed reviewers: + dewittj@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 jianli@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 jianli@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...
thanks, this looks good! https://codereview.chromium.org/2856793002/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_request_fetcher.cc (right): https://codereview.chromium.org/2856793002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_request_fetcher.cc:51: url_fetcher_->SetRequestContext(request_context_getter_.get()); I think this section also needs: url_fetcher_->SetAutomaticallyRetryOn5xx(false); url_fetcher_->SetAutomaticallyRetryOnNetworkChanges(0); url_fetcher_->SetStopOnRedirect(true); Even if they aren't needed, we should have tests for these cases. https://codereview.chromium.org/2856793002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_request_fetcher.cc:64: // TODO(jianli): Report UMA. nit: TODO(crbug.com/XXX) https://codereview.chromium.org/2856793002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_request_fetcher.cc:74: return Status::SHOULD_RETRY_WITHOUT_BACKOFF; ERR_BLOCKED_BY_ADMINISTRATOR -> SHOULD_SUSPEND ERR_CONNECTION_RESET -> SHOULD_RETRY_WITH_BACKOFF ERR_CONNECTION_CLOSED -> SHOULD_RETRY_WITH_BACKOFF ERR_CONNECTION_REFUSED -> SHOULD_RETRY_WITH_BACKOFF SSL Handshake Problems -> SHOULD_RETRY_WITH_BACKOFF https://codereview.chromium.org/2856793002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_request_fetcher.cc:80: DVLOG(1) << "HTTP status: " << response_status; http 400 is currently DISCARD per the design doc go/network-errors-and-responses-when-prefetching-offline-pages - either need to add that here or modify the design doc. https://codereview.chromium.org/2856793002/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_request_fetcher_unittest.cc (right): https://codereview.chromium.org/2856793002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_request_fetcher_unittest.cc:72: base::MockCallback<PrefetchRequestFetcher::FinishedCallback> callback; Looks like these tests follow a common form: * Make request with certain response code * Expect that the data is OK and the Status:: is as expected. Could you refactor that into some helper function so these all become one liners, and then test all the cases from the design doc explicitly?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2856793002/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_request_fetcher.cc (right): https://codereview.chromium.org/2856793002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_request_fetcher.cc:51: url_fetcher_->SetRequestContext(request_context_getter_.get()); On 2017/05/02 21:16:35, dewittj wrote: > I think this section also needs: > url_fetcher_->SetAutomaticallyRetryOn5xx(false); > url_fetcher_->SetAutomaticallyRetryOnNetworkChanges(0); > url_fetcher_->SetStopOnRedirect(true); > > Even if they aren't needed, we should have tests for these cases. Added 1st and 2nd. For SetStopOnRedirect, I am a bit hesitated since I think redirect is a perfectly fine case for server to reroute/reconfig. Otherwise the client will just break. Unless redirect will cause harm to the client or the server has an explicit doc saying no redirect, we will probably support it. It is hard to test disbaling these retries since TestURLFetcher does not support this. https://codereview.chromium.org/2856793002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_request_fetcher.cc:64: // TODO(jianli): Report UMA. On 2017/05/02 21:16:35, dewittj wrote: > nit: TODO(crbug.com/XXX) I am going to add this probably in next patch after we hook up the fetcher code with service and start to do real testing. Creating a new crbug seems not to be necessary. https://codereview.chromium.org/2856793002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_request_fetcher.cc:74: return Status::SHOULD_RETRY_WITHOUT_BACKOFF; On 2017/05/02 21:16:35, dewittj wrote: > ERR_BLOCKED_BY_ADMINISTRATOR -> SHOULD_SUSPEND > > ERR_CONNECTION_RESET -> SHOULD_RETRY_WITH_BACKOFF > ERR_CONNECTION_CLOSED -> SHOULD_RETRY_WITH_BACKOFF > ERR_CONNECTION_REFUSED -> SHOULD_RETRY_WITH_BACKOFF > SSL Handshake Problems -> SHOULD_RETRY_WITH_BACKOFF Added ERR_BLOCKED_BY_ADMINISTRATOR. All other net errors are simply treated as SHOULD_RETRY_WITH_BACKOFF. https://codereview.chromium.org/2856793002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_request_fetcher.cc:80: DVLOG(1) << "HTTP status: " << response_status; On 2017/05/02 21:16:35, dewittj wrote: > http 400 is currently DISCARD per the design doc > go/network-errors-and-responses-when-prefetching-offline-pages - either need to > add that here or modify the design doc. I also think we may not want to support DISCARD since it is not necessary and make the whole design more complciated. Left a feedback on the doc. https://codereview.chromium.org/2856793002/diff/20001/components/offline_page... File components/offline_pages/core/prefetch/prefetch_request_fetcher_unittest.cc (right): https://codereview.chromium.org/2856793002/diff/20001/components/offline_page... components/offline_pages/core/prefetch/prefetch_request_fetcher_unittest.cc:72: base::MockCallback<PrefetchRequestFetcher::FinishedCallback> callback; On 2017/05/02 21:16:35, dewittj wrote: > Looks like these tests follow a common form: > * Make request with certain response code > * Expect that the data is OK and the Status:: is as expected. > > Could you refactor that into some helper function so these all become one > liners, and then test all the cases from the design doc explicitly? Done.
The CQ bit was checked by jianli@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.
lgtm, thanks!
The CQ bit was checked by jianli@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493844029255790,
"parent_rev": "12e5f8a1b2d94ef823b88a01f5798a0cc8133a9b", "commit_rev":
"997646dd86a04c4f08c6908eae431d6c13ec587c"}
Message was sent while issue was closed.
Description was changed from ========== [Offline Prefetch] Prefetch request fetcher BUG=717305 TEST=new tests ========== to ========== [Offline Prefetch] Prefetch request fetcher BUG=717305 TEST=new tests Review-Url: https://codereview.chromium.org/2856793002 Cr-Commit-Position: refs/heads/master@{#469110} Committed: https://chromium.googlesource.com/chromium/src/+/997646dd86a04c4f08c6908eae43... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/997646dd86a04c4f08c6908eae43... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
