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 c9ab1bf7d49b80fd04537fa57badfb65b8920df9..d992c830645426af61a43d8b5343aa0e4f02fc1c 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
@@ -8,12 +8,12 @@
 #include "chrome/browser/predictors/resource_prefetch_predictor_test_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #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"
@@ -91,6 +91,12 @@ class InitializationObserver : public TestObserver {
 using PageRequestSummary = ResourcePrefetchPredictor::PageRequestSummary;
 using URLRequestSummary = ResourcePrefetchPredictor::URLRequestSummary;
 
+struct ResourceComparisonParameters {

[Benoit L, 2016/12/20 09:53:39]

Please remove this struct, it adds unnecessary boilerplate to the code.

[ahemery, 2016/12/20 13:00:00]

Removed

+  bool match_navigation_id;
+
+  ResourceComparisonParameters() : match_navigation_id(true) {}
+};
+
 void RemoveDuplicateSubresources(std::vector<URLRequestSummary>* subresources) {
   std::stable_sort(subresources->begin(), subresources->end(),
                    [](const URLRequestSummary& x, const URLRequestSummary& y) {
@@ -107,14 +113,14 @@ 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");
 }
 
-void ModifySubresourceForComparison(URLRequestSummary* subresource,
-                                    bool match_navigation_id) {
-  if (!match_navigation_id)
+void ModifySubresourceForComparison(
+    URLRequestSummary* subresource,
+    const ResourceComparisonParameters& params) {
+  if (!params.match_navigation_id)
     SetValidNavigationID(&subresource->navigation_id);
   if (subresource->resource_type == content::RESOURCE_TYPE_IMAGE &&
       subresource->priority == net::LOWEST) {
@@ -129,15 +135,15 @@ void ModifySubresourceForComparison(URLRequestSummary* subresource,
 // and fail the test if the expectation is not met.
 void CompareSubresources(std::vector<URLRequestSummary> actual_subresources,
                          std::vector<URLRequestSummary> expected_subresources,
-                         bool match_navigation_id) {
+                         const ResourceComparisonParameters& params) {
   // Duplicate resources can be observed in a single navigation but
   // ResourcePrefetchPredictor only cares about the first occurrence of each.
   RemoveDuplicateSubresources(&actual_subresources);
 
   for (auto& subresource : actual_subresources)
-    ModifySubresourceForComparison(&subresource, match_navigation_id);
+    ModifySubresourceForComparison(&subresource, params);
   for (auto& subresource : expected_subresources)
-    ModifySubresourceForComparison(&subresource, match_navigation_id);
+    ModifySubresourceForComparison(&subresource, params);
 
   EXPECT_THAT(actual_subresources,
               testing::UnorderedElementsAreArray(expected_subresources));
@@ -156,11 +162,11 @@ class ResourcePrefetchPredictorTestObserver : public TestObserver {
       ResourcePrefetchPredictor* predictor,
       const size_t expected_url_visit_count,
       const PageRequestSummary& expected_summary,
-      bool match_navigation_id)
+      const ResourceComparisonParameters& resource_comparison_params)
       : TestObserver(predictor),
         url_visit_count_(expected_url_visit_count),
         summary_(expected_summary),
-        match_navigation_id_(match_navigation_id) {}
+        resource_comparison_params_(resource_comparison_params) {}
 
   // TestObserver:
   void OnNavigationLearned(size_t url_visit_count,
@@ -168,18 +174,26 @@ class ResourcePrefetchPredictorTestObserver : 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_[resource.navigation_id];
     CompareSubresources(summary.subresource_requests,
-                        summary_.subresource_requests, match_navigation_id_);
+                        summary_.subresource_requests,
+                        resource_comparison_params_);
     run_loop_.Quit();
   }
 
   void Wait() { run_loop_.Run(); }
 
+  std::map<NavigationID, int>* GetNavigationIds() {

[Benoit L, 2016/12/20 09:53:39]

From the Chromium C++ style guide:
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

"Simple accessors should generally be the only inline functions. These should be
named unix_hacker_style()."

Meaning that this should be:
navigation_ids().

Also, I would expect this to be a set, from the naming.
Perhaps resource_count_by_navigation_id?

Finally, this can return a non-const reference. Please change the code to do so.

[ahemery, 2016/12/20 13:00:00]

Renamed and updated types to be more precise

+    return &current_navigation_ids_;
+  }
+
  private:
   base::RunLoop run_loop_;
   size_t url_visit_count_;
   PageRequestSummary summary_;
-  bool match_navigation_id_;
+  ResourceComparisonParameters resource_comparison_params_;
+  std::map<NavigationID, int> current_navigation_ids_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictorTestObserver);
 };
@@ -195,6 +209,10 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
   }
 
   void SetUpOnMainThread() override {
+    SetCurrentHostName("127.0.0.1");
+    host_resolver()->AddRule("*", "127.0.0.1");
+    embedded_test_server()->AddDefaultHandlers(

[alexilin, 2016/12/20 14:06:47]

It is not necessary to do.
This is already done in InProcessBrowserTest constructor
https://cs.chromium.org/chromium/src/chrome/test/base/in_process_browser_test...

[ahemery, 2016/12/20 14:44:56]

Done in patch 5

+        base::FilePath(FILE_PATH_LITERAL("chrome/test/data")));

[ahemery, 2016/12/20 13:00:00]

Removed as this was unnecessary

     embedded_test_server()->RegisterRequestHandler(
         base::Bind(&ResourcePrefetchPredictorBrowserTest::HandleRedirectRequest,
                    base::Unretained(this)));
@@ -219,15 +237,26 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
       url_request_summaries.push_back(
           GetURLRequestSummaryForResource(endpoint_url, kv.second));
     }
+
+    ResourceComparisonParameters resource_comparison_params;
+    if (disposition != WindowOpenDisposition::CURRENT_TAB)
+      resource_comparison_params.match_navigation_id = false;
+
     ResourcePrefetchPredictorTestObserver observer(
         predictor_, UpdateAndGetVisitCount(main_frame_url),
         CreatePageRequestSummary(endpoint_url.spec(), main_frame_url.spec(),
                                  url_request_summaries),
-        true);  // Matching navigation id by default
+        resource_comparison_params);
     ui_test_utils::NavigateToURLWithDisposition(
         browser(), main_frame_url, disposition,
         ui_test_utils::BROWSER_TEST_NONE);
     observer.Wait();
+    for (auto& kv : resources_) {
+      if (!kv.second.is_no_store && kv.second.should_be_recorded)
+        kv.second.request.was_cached = true;
+    }
+    for (const auto& nav : *observer.GetNavigationIds())
+      ++navigation_id_history_[nav.first];
   }
 
   ResourceSummary* AddResource(const GURL& resource_url,
@@ -273,9 +302,17 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     }
   }
 
