Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2519963002:
predictors: Add browsertest that checks redirects. (Closed)
Descriptionpredictors: Add browsertest that checks redirects.
Make sure that ResourcePrefetchPredictor learns about subresources when a main
resource has redirects.
BUG=650253
Committed: https://crrev.com/bf5b8e0f7794b08c10af2bb682bee81978df84c5
Cr-Commit-Position: refs/heads/master@{#433932}
Patch Set 1 : . #
Total comments: 20
Patch Set 2 : Refactoring + add new tests. #Patch Set 3 : . #
Total comments: 4
Patch Set 4 : Some style fixes. #Patch Set 5 : . #
Total comments: 2
Patch Set 6 : Nits. #Messages
Total messages: 24 (11 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
alexilin@chromium.org changed reviewers: + lizeb@chromium.org, pasko@chromium.org
Hi, Let's move in small steps.
On 2016/11/21 15:02:45, alexilin wrote: > Hi, > Let's move in small steps. Thanks! lgtm with minor comments. Minor comments: - Can you make the methods const where applicable? It seems that the request handling ones could be. - This can be safely deferred, but it would be nice to have tests for the following scenarios as well: - 301 - Several redirects - HTTP -> HTTPS redirects. (may be hard...)
https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:236: http_response->set_code(net::HTTP_FOUND); Can you also test with a 301?
overall looks good, a few style-related suggestions https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:137: ResourceSummary* AddResource(const std::string& resource_path, should this method be protected? https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:146: auto result = resources_.insert(std::make_pair(resource_path, resource)); should we also assert that we are not inserting the same key twice? checking the value of result.second basically .. https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:147: return &(result.first->second); 'auto result' followed by '&(result.first->second)' is quite cryptic :/ Slight changes in naming and modifying the ResourceSummary that is already inserted into the map before returning it makes the types clearer. Suggesting this: auto pair_and_whether_inserted = resources_.insert(std::make_pair(resource_path, ResourceSummary())); DCHECK(pair_and_whether_inserted.second); ResourceSummary* resource = &pair_and_whether_inserted.first->second; resource->request.resource_url = embedded_test_server()->GetURL(resource_path); resource->request.resource_type = resource_type; resource->request.priority = priority; resource->request.has_validators = true; return resource; hopefully this also works :) https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:150: void AddRedirectChain(const std::vector<std::string>& redirect_chain) { protected as well? https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:152: for (size_t i = 0; i + 1 < redirect_chain.size(); ++i) { looks unusual, a more common way to write it would be for (size_t i = 0; i < redirect_chain.size() - 1; ++i) faster to read this way, at least for me https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:193: for (auto it = redirects_.find(*current); it != redirects_.end(); this for-loop is multi-line and unusual enough so that I think a while loop would be easier to understand: while (true) { auto it = redirects_.find(*current); if (it == redirects_.end()) break; current = &(it->second); }
> - Can you make the methods const where applicable? It seems > that the request handling ones could be. Done. Thanks for this nit. > - This can be safely deferred, but it would be nice to have > tests for the following scenarios as well: > - 301 Done. I added corresponding edge labels to the redirects graph. > - Several redirects Had already done. I added test with one redirect. > - HTTP -> HTTPS redirects. (may be hard...) Done. It required some refactoring of test class. Now it uses URLs instead of relative paths to not have dependence on EmbeddedTestServer instance we use. https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:236: http_response->set_code(net::HTTP_FOUND); On 2016/11/21 16:24:25, Benoit L wrote: > Can you also test with a 301? Done.
https://codereview.chromium.org/2519963002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2519963002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:156: const GURL* current = &initial_url; Per offline discussion, this is slightly convoluted even though it's correct. Can you simplify the GURL handling here? https://codereview.chromium.org/2519963002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:164: // Simply shortcut for convenience. nit: s/Simply//
pasko@: I haven't seen your comments yesterday :) https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:137: ResourceSummary* AddResource(const std::string& resource_path, On 2016/11/21 18:42:39, pasko wrote: > should this method be protected? I can't reveal the logic behind this decision. As far as all test methods have an access to protected fields of fixture class I can simply mark all methods as protected and it will work. Test fixture class hasn't other consumers. Explain, please, why this method should be protected and all others shouldn't. https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:146: auto result = resources_.insert(std::make_pair(resource_path, resource)); On 2016/11/21 18:42:39, pasko wrote: > should we also assert that we are not inserting the same key twice? checking the > value of result.second basically .. Here we definitely have problems with resources that could be requested twice (This could happen and this is unpredictable). I'll propose the change of the method how we check subresources consumed by ResourcePrefetchPredictor in the next CL. Nevertheless your suggestion was correct. Done. https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:147: return &(result.first->second); On 2016/11/21 18:42:39, pasko wrote: > 'auto result' followed by '&(result.first->second)' is quite cryptic :/ > > Slight changes in naming and modifying the ResourceSummary that is already > inserted into the map before returning it makes the types clearer. Suggesting > this: > > auto pair_and_whether_inserted = > resources_.insert(std::make_pair(resource_path, ResourceSummary())); > DCHECK(pair_and_whether_inserted.second); > ResourceSummary* resource = &pair_and_whether_inserted.first->second; > resource->request.resource_url = > embedded_test_server()->GetURL(resource_path); > resource->request.resource_type = resource_type; > resource->request.priority = priority; > resource->request.has_validators = true; > return resource; > > hopefully this also works :) Thanks, it works! Btw, what about DCHECK in this code? I can't change it to ASSERT_TRUE because of non-void return type in this function. Maybe EXPECT_TRUE will be better here? https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:152: for (size_t i = 0; i + 1 < redirect_chain.size(); ++i) { On 2016/11/21 18:42:39, pasko wrote: > looks unusual, a more common way to write it would be > for (size_t i = 0; i < redirect_chain.size() - 1; ++i) > > faster to read this way, at least for me I rewrote this method to support various HTTP redirect codes. The reason why I had written this in such way is because I don't like subtract something from unsigned typed variables. Anyway, I had had ASSERT before so it would be safe. https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:193: for (auto it = redirects_.find(*current); it != redirects_.end(); On 2016/11/21 18:42:39, pasko wrote: > this for-loop is multi-line and unusual enough so that I think a while loop > would be easier to understand: > while (true) { > auto it = redirects_.find(*current); > if (it == redirects_.end()) > break; > current = &(it->second); > } Done. https://codereview.chromium.org/2519963002/diff/80001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2519963002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:156: const GURL* current = &initial_url; On 2016/11/22 10:58:28, Benoit L wrote: > Per offline discussion, this is slightly convoluted even though it's correct. > Can you simplify the GURL handling here? Done. https://codereview.chromium.org/2519963002/diff/80001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:164: // Simply shortcut for convenience. On 2016/11/22 10:58:28, Benoit L wrote: > nit: s/Simply// Done.
lgtm with two remaining requests addressed https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:137: ResourceSummary* AddResource(const std::string& resource_path, On 2016/11/22 11:04:29, alexilin wrote: > On 2016/11/21 18:42:39, pasko wrote: > > should this method be protected? > > I can't reveal the logic behind this decision. As far as all test methods have > an access to protected fields of fixture class I can simply mark all methods as > protected and it will work. Test fixture class hasn't other consumers. > Explain, please, why this method should be protected and all others shouldn't. Yeah, it does not make sense to keep some methods public and some protected. I just looked at new-ish methods and it did not make sense to make them public. We should just make everything protected to be clearer about the class usage, though, as you noted, it is not a big issue, this is a test fixture anyway. The gtest FAQ examples have everything protected, so feels good to be consistent: https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:147: return &(result.first->second); On 2016/11/22 11:04:29, alexilin wrote: > On 2016/11/21 18:42:39, pasko wrote: > > 'auto result' followed by '&(result.first->second)' is quite cryptic :/ > > > > Slight changes in naming and modifying the ResourceSummary that is already > > inserted into the map before returning it makes the types clearer. Suggesting > > this: > > > > auto pair_and_whether_inserted = > > resources_.insert(std::make_pair(resource_path, ResourceSummary())); > > DCHECK(pair_and_whether_inserted.second); > > ResourceSummary* resource = &pair_and_whether_inserted.first->second; > > resource->request.resource_url = > > embedded_test_server()->GetURL(resource_path); > > resource->request.resource_type = resource_type; > > resource->request.priority = priority; > > resource->request.has_validators = true; > > return resource; > > > > hopefully this also works :) > > Thanks, it works! > Btw, what about DCHECK in this code? I can't change it to ASSERT_TRUE because of > non-void return type in this function. Maybe EXPECT_TRUE will be better here? argh, forgot about DCHECK, yeah, I think EXPECT_TRUE is appropriate https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:152: for (size_t i = 0; i + 1 < redirect_chain.size(); ++i) { On 2016/11/22 11:04:29, alexilin wrote: > On 2016/11/21 18:42:39, pasko wrote: > > looks unusual, a more common way to write it would be > > for (size_t i = 0; i < redirect_chain.size() - 1; ++i) > > > > faster to read this way, at least for me > > I rewrote this method to support various HTTP redirect codes. > The reason why I had written this in such way is because I don't like subtract > something from unsigned typed variables. Anyway, I had had ASSERT before so it > would be safe. ah, it explains a lot, makes sense. I actually should have written "- 1U" .. and it's all rewritten to keep status code on the edge - nice. https://codereview.chromium.org/2519963002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2519963002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:44: net::HttpStatusCode code; Makes sense to clarify in the comment whether it is the response code for the |url| or the previous/next url in the chain.
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...
https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:137: ResourceSummary* AddResource(const std::string& resource_path, On 2016/11/22 15:42:14, pasko wrote: > On 2016/11/22 11:04:29, alexilin wrote: > > On 2016/11/21 18:42:39, pasko wrote: > > > should this method be protected? > > > > I can't reveal the logic behind this decision. As far as all test methods have > > an access to protected fields of fixture class I can simply mark all methods > as > > protected and it will work. Test fixture class hasn't other consumers. > > Explain, please, why this method should be protected and all others shouldn't. > > Yeah, it does not make sense to keep some methods public and some protected. I > just looked at new-ish methods and it did not make sense to make them public. We > should just make everything protected to be clearer about the class usage, > though, as you noted, it is not a big issue, this is a test fixture anyway. > > The gtest FAQ examples have everything protected, so feels good to be > consistent: > https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md Done. https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:147: return &(result.first->second); On 2016/11/22 15:42:14, pasko wrote: > On 2016/11/22 11:04:29, alexilin wrote: > > On 2016/11/21 18:42:39, pasko wrote: > > > 'auto result' followed by '&(result.first->second)' is quite cryptic :/ > > > > > > Slight changes in naming and modifying the ResourceSummary that is already > > > inserted into the map before returning it makes the types clearer. > Suggesting > > > this: > > > > > > auto pair_and_whether_inserted = > > > resources_.insert(std::make_pair(resource_path, ResourceSummary())); > > > DCHECK(pair_and_whether_inserted.second); > > > ResourceSummary* resource = &pair_and_whether_inserted.first->second; > > > resource->request.resource_url = > > > embedded_test_server()->GetURL(resource_path); > > > resource->request.resource_type = resource_type; > > > resource->request.priority = priority; > > > resource->request.has_validators = true; > > > return resource; > > > > > > hopefully this also works :) > > > > Thanks, it works! > > Btw, what about DCHECK in this code? I can't change it to ASSERT_TRUE because > of > > non-void return type in this function. Maybe EXPECT_TRUE will be better here? > > argh, forgot about DCHECK, yeah, I think EXPECT_TRUE is appropriate Acknowledged. https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:150: void AddRedirectChain(const std::vector<std::string>& redirect_chain) { On 2016/11/21 18:42:39, pasko wrote: > protected as well? Done. https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:152: for (size_t i = 0; i + 1 < redirect_chain.size(); ++i) { > ah, it explains a lot, makes sense. I actually should have written "- 1U" .. and > it's all rewritten to keep status code on the edge - nice. Done. Actually, there is no difference between i < redirect_chain.size() - 1 and i < redirect_chain.size() - 1U The result is incorrect in both cases if redirect_chain is empty (cause to endless loop due unsigned overflow). https://codereview.chromium.org/2519963002/diff/120001/chrome/browser/predict... File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc (right): https://codereview.chromium.org/2519963002/diff/120001/chrome/browser/predict... chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:44: net::HttpStatusCode code; On 2016/11/22 15:42:14, pasko wrote: > Makes sense to clarify in the comment whether it is the response code for the > |url| or the previous/next url in the chain. Done.
On 2016/11/22 16:12:36, alexilin wrote: > https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... > File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc > (right): > > https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... > chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:137: > ResourceSummary* AddResource(const std::string& resource_path, > On 2016/11/22 15:42:14, pasko wrote: > > On 2016/11/22 11:04:29, alexilin wrote: > > > On 2016/11/21 18:42:39, pasko wrote: > > > > should this method be protected? > > > > > > I can't reveal the logic behind this decision. As far as all test methods > have > > > an access to protected fields of fixture class I can simply mark all methods > > as > > > protected and it will work. Test fixture class hasn't other consumers. > > > Explain, please, why this method should be protected and all others > shouldn't. > > > > Yeah, it does not make sense to keep some methods public and some protected. I > > just looked at new-ish methods and it did not make sense to make them public. > We > > should just make everything protected to be clearer about the class usage, > > though, as you noted, it is not a big issue, this is a test fixture anyway. > > > > The gtest FAQ examples have everything protected, so feels good to be > > consistent: > > https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md > > Done. > > https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... > chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:147: return > &(result.first->second); > On 2016/11/22 15:42:14, pasko wrote: > > On 2016/11/22 11:04:29, alexilin wrote: > > > On 2016/11/21 18:42:39, pasko wrote: > > > > 'auto result' followed by '&(result.first->second)' is quite cryptic :/ > > > > > > > > Slight changes in naming and modifying the ResourceSummary that is already > > > > inserted into the map before returning it makes the types clearer. > > Suggesting > > > > this: > > > > > > > > auto pair_and_whether_inserted = > > > > resources_.insert(std::make_pair(resource_path, > ResourceSummary())); > > > > DCHECK(pair_and_whether_inserted.second); > > > > ResourceSummary* resource = &pair_and_whether_inserted.first->second; > > > > resource->request.resource_url = > > > > embedded_test_server()->GetURL(resource_path); > > > > resource->request.resource_type = resource_type; > > > > resource->request.priority = priority; > > > > resource->request.has_validators = true; > > > > return resource; > > > > > > > > hopefully this also works :) > > > > > > Thanks, it works! > > > Btw, what about DCHECK in this code? I can't change it to ASSERT_TRUE > because > > of > > > non-void return type in this function. Maybe EXPECT_TRUE will be better > here? > > > > argh, forgot about DCHECK, yeah, I think EXPECT_TRUE is appropriate > > Acknowledged. > > https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... > chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:150: void > AddRedirectChain(const std::vector<std::string>& redirect_chain) { > On 2016/11/21 18:42:39, pasko wrote: > > protected as well? > > Done. > > https://codereview.chromium.org/2519963002/diff/40001/chrome/browser/predicto... > chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:152: for > (size_t i = 0; i + 1 < redirect_chain.size(); ++i) { > > ah, it explains a lot, makes sense. I actually should have written "- 1U" .. > and > > it's all rewritten to keep status code on the edge - nice. > > Done. > Actually, there is no difference between > i < redirect_chain.size() - 1 > and > i < redirect_chain.size() - 1U > The result is incorrect in both cases if redirect_chain is empty (cause to > endless loop due unsigned overflow). > > https://codereview.chromium.org/2519963002/diff/120001/chrome/browser/predict... > File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc > (right): > > https://codereview.chromium.org/2519963002/diff/120001/chrome/browser/predict... > chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:44: > net::HttpStatusCode code; > On 2016/11/22 15:42:14, pasko wrote: > > Makes sense to clarify in the comment whether it is the response code for the > > |url| or the previous/next url in the chain. > > Done. still lgtm.
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
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org Link to the patchset: https://codereview.chromium.org/2519963002/#ps140001 (title: "Nits.")
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": 140001, "attempt_start_ts": 1479840687164410,
"parent_rev": "eba7522a9d690d7d08390ad5d2c55b72f6c1c26a", "commit_rev":
"b2e7cfafd45d8b858c47c6b72b9788c8f6238478"}
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== predictors: Add browsertest that checks redirects. Make sure that ResourcePrefetchPredictor learns about subresources when a main resource has redirects. BUG=650253 ========== to ========== predictors: Add browsertest that checks redirects. Make sure that ResourcePrefetchPredictor learns about subresources when a main resource has redirects. BUG=650253 Committed: https://crrev.com/bf5b8e0f7794b08c10af2bb682bee81978df84c5 Cr-Commit-Position: refs/heads/master@{#433932} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/bf5b8e0f7794b08c10af2bb682bee81978df84c5 Cr-Commit-Position: refs/heads/master@{#433932} |
