predictors: Add browsertest that checks redirects.

chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

Index: chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
diff --git a/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc b/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
index ab06012517352d5248c1011db73166607aab2b43..3c2ee2dc3d98686e3d8758faa6c68fc1ddf2f8f1 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
@@ -21,11 +21,14 @@
 
 namespace predictors {
 
-static const char kImageUrl[] = "/predictors/image.png";
-static const char kStyleUrl[] = "/predictors/style.css";
-static const char kScriptUrl[] = "/predictors/script.js";
-static const char kFontUrl[] = "/predictors/font.ttf";
-static const char kHtmlSubresourcesUrl[] = "/predictors/html_subresources.html";
+static const char kImagePath[] = "/predictors/image.png";
+static const char kStylePath[] = "/predictors/style.css";
+static const char kScriptPath[] = "/predictors/script.js";
+static const char kFontPath[] = "/predictors/font.ttf";
+static const char kHtmlSubresourcesPath[] =
+    "/predictors/html_subresources.html";
+static const char kRedirectPath[] = "/predictors/redirect.html";
+static const char kRedirectPath2[] = "/predictors/redirect2.html";
 
 struct ResourceSummary {
   ResourceSummary() : is_no_store(false), version(0) {}
@@ -100,7 +103,10 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
 
   void SetUpOnMainThread() override {
     embedded_test_server()->RegisterRequestHandler(
-        base::Bind(&ResourcePrefetchPredictorBrowserTest::HandleRequest,
+        base::Bind(&ResourcePrefetchPredictorBrowserTest::HandleRedirectRequest,
+                   base::Unretained(this)));
+    embedded_test_server()->RegisterRequestHandler(
+        base::Bind(&ResourcePrefetchPredictorBrowserTest::HandleResourceRequest,
                    base::Unretained(this)));
     ASSERT_TRUE(embedded_test_server()->Start());
     predictor_ =
@@ -109,38 +115,48 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     EnsurePredictorInitialized();
   }
 
-  void NavigateToURLAndCheckSubresources(
-      const std::string& main_frame_relative) {
-    GURL main_frame_absolute =
-        embedded_test_server()->GetURL(main_frame_relative);
+  void NavigateToURLAndCheckSubresources(const std::string& main_frame_path) {
+    std::string endpoint_path = GetRedirectEndpoint(main_frame_path);
     std::vector<URLRequestSummary> url_request_summaries;
     for (const auto& kv : resources_) {
       if (kv.second.is_no_store)
         continue;
       url_request_summaries.push_back(
-          GetURLRequestSummaryForResource(main_frame_absolute, kv.second));
+          GetURLRequestSummaryForResource(endpoint_path, kv.second));
     }
+    GURL endpoint_url = embedded_test_server()->GetURL(endpoint_path);
+    GURL main_frame_url = embedded_test_server()->GetURL(main_frame_path);
     ResourcePrefetchPredictorTestObserver observer(
-        predictor_, UpdateAndGetVisitCount(main_frame_relative),
-        CreatePageRequestSummary(main_frame_absolute.spec(),
-                                 main_frame_absolute.spec(),
+        predictor_, UpdateAndGetVisitCount(main_frame_path),
+        CreatePageRequestSummary(endpoint_url.spec(), main_frame_url.spec(),
                                  url_request_summaries));
-    ui_test_utils::NavigateToURL(browser(), main_frame_absolute);
+    ui_test_utils::NavigateToURL(browser(), main_frame_url);
     observer.Wait();
   }
 
-  ResourceSummary* AddResource(const std::string& relative_url,
+  ResourceSummary* AddResource(const std::string& resource_path,

[pasko, 2016/11/21 18:42:39]

should this method be protected?

[alexilin, 2016/11/22 11:04:29]

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.

[pasko, 2016/11/22 15:42:14]

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

[alexilin, 2016/11/22 16:12:35]

Done.

                                content::ResourceType resource_type,
                                net::RequestPriority priority) {
     ResourceSummary resource;
     resource.request.resource_url =
-        embedded_test_server()->GetURL(relative_url);
+        embedded_test_server()->GetURL(resource_path);
     resource.request.resource_type = resource_type;
     resource.request.priority = priority;
-    auto result = resources_.insert(std::make_pair(relative_url, resource));
+    resource.request.has_validators = true;
+    auto result = resources_.insert(std::make_pair(resource_path, resource));

[pasko, 2016/11/21 18:42:39]

should we also assert that we are not inserting the same key twice? checking the
value of result.second basically ..

[alexilin, 2016/11/22 11:04:29]

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.

     return &(result.first->second);

[pasko, 2016/11/21 18:42:39]

'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 :)

[alexilin, 2016/11/22 11:04:29]

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?

[pasko, 2016/11/22 15:42:14]

argh, forgot about DCHECK, yeah, I think EXPECT_TRUE is appropriate

[alexilin, 2016/11/22 16:12:35]

Acknowledged.

   }
 
+  void AddRedirectChain(const std::vector<std::string>& redirect_chain) {

[pasko, 2016/11/21 18:42:39]

protected as well?

[alexilin, 2016/11/22 16:12:35]

Done.

+    ASSERT_GE(redirect_chain.size(), 2U);
+    for (size_t i = 0; i + 1 < redirect_chain.size(); ++i) {

[pasko, 2016/11/21 18:42:39]

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

[alexilin, 2016/11/22 11:04:29]

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.

[pasko, 2016/11/22 15:42:14]

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.

[alexilin, 2016/11/22 16:12:35]

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).

+      auto result = redirects_.insert(
+          std::make_pair(redirect_chain[i], redirect_chain[i + 1]));
+      ASSERT_TRUE(result.second) << redirect_chain[i]
+                                 << " already has a redirect.";
+    }
+  }
+
  private:
   // ResourcePrefetchPredictor needs to be initialized before the navigation
   // happens otherwise this navigation will be ignored by predictor.
@@ -159,19 +175,29 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
   }
 
   URLRequestSummary GetURLRequestSummaryForResource(
-      const GURL& main_frame_url,
+      const std::string& main_frame_path,
       const ResourceSummary& resource_summary) {
     URLRequestSummary summary(resource_summary.request);
     content::WebContents* web_contents =
         browser()->tab_strip_model()->GetActiveWebContents();
     int process_id = web_contents->GetRenderProcessHost()->GetID();
     int frame_id = web_contents->GetMainFrame()->GetRoutingID();
+    GURL main_frame_url = embedded_test_server()->GetURL(main_frame_path);
     summary.navigation_id =
         CreateNavigationID(process_id, frame_id, main_frame_url.spec());
     return summary;
   }
 
-  std::unique_ptr<net::test_server::HttpResponse> HandleRequest(
+  std::string GetRedirectEndpoint(const std::string& path) {
+    const std::string* current = &path;
+    for (auto it = redirects_.find(*current); it != redirects_.end();

[pasko, 2016/11/21 18:42:39]

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);
    }

[alexilin, 2016/11/22 11:04:29]

Done.

+         it = redirects_.find(*current)) {
+      current = &(it->second);
+    }
+    return *current;
+  }
+
+  std::unique_ptr<net::test_server::HttpResponse> HandleResourceRequest(
       const net::test_server::HttpRequest& request) {
     auto resource_it = resources_.find(request.relative_url);
     if (resource_it == resources_.end())
@@ -183,7 +209,8 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     http_response->set_code(net::HTTP_OK);
     if (!summary.request.mime_type.empty())
       http_response->set_content_type(summary.request.mime_type);
-    http_response->set_content(summary.content);
+    if (!summary.content.empty())
+      http_response->set_content(summary.content);
     if (summary.is_no_store)
       http_response->AddCustomHeader("Cache-Control", "no-store");
     if (summary.request.has_validators) {
@@ -198,23 +225,48 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     return std::move(http_response);
   }
 
-  size_t UpdateAndGetVisitCount(const std::string& main_frame_relative) {
-    return ++visit_count_[main_frame_relative];
+  std::unique_ptr<net::test_server::HttpResponse> HandleRedirectRequest(
+      const net::test_server::HttpRequest& request) {
+    auto redirect_it = redirects_.find(request.relative_url);
+    if (redirect_it == redirects_.end())
+      return nullptr;
+
+    auto http_response =
+        base::MakeUnique<net::test_server::BasicHttpResponse>();
+    http_response->set_code(net::HTTP_FOUND);

[Benoit L, 2016/11/21 16:24:25]

Can you also test with a 301?

[alexilin, 2016/11/21 18:47:35]

Done.

+    http_response->AddCustomHeader(
+        "Location", embedded_test_server()->GetURL(redirect_it->second).spec());
+    return std::move(http_response);
+  }
+
+  size_t UpdateAndGetVisitCount(const std::string& main_frame_path) {
+    return ++visit_count_[main_frame_path];
   }
 
   ResourcePrefetchPredictor* predictor_;
   std::map<std::string, ResourceSummary> resources_;
+  std::map<std::string, std::string> redirects_;
   std::map<std::string, size_t> visit_count_;
 };
 
 IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, LearningSimple) {
   // These resources have default priorities that correspond to
   // blink::typeToPriority function.
-  AddResource(kImageUrl, content::RESOURCE_TYPE_IMAGE, net::LOWEST);
-  AddResource(kStyleUrl, content::RESOURCE_TYPE_STYLESHEET, net::HIGHEST);
-  AddResource(kScriptUrl, content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
-  AddResource(kFontUrl, content::RESOURCE_TYPE_FONT_RESOURCE, net::HIGHEST);
-  NavigateToURLAndCheckSubresources(kHtmlSubresourcesUrl);
+  AddResource(kImagePath, content::RESOURCE_TYPE_IMAGE, net::LOWEST);
+  AddResource(kStylePath, content::RESOURCE_TYPE_STYLESHEET, net::HIGHEST);
+  AddResource(kScriptPath, content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
+  AddResource(kFontPath, content::RESOURCE_TYPE_FONT_RESOURCE, net::HIGHEST);
+  NavigateToURLAndCheckSubresources(kHtmlSubresourcesPath);
+}
+
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
+                       LearningAfterRedirects) {
+  AddRedirectChain({kRedirectPath, kRedirectPath2, kHtmlSubresourcesPath});
+  AddResource(kImagePath, content::RESOURCE_TYPE_IMAGE, net::LOWEST);
+  AddResource(kStylePath, content::RESOURCE_TYPE_STYLESHEET, net::HIGHEST);
+  AddResource(kScriptPath, content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
+  AddResource(kFontPath, content::RESOURCE_TYPE_FONT_RESOURCE, net::HIGHEST);
+  NavigateToURLAndCheckSubresources(kRedirectPath);
 }
 
 }  // namespace predictors