+  void ClearResources() { resources_.clear(); }
+
+  void ClearCache() {
+    chrome::ClearCache(browser());
+    for (auto& kv : resources_)
+      kv.second.request.was_cached = false;
+  }
+
   // 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);

[alexilin, 2016/12/20 14:06:47]

While I find that the ability to set arbitrary host for a request is useful, I
also think that we need to change the approach.
Because, for example, it won't work with https server. You will have to pass the
right host name along all https_server()->GetURL() calls. It works now because
in that test we use default host but if you add SetCurrentHostName("x.y") in the
beginning of the test it is broken.

Moreover it adds unobvious hidden dependencies:
  GURL a = GetURL("/a");
  SetCurrentHostName("x.y");
  GURL b = GetURL("/a");
  EXPECT_EQ(a, b); // fails

I would suggest you to have immutable host for each embedded_test_server
instance. That is if you want to have different host for the server you just
create a new one and pass desired host string into the constructor.
You could modify existing EnableHttpsServer() function to be a factory method
for new servers.
You also could add host name parameter to
HandleRedirectRequest/HandleResourceRequest and bind it to the value passed into
the constructor.

Then CrossSiteLearning test will look like that (perhaps you can come up with a
better idea for getting url):

std::unique_ptr<net::EmbeddedTestServer> foo_server =
CreateAndRunTestServer("foo.com", net::EmbeddedTestServer::TYPE_HTTP);
// add resources
NavigateToURLAndCheckSubresources(foo_server->GetURL("foo.com",
kHtmlSubresourcesPath));
ClearResources();

