Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2592243003:
predictors: ResourcePrefetchPredictorBrowserTest improvements. (Closed)
Descriptionpredictors: ResourcePrefetchPredictorBrowserTest improvements.
This CL contains 4 minor changes bundled together.
- ResourceSummary flags refactoring. Now they have more clean and strict
meanings + additional descriptions in comments.
- New check at HTTP server side that there is no resource requests after
prefetch making the tests more strict.
- New test with "Cache-control: no-cache" resources.
- Respectful handling of validation requests.
BUG=650253
Committed: https://crrev.com/820fc0653f57cfa4d75396b2606116430cb67aeb
Cr-Commit-Position: refs/heads/master@{#440740}
Patch Set 1 #Patch Set 2 : Cover external resources also. #
Total comments: 19
Patch Set 3 : Rebase. #Patch Set 4 : Fix merge WTF. #Patch Set 5 : Fix merge WTF part 2. #Patch Set 6 : Address pasko@ comments. #
Total comments: 5
Patch Set 7 : Fix iterator type and nits. #Messages
Total messages: 24 (14 generated)
Description was changed from ========== predictors: ResourcePrefetchPredictorBrowserTest improvements. This CL contains 4 minor changes bundled together. - ResourceSummary flags refactoring. Now they have more clean and strict meanings + additional descriptions in comments. - New check at HTTP server side making the tests more strict. - New test with "Cache-control: no-cache" resources. - Respectful handling of validation requests. BUG=650253 ========== to ========== predictors: ResourcePrefetchPredictorBrowserTest improvements. This CL contains 4 minor changes bundled together. - ResourceSummary flags refactoring. Now they have more clean and strict meanings + additional descriptions in comments. - New check at HTTP server side that there is no resource requests after prefetch making the tests more strict. - New test with "Cache-control: no-cache" resources. - Respectful handling of validation requests. BUG=650253 ==========
alexilin@chromium.org changed reviewers: + lizeb@chromium.org
Description was changed from ========== predictors: ResourcePrefetchPredictorBrowserTest improvements. This CL contains 4 minor changes bundled together. - ResourceSummary flags refactoring. Now they have more clean and strict meanings + additional descriptions in comments. - New check at HTTP server side that there is no resource requests after prefetch making the tests more strict. - New test with "Cache-control: no-cache" resources. - Respectful handling of validation requests. BUG=650253 ========== to ========== predictors: ResourcePrefetchPredictorBrowserTest improvements. This CL contains 4 minor changes bundled together. - ResourceSummary flags refactoring. Now they have more clean and strict meanings + additional descriptions in comments. - New check at HTTP server side that there is no resource requests after prefetch making the tests more strict. - New test with "Cache-control: no-cache" resources. - Respectful handling of validation requests. BUG=650253 ==========
alexilin@chromium.org changed reviewers: + pasko@chromium.org
pasko, could you please look at this while lizeb is on vacation?
overall looks good, nice extra checks that, I remember we wanted them I have a few stylistic requests, and please rebase. https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:76: // True iff a test must fail on request to this resource. nit: since we are in ResourceSummary, we could remove some redundancy with s/is_request_prohibited/is_prohibited/. Up to you. https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:76: // True iff a test must fail on request to this resource. I'd rephrase slightly (because a test can fail for other reasons): // A request with |is_request_prohibited| set to true makes the test (that originates the request) fail. https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:178: std::string GetVersionnedETag(size_t version, const std::string& path) { s/GetVersionned/MakeVersioned/ (we are not looking up the tag anywhere, so I think 'make' is more applicable) https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:296: kv.second.is_request_prohibited = false; why should this be returned back to false? Are we doing more than one TestLearningAndPrefetching() in a single test? https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:312: bool match_navigation_id = this part is already committed in crrev.com/62729f8a, please rebase https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:470: auto resource_it = resources_.find(request.GetURL()); since the type for |resources_| is declared elsewhere, it is not obvious what the type of |resource_it| is. Please declare the type explicitly. https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:480: std::unique_ptr<net::test_server::HttpResponse> HandleResourceRequest( I think it is time for a brief comment of how requests are handled: 1. where it takes the data to serve requests 2. what parts of |request| it uses 3. mention that it always responds with HTTP 304 on validation requests what else would be useful?
https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:76: // True iff a test must fail on request to this resource. On 2016/12/26 14:12:16, pasko wrote: > nit: since we are in ResourceSummary, we could remove some redundancy with > s/is_request_prohibited/is_prohibited/. Up to you. Done. https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:178: std::string GetVersionnedETag(size_t version, const std::string& path) { On 2016/12/26 14:12:17, pasko wrote: > s/GetVersionned/MakeVersioned/ > > (we are not looking up the tag anywhere, so I think 'make' is more applicable) I'd prefer to name it CreateVersionnedETag since it more consistent with other helper functions in resource_prefetch_predictor_test_utils. https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:296: kv.second.is_request_prohibited = false; On 2016/12/26 14:12:17, pasko wrote: > why should this be returned back to false? Are we doing more than one > TestLearningAndPrefetching() in a single test? Potentially yes. For example, we already have CrossSiteNavigation test but it cleans resources between two function calls so it doesn't count. I just think that it'd be nice to return the system to the initial state after we've done here. https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:312: bool match_navigation_id = On 2016/12/26 14:12:17, pasko wrote: > this part is already committed in crrev.com/62729f8a, please rebase Done. https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:470: auto resource_it = resources_.find(request.GetURL()); On 2016/12/26 14:12:17, pasko wrote: > since the type for |resources_| is declared elsewhere, it is not obvious what > the type of |resource_it| is. Please declare the type explicitly. It is debatable whether "std::map<X, Y>::const_iterator" is more readable than "auto" with a proper variable name. It's a little bit better with typedef for the map type. I also fixed other similar cases in this file. https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:480: std::unique_ptr<net::test_server::HttpResponse> HandleResourceRequest( On 2016/12/26 14:12:17, pasko wrote: > I think it is time for a brief comment of how requests are handled: > 1. where it takes the data to serve requests > 2. what parts of |request| it uses > 3. mention that it always responds with HTTP 304 on validation requests > > what else would be useful? Done. + added remark about the execution thread. I think it's too verbose to describe how each field of ResourceSummary is used to form a response.
please do a CQ dry run on the rebased version https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:178: std::string GetVersionnedETag(size_t version, const std::string& path) { On 2016/12/26 17:33:22, alexilin wrote: > On 2016/12/26 14:12:17, pasko wrote: > > s/GetVersionned/MakeVersioned/ > > > > (we are not looking up the tag anywhere, so I think 'make' is more applicable) > > I'd prefer to name it CreateVersionnedETag since it more consistent with other > helper functions in resource_prefetch_predictor_test_utils. sgtm on the "Create" part, but s/Versionned/Versioned/ https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:296: kv.second.is_request_prohibited = false; On 2016/12/26 17:33:22, alexilin wrote: > On 2016/12/26 14:12:17, pasko wrote: > > why should this be returned back to false? Are we doing more than one > > TestLearningAndPrefetching() in a single test? > > Potentially yes. For example, we already have CrossSiteNavigation test but it > cleans resources between two function calls so it doesn't count. > I just think that it'd be nice to return the system to the initial state after > we've done here. OK, makes sense, agreed. https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:470: auto resource_it = resources_.find(request.GetURL()); On 2016/12/26 17:33:23, alexilin wrote: > On 2016/12/26 14:12:17, pasko wrote: > > since the type for |resources_| is declared elsewhere, it is not obvious what > > the type of |resource_it| is. Please declare the type explicitly. > > It is debatable whether "std::map<X, Y>::const_iterator" is more readable than > "auto" with a proper variable name. It's a little bit better with typedef for > the map type. > > I also fixed other similar cases in this file. Arguably finding the definition of ResourceMap is extra work when reading, and it is somewhere in the middle of the file. The style guide recommends declaring typedefs/using near the top of the 'private' section: https://google.github.io/styleguide/cppguide.html#Declaration_Order ResourceMap is too ambiguous, I would agree with GurlToResourceSummaryMap, if put in the right order. https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:480: std::unique_ptr<net::test_server::HttpResponse> HandleResourceRequest( On 2016/12/26 17:33:23, alexilin wrote: > On 2016/12/26 14:12:17, pasko wrote: > > I think it is time for a brief comment of how requests are handled: > > 1. where it takes the data to serve requests > > 2. what parts of |request| it uses > > 3. mention that it always responds with HTTP 304 on validation requests > > > > what else would be useful? > > Done. > + added remark about the execution thread. > I think it's too verbose to describe how each field of ResourceSummary is used > to form a response. Thanks! https://codereview.chromium.org/2592243003/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2592243003/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:510: // ETag values match, the handler responds with a HTTP 304 status. nit: please add two spaces before "ETag" https://codereview.chromium.org/2592243003/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:584: typedef std::map<GURL, ResourceSummary> ResourceMap; Use type aliases instead of typedef: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/8dOAMzgR4ao
The CQ bit was checked by alexilin@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...
It seems like all bots will be happy eventually with the rebased version. https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:178: std::string GetVersionnedETag(size_t version, const std::string& path) { On 2016/12/26 17:56:49, pasko wrote: > On 2016/12/26 17:33:22, alexilin wrote: > > On 2016/12/26 14:12:17, pasko wrote: > > > s/GetVersionned/MakeVersioned/ > > > > > > (we are not looking up the tag anywhere, so I think 'make' is more > applicable) > > > > I'd prefer to name it CreateVersionnedETag since it more consistent with other > > helper functions in resource_prefetch_predictor_test_utils. > > sgtm on the "Create" part, but s/Versionned/Versioned/ Done. https://codereview.chromium.org/2592243003/diff/20001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:470: auto resource_it = resources_.find(request.GetURL()); On 2016/12/26 17:56:49, pasko wrote: > On 2016/12/26 17:33:23, alexilin wrote: > > On 2016/12/26 14:12:17, pasko wrote: > > > since the type for |resources_| is declared elsewhere, it is not obvious > what > > > the type of |resource_it| is. Please declare the type explicitly. > > > > It is debatable whether "std::map<X, Y>::const_iterator" is more readable than > > "auto" with a proper variable name. It's a little bit better with typedef for > > the map type. > > > > I also fixed other similar cases in this file. > > Arguably finding the definition of ResourceMap is extra work when reading, and > it is somewhere in the middle of the file. The style guide recommends declaring > typedefs/using near the top of the 'private' section: > https://google.github.io/styleguide/cppguide.html#Declaration_Order > > ResourceMap is too ambiguous, I would agree with GurlToResourceSummaryMap, if > put in the right order. GurlToResourceSummaryMap1234567 std::map<GURL, ResourceSummary> 7 chars win. I'd prefer to leave full type definition then. Done. https://codereview.chromium.org/2592243003/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2592243003/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:510: // ETag values match, the handler responds with a HTTP 304 status. On 2016/12/26 17:56:49, pasko wrote: > nit: please add two spaces before "ETag" Done. https://codereview.chromium.org/2592243003/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:584: typedef std::map<GURL, ResourceSummary> ResourceMap; On 2016/12/26 17:56:49, pasko wrote: > Use type aliases instead of typedef: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/8dOAMzgR4ao Thanks for pointing me at this thread. We have a mix of both styles in /predictors directory and I didn't know whether there was a preference. I also have found the same recommendation on https://chromium-cpp.appspot.com/ so we definitely should follow it.
The CQ bit was checked by alexilin@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...
lgtm, thank you! https://codereview.chromium.org/2592243003/diff/100001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2592243003/diff/100001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:584: typedef std::map<GURL, ResourceSummary> ResourceMap; On 2016/12/26 18:27:08, alexilin wrote: > On 2016/12/26 17:56:49, pasko wrote: > > Use type aliases instead of typedef: > > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/8dOAMzgR4ao > > Thanks for pointing me at this thread. We have a mix of both styles in > /predictors directory and I didn't know whether there was a preference. > I also have found the same recommendation on https://chromium-cpp.appspot.com/ > so we definitely should follow it. That's basically where I found this thread :)
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 alexilin@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": 120001, "attempt_start_ts": 1482826519316880,
"parent_rev": "40067418a46c514cf01d4bbd986254eb2c2f199d", "commit_rev":
"36d78edaf3dfb10555eb8850261f788f389b468c"}
Message was sent while issue was closed.
Description was changed from ========== predictors: ResourcePrefetchPredictorBrowserTest improvements. This CL contains 4 minor changes bundled together. - ResourceSummary flags refactoring. Now they have more clean and strict meanings + additional descriptions in comments. - New check at HTTP server side that there is no resource requests after prefetch making the tests more strict. - New test with "Cache-control: no-cache" resources. - Respectful handling of validation requests. BUG=650253 ========== to ========== predictors: ResourcePrefetchPredictorBrowserTest improvements. This CL contains 4 minor changes bundled together. - ResourceSummary flags refactoring. Now they have more clean and strict meanings + additional descriptions in comments. - New check at HTTP server side that there is no resource requests after prefetch making the tests more strict. - New test with "Cache-control: no-cache" resources. - Respectful handling of validation requests. BUG=650253 Review-Url: https://codereview.chromium.org/2592243003 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== predictors: ResourcePrefetchPredictorBrowserTest improvements. This CL contains 4 minor changes bundled together. - ResourceSummary flags refactoring. Now they have more clean and strict meanings + additional descriptions in comments. - New check at HTTP server side that there is no resource requests after prefetch making the tests more strict. - New test with "Cache-control: no-cache" resources. - Respectful handling of validation requests. BUG=650253 Review-Url: https://codereview.chromium.org/2592243003 ========== to ========== predictors: ResourcePrefetchPredictorBrowserTest improvements. This CL contains 4 minor changes bundled together. - ResourceSummary flags refactoring. Now they have more clean and strict meanings + additional descriptions in comments. - New check at HTTP server side that there is no resource requests after prefetch making the tests more strict. - New test with "Cache-control: no-cache" resources. - Respectful handling of validation requests. BUG=650253 Committed: https://crrev.com/820fc0653f57cfa4d75396b2606116430cb67aeb Cr-Commit-Position: refs/heads/master@{#440740} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/820fc0653f57cfa4d75396b2606116430cb67aeb Cr-Commit-Position: refs/heads/master@{#440740} |
