Rework of ResourcePrefetchPredictor browser tests infrastructure.

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 91e3d93786e17c3e8a87925d0a4301fed1fa73b3..e3000030161f111c95ed2198ec951cabc6e99ca9 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
@@ -107,6 +107,35 @@ std::vector<URLRequestSummary> GetUniqueSubresources(
   return subresources;
 }
 
+// Fill a NavigationID with "empty" data that does not trigger
+// the is_valid DCHECK(). Allows comparing.
+void EmptyNavigationId(NavigationID* navigation_id) {

[Benoit L, 2016/12/08 15:07:44]

nit: I think it would be clearer to change this to

void SetValidNavigationID(URLRequestSummary* summary);

[ahemery, 2016/12/09 14:19:16]

Changed

+  navigation_id->render_process_id = 0;
+  navigation_id->render_frame_id = 0;
+  navigation_id->main_frame_url = GURL("http://127.0.0.1");
+}
+
+// Does a custom comparison of subresources of URLRequestSummary
+// and fail the test if the expectation is not met.
+void CompareSubresources(const PageRequestSummary& actual_summary,
+                         const PageRequestSummary& expected_summary,
+                         bool match_navigation_id) {
+  // Duplicate resources can be observed in a single navigation but
+  // ResourcePrefetchPredictor only cares about the first occurrence of each.
+  std::vector<ResourcePrefetchPredictor::URLRequestSummary>
+      actual_subresources = GetUniqueSubresources(actual_summary);
+  std::vector<ResourcePrefetchPredictor::URLRequestSummary>
+      expected_subresources(expected_summary.subresource_requests);
+  if (!match_navigation_id) {
+    for (auto& subresource : actual_subresources)
+      EmptyNavigationId(&subresource.navigation_id);
+    for (auto& subresource : expected_subresources)
+      EmptyNavigationId(&subresource.navigation_id);
+  }
+  EXPECT_THAT(actual_subresources,
+              testing::UnorderedElementsAreArray(expected_subresources));
+}
+
 }  // namespace
 
 // Helper class to track and allow waiting for ResourcePrefetchPredictor events.
@@ -119,10 +148,12 @@ class ResourcePrefetchPredictorTestObserver : public TestObserver {
   explicit ResourcePrefetchPredictorTestObserver(
       ResourcePrefetchPredictor* predictor,
       const size_t expected_url_visit_count,
-      const PageRequestSummary& expected_summary)
+      const PageRequestSummary& expected_summary,
+      bool match_navigation_id)
       : TestObserver(predictor),
         url_visit_count_(expected_url_visit_count),
-        summary_(expected_summary) {}
+        summary_(expected_summary),
+        match_navigation_id_(match_navigation_id) {}
 
   // TestObserver:
   void OnNavigationLearned(size_t url_visit_count,
@@ -130,12 +161,7 @@ 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);
-    // Duplicate resources can be observed in a single navigation but
-    // ResourcePrefetchPredictor only cares about the first occurrence of each.
-    std::vector<ResourcePrefetchPredictor::URLRequestSummary> subresources =
-        GetUniqueSubresources(summary);
-    EXPECT_THAT(subresources, testing::UnorderedElementsAreArray(
-                                  summary_.subresource_requests));
+    CompareSubresources(summary, summary_, match_navigation_id_);

[alexilin, 2016/12/08 12:57:25]

nit: I found a little be confusing that the function CompareSubresources accepts
summaries.
It would be probably better to pass there two vectors of resources explicitly.
(Feel free to change GetUniqueSubresources function as well)

[ahemery, 2016/12/09 14:19:16]

Modified. The copy is now done by parameters and duplicate removal happens in
place.

     run_loop_.Quit();
   }
 
@@ -145,6 +171,7 @@ class ResourcePrefetchPredictorTestObserver : public TestObserver {
   base::RunLoop run_loop_;
   size_t url_visit_count_;
   PageRequestSummary summary_;
+  bool match_navigation_id_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictorTestObserver);
 };
@@ -173,7 +200,9 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
     EnsurePredictorInitialized();
   }
 
-  void NavigateToURLAndCheckSubresources(const GURL& main_frame_url) {
+  void NavigateToURLAndCheckSubresources(
+      const GURL& main_frame_url,
+      WindowOpenDisposition disposition = WindowOpenDisposition::CURRENT_TAB) {
     GURL endpoint_url = GetRedirectEndpoint(main_frame_url);
     std::vector<URLRequestSummary> url_request_summaries;
     for (const auto& kv : resources_) {
@@ -182,11 +211,16 @@ class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
       url_request_summaries.push_back(
           GetURLRequestSummaryForResource(endpoint_url, kv.second));
     }
+    bool match_navigation_id =
+        (disposition == WindowOpenDisposition::CURRENT_TAB);
     ResourcePrefetchPredictorTestObserver observer(
         predictor_, UpdateAndGetVisitCount(main_frame_url),
         CreatePageRequestSummary(endpoint_url.spec(), main_frame_url.spec(),
-                                 url_request_summaries));
-    ui_test_utils::NavigateToURL(browser(), main_frame_url);
+                                 url_request_summaries),
+        match_navigation_id);
+    ui_test_utils::NavigateToURLWithDisposition(
+        browser(), main_frame_url, disposition,
+        ui_test_utils::BROWSER_TEST_NONE);
     observer.Wait();
   }
 
@@ -489,4 +523,26 @@ IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest,
   NavigateToURLAndCheckSubresources(GetURL(kHtmlIframePath));
 }
 
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, LearningInNewTab) {

[Benoit L, 2016/12/08 15:07:44]

It's not entirely clear to me that these tests do something different from the
existing ones, from looking at the code in ui_test_utils.

What's the aim here?

[ahemery, 2016/12/09 14:19:16]

It does not test any specific behavior but is more about making sure nothing
crazy happens for edge cases. In particular that navigating to a new tab or a
new popup does not crash. Should help specifically when moving to tab_ids. 

[Benoit L, 2016/12/09 14:44:01]

Ok, reading through the code in ui_test_utils seems to confirm that this indeed
does something different.

Can you hold off landing this CL until there is the actual codefor the tab id
stuff in?

The reasoning being that introducing tests that are not (yet) useful is not
(yet) useful.

+  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),
+                                    WindowOpenDisposition::NEW_FOREGROUND_TAB);
+}
+
+IN_PROC_BROWSER_TEST_F(ResourcePrefetchPredictorBrowserTest, LearningInPopup) {
+  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),
+                                    WindowOpenDisposition::NEW_POPUP);
+}
+
 }  // namespace predictors