std::unique_ptr<net::EmbeddedTestServer> bar_server =
CreateAndRunTestServer("bar.com", net::EmbeddedTestServer::TYPE_HTTP);
// add resources
NavigateToURLAndCheckSubresources(bar_server->GetURL("bar.com",
kHtmlSubresourcesPath));

   }
 
   void EnableHttpsServer() {
@@ -301,6 +338,12 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
 
   net::EmbeddedTestServer* https_server() { return https_server_.get(); }
 
+  void SetCurrentHostName(const std::string& new_host) {

[Benoit L, 2016/12/20 09:53:39]

nit: set_current_host_name()

[ahemery, 2016/12/20 13:00:00]

Done

+    current_host_name_ = new_host;
+  }
+
+  unsigned int GetHistoryCount() { return navigation_id_history_.size(); }

[Benoit L, 2016/12/20 09:53:39]

map::size() returns a size_t, not an unsigned int.

[ahemery, 2016/12/20 13:00:00]

Done

+
  private:
   // ResourcePrefetchPredictor needs to be initialized before the navigation
   // happens otherwise this navigation will be ignored by predictor.
@@ -324,10 +367,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;
   }
 
@@ -344,7 +385,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();

[alexilin, 2016/12/20 14:06:47]

It would be better to extract this to 
GURL ReplaceHost(const GURL& x) function

+    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;
 
@@ -390,11 +436,26 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     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::map<NavigationID, int> navigation_id_history_;
+};
+
+// TODO(ahemery): Remove when https://codereview.chromium.org/2553083002/

[Benoit L, 2016/12/20 09:53:39]

You can rebase the patch now, as the CL mentioned here has landed.

Also, please don't mention people by name. It's ambiguous, here alexilin@ would
be better, or just drop it, as the CL number is enough.

Also, crrev.com/something is quite useful. "Something" can be an issue number or
commit hash. Doesn't make a difference of course, just nice to know.

[ahemery, 2016/12/20 13:00:00]

Noted, will use logins instead from now on. Regarding the code itself, there was
a confusion about what alexilin@ code was doing. This is still needed.

+// is landed by Alex. LearningObserver should be used by default.
+// His change will disable actual prefetching by default
+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, LearningSimple) {
@@ -529,4 +590,54 @@ IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
   NavigateToURLAndCheckSubresources(GetURL(kHtmlIframePath));
 }
 
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
+                       CrossSiteLearning) {
+  SetCurrentHostName("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);
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath));
+  ClearResources();
+
+  SetCurrentHostName("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);
+
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath));
+}
+
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorLearningBrowserTest,
+                       TabIdBehavingAsExpected) {

[alexilin, 2016/12/20 14:06:47]

Session id?

[ahemery, 2016/12/20 14:44:56]

Done.

+  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(GetHistoryCount(), 1U);
+  ClearCache();
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath));
+  EXPECT_EQ(GetHistoryCount(), 1U);
+  ClearCache();
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath),
+                                    WindowOpenDisposition::NEW_FOREGROUND_TAB);

[alexilin, 2016/12/20 14:06:47]

It would be worth to add some comment about and what we are testing here what
behavior we are expecting.

[ahemery, 2016/12/20 14:44:56]

Done.

+  EXPECT_EQ(GetHistoryCount(), 2U);
+  ClearCache();
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath),
+                                    WindowOpenDisposition::NEW_WINDOW);
+  EXPECT_EQ(GetHistoryCount(), 3U);
+  ClearCache();
+  NavigateToURLAndCheckSubresources(GetURL(kHtmlSubresourcesPath),
+                                    WindowOpenDisposition::NEW_POPUP);
+  EXPECT_EQ(GetHistoryCount(), 4U);
+}
+
 }  // namespace predictors