predictors: Make speculative_prefetch_predictor work with PlzNavigate

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 70c3131eee72b3618091e2b5e499350e753622f6..e804298cf98bf161cd05a02616c28b28dce068a7 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
@@ -2,6 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <cstddef>
+#include <set>
+
 #include "base/command_line.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor_factory.h"
@@ -13,8 +16,7 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
-#include "content/public/browser/render_frame_host.h"
-#include "content/public/browser/render_process_host.h"
+#include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/http_request.h"
 #include "net/test/embedded_test_server/http_response.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -111,8 +113,7 @@ void RemoveDuplicateSubresources(std::vector<URLRequestSummary>* subresources) {
 // Fill a NavigationID with "empty" data that does not trigger
 // the is_valid DCHECK(). Allows comparing.
 void SetValidNavigationID(NavigationID* navigation_id) {
-  navigation_id->render_process_id = 0;
-  navigation_id->render_frame_id = 0;
+  navigation_id->session_id = 0;
   navigation_id->main_frame_url = GURL("http://127.0.0.1");
 }
 
@@ -171,6 +172,8 @@ class LearningObserver : public TestObserver {
     EXPECT_EQ(url_visit_count, url_visit_count_);
     EXPECT_EQ(summary.main_frame_url, summary_.main_frame_url);
     EXPECT_EQ(summary.initial_url, summary_.initial_url);
+    for (const auto& resource : summary.subresource_requests)
+      current_navigation_ids_.insert(resource.navigation_id);
     CompareSubresources(summary.subresource_requests,
                         summary_.subresource_requests, match_navigation_id_);
     run_loop_.Quit();
@@ -178,11 +181,14 @@ class LearningObserver : public TestObserver {
 
   void Wait() { run_loop_.Run(); }
 
+  std::set<NavigationID>& navigation_ids() { return current_navigation_ids_; }

[Benoit L, 2016/12/20 13:20:49]

nit: This is not enforced by the style guide, but I prefer it when the accessor
has the same name as the member variable (minus the trailing _).

[ahemery, 2016/12/20 14:06:35]

Done.

+
  private:
   base::RunLoop run_loop_;
   size_t url_visit_count_;
   PageRequestSummary summary_;
   bool match_navigation_id_;
+  std::set<NavigationID> current_navigation_ids_;
 
   DISALLOW_COPY_AND_ASSIGN(LearningObserver);
 };
@@ -226,6 +232,8 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
   }
 
   void SetUpOnMainThread() override {
+    set_current_host_name("127.0.0.1");
+    host_resolver()->AddRule("*", "127.0.0.1");

[Benoit L, 2016/12/20 13:20:49]

nit: Can you add a comment saying why this is needed?
I guess it's for multi-origin loads, right?

[ahemery, 2016/12/20 14:06:35]

Done.

     embedded_test_server()->RegisterRequestHandler(
         base::Bind(&ResourcePrefetchPredictorBrowserTest::HandleRedirectRequest,
                    base::Unretained(this)));
@@ -263,11 +271,16 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
       url_request_summaries.push_back(
           GetURLRequestSummaryForResource(endpoint_url, kv.second));
     }
+
+    bool match_navigation_id = true;
+    if (disposition != WindowOpenDisposition::CURRENT_TAB)
+      match_navigation_id = false;
+
     LearningObserver observer(
         predictor_, UpdateAndGetVisitCount(main_frame_url),
         CreatePageRequestSummary(endpoint_url.spec(), main_frame_url.spec(),
                                  url_request_summaries),
-        true);  // Matching navigation id by default
+        match_navigation_id);
     ui_test_utils::NavigateToURLWithDisposition(
         browser(), main_frame_url, disposition,
         ui_test_utils::BROWSER_TEST_NONE);
@@ -276,6 +289,8 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
       if (!kv.second.is_no_store && kv.second.should_be_recorded)
         kv.second.request.was_cached = true;
     }
+    for (const auto& nav : observer.navigation_ids())
+      navigation_id_history_.insert(nav);
   }
 
   void PrefetchURL(const GURL& main_frame_url) {
@@ -331,6 +346,8 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     }
   }
 
