Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2237453002:
predictors: Add tests to ResourcePrefetchPredictor. (Closed)
Descriptionpredictors: Add tests to ResourcePrefetchPredictor.
BUG=631966
Committed: https://crrev.com/c3833fe89053051ead372b0bd129e0794507c2e7
Cr-Commit-Position: refs/heads/master@{#412514}
Patch Set 1 #Patch Set 2 : . #
Total comments: 6
Patch Set 3 : Address comments. #Messages
Total messages: 18 (11 generated)
lizeb@chromium.org changed reviewers: + pasko@chromium.org
The CQ bit was checked by lizeb@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
sorry to see the pain to create URLRequest for testing. Looks OK. Feel free to add some net/ experts if after extensive looking around you are still interested in finding alternatives. https://codereview.chromium.org/2237453002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2237453002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:62: class MockURLRequestDelegate : public net::URLRequest::Delegate { it is not a powerful mock, would be more readable to call it something like NoopURLRequestDelegate or EmptyURLRequestDelegate. https://codereview.chromium.org/2237453002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:128: net::HttpResponseInfo response_info; public members in classes are discouraged, sorry. This class is not struct-level trivial. In particular, it is making it less easy to read the MaybeCreateJob[...], totally not obvious where 'mime_type' is coming from. https://codereview.chromium.org/2237453002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:211: auto request = url_request_context_.CreateRequest(url, priority, TIL: we discourage auto-ing smart pointers, see discussion: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5-Bt3BJzAo0
Thanks! All done. Per offline discussion, there does not seem to be that much opportunity to refactor testing code, as other uses in tests are different. I guess it's better to leave it as is. PTAL. https://codereview.chromium.org/2237453002/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right): https://codereview.chromium.org/2237453002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:62: class MockURLRequestDelegate : public net::URLRequest::Delegate { On 2016/08/10 16:40:07, pasko wrote: > it is not a powerful mock, would be more readable to call it something like > NoopURLRequestDelegate or EmptyURLRequestDelegate. Done. https://codereview.chromium.org/2237453002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:128: net::HttpResponseInfo response_info; On 2016/08/10 16:40:06, pasko wrote: > public members in classes are discouraged, sorry. This class is not struct-level > trivial. > > In particular, it is making it less easy to read the MaybeCreateJob[...], > totally not obvious where 'mime_type' is coming from. Done. https://codereview.chromium.org/2237453002/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:211: auto request = url_request_context_.CreateRequest(url, priority, On 2016/08/10 16:40:07, pasko wrote: > TIL: we discourage auto-ing smart pointers, see discussion: > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/5-Bt3BJzAo0 Done.
lgtm, thank you, Benoit
The CQ bit was checked by lizeb@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 lizeb@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 #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== predictors: Add tests to ResourcePrefetchPredictor. BUG=631966 ========== to ========== predictors: Add tests to ResourcePrefetchPredictor. BUG=631966 Committed: https://crrev.com/c3833fe89053051ead372b0bd129e0794507c2e7 Cr-Commit-Position: refs/heads/master@{#412514} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c3833fe89053051ead372b0bd129e0794507c2e7 Cr-Commit-Position: refs/heads/master@{#412514} |