+  void ClearResources() { resources_.clear(); }
+
   void ClearCache() {
     chrome::ClearCache(browser());
     for (auto& kv : resources_)
@@ -339,7 +356,7 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
 
   // Shortcut for convenience.
   GURL GetURL(const std::string& path) const {
-    return embedded_test_server()->GetURL(path);
+    return embedded_test_server()->GetURL(current_host_name_, path);
   }
 
   void EnableHttpsServer() {
@@ -365,6 +382,14 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
 
   net::EmbeddedTestServer* https_server() { return https_server_.get(); }
 
+  void set_current_host_name(const std::string& new_host) {
+    current_host_name_ = new_host;
+  }
+
+  std::size_t get_navigation_ids_history_size() {

[Benoit L, 2016/12/20 13:20:49]

nit: getters don't have the initial "get_" in the name.

[ahemery, 2016/12/20 14:06:35]

Done.

+    return navigation_id_history_.size();
+  }
+
  private:
   // ResourcePrefetchPredictor needs to be initialized before the navigation
   // happens otherwise this navigation will be ignored by predictor.
@@ -388,10 +413,8 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     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();
     summary.navigation_id =
-        CreateNavigationID(process_id, frame_id, main_frame_url.spec());
+        NavigationID(web_contents, main_frame_url, base::TimeTicks::Now());
     return summary;
   }
 
@@ -408,7 +431,12 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
 
   std::unique_ptr<net::test_server::HttpResponse> HandleResourceRequest(
       const net::test_server::HttpRequest& request) const {
-    auto resource_it = resources_.find(request.GetURL());
+    GURL resource_url = request.GetURL();
+    GURL::Replacements replace_host;
+    replace_host.SetHostStr(current_host_name_);
+    resource_url = resource_url.ReplaceComponents(replace_host);
+
+    auto resource_it = resources_.find(resource_url);
     if (resource_it == resources_.end())
       return nullptr;
 
@@ -450,15 +478,30 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     return std::move(http_response);
   }
 
-  size_t UpdateAndGetVisitCount(const GURL& main_frame_url) {
+  std::size_t UpdateAndGetVisitCount(const GURL& main_frame_url) {
     return ++visit_count_[main_frame_url];
   }
 
+  std::string current_host_name_;
   ResourcePrefetchPredictor* predictor_;
   std::unique_ptr<net::EmbeddedTestServer> https_server_;
   std::map<GURL, ResourceSummary> resources_;
   std::map<GURL, RedirectEdge> redirects_;
   std::map<GURL, size_t> visit_count_;
+  std::set<NavigationID> navigation_id_history_;
+};
+
+// This browser test override is specifically used to have ONLY

[Benoit L, 2016/12/20 13:20:49]

What is the issue here?

[ahemery, 2016/12/20 14:06:35]

After more than 2 calls to NavigateToURLAndCheckSubresources(...) the predictor
actually starts prefetching the resources, tagging them as "was_cached = true"
and causing mismatches in the subsequent calls. Alternative would be another
boolean match_caching_information. We would still need ClearCache() calls
because server caching is trickier (fills in mime type, etc.)

[Benoit L, 2016/12/21 12:38:25]

Not is you set the flags to "external only". This flag can be set everywhere,
making the code simpler.

[alexilin, 2016/12/21 13:08:12]

To clarify: are you saying that we can change this switch directly in the test
code? I'm not sure that this is true, because we have to set it before the
ResourcePrefetchPredictor creation.

+// the learning part of the prefetcher without the actual prefetching
+// Used to avoid having caching issues for matcher
+class ResourcePrefetchPredictorLearningBrowserTest
+    : public ResourcePrefetchPredictorBrowserTest {
+ protected:
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitchASCII(
+        switches::kSpeculativeResourcePrefetching,
+        switches::kSpeculativeResourcePrefetchingLearning);
+  }
 };
 
 IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, Simple) {
@@ -591,4 +634,53 @@ IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
   TestLearningAndPrefetching(GetURL(kHtmlIframePath));
 }
 
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
+                       CrossSiteLearning) {
+  set_current_host_name("foo.com");
+  AddResource(GetURL(kImagePath), content::RESOURCE_TYPE_IMAGE, net::LOWEST);
+  AddResource(GetURL(kStylePath), content::RESOURCE_TYPE_STYLESHEET,
+              net::HIGHEST);
+  AddResource(GetURL(kScriptPath), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
+  AddResource(GetURL(kFontPath), content::RESOURCE_TYPE_FONT_RESOURCE,
+              net::HIGHEST);
+  TestLearningAndPrefetching(GetURL(kHtmlSubresourcesPath));
+  ClearResources();
+
+  set_current_host_name("bar.com");
+  AddResource(GetURL(kImagePath), content::RESOURCE_TYPE_IMAGE, net::LOWEST);
+  AddResource(GetURL(kStylePath), content::RESOURCE_TYPE_STYLESHEET,
+              net::HIGHEST);
+  AddResource(GetURL(kScriptPath), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
+  AddResource(GetURL(kFontPath), content::RESOURCE_TYPE_FONT_RESOURCE,
+              net::HIGHEST);
+  TestLearningAndPrefetching(GetURL(kHtmlSubresourcesPath));
+}
+
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorLearningBrowserTest,
+                       TabIdBehavingAsExpected) {
+  AddResource(GetURL(kImagePath), content::RESOURCE_TYPE_IMAGE, net::LOWEST);
+  AddResource(GetURL(kStylePath), content::RESOURCE_TYPE_STYLESHEET,
+              net::HIGHEST);
+  AddResource(GetURL(kScriptPath), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
+  AddResource(GetURL(kFontPath), content::RESOURCE_TYPE_FONT_RESOURCE,
+              net::HIGHEST);
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath));
+  EXPECT_EQ(get_navigation_ids_history_size(), 1U);
+  ClearCache();
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath));
+  EXPECT_EQ(get_navigation_ids_history_size(), 1U);
+  ClearCache();
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath),
+                                    WindowOpenDisposition::NEW_FOREGROUND_TAB);
+  EXPECT_EQ(get_navigation_ids_history_size(), 2U);
+  ClearCache();
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath),
+                                    WindowOpenDisposition::NEW_WINDOW);
+  EXPECT_EQ(get_navigation_ids_history_size(), 3U);
+  ClearCache();
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath),
+                                    WindowOpenDisposition::NEW_POPUP);
+  EXPECT_EQ(get_navigation_ids_history_size(), 4U);
+}
+
 }  // namespace predictors