Rework of ResourcePrefetchPredictor browser tests infrastructure.

chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/command_line.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor_factory.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor_test_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
@@ skipping 89 matching lines @@
                    });
   subresources.erase(
       std::unique(subresources.begin(), subresources.end(),
                   [](const URLRequestSummary& x, const URLRequestSummary& y) {
                     return x.resource_url == y.resource_url;
                   }),
       subresources.end());
   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.
 // These events are also used to verify that ResourcePrefetchPredictor works as
 // expected.
 class ResourcePrefetchPredictorTestObserver : public TestObserver {
  public:
   using PageRequestSummary = ResourcePrefetchPredictor::PageRequestSummary;
 
   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,
                            const PageRequestSummary& summary) override {
     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();
   }
 
   void Wait() { run_loop_.Run(); }
 
  private:
   base::RunLoop run_loop_;
   size_t url_visit_count_;
   PageRequestSummary summary_;
+  bool match_navigation_id_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictorTestObserver);
 };
 
 class ResourcePrefetchPredictorBrowserTest : public InProcessBrowserTest {
  protected:
   using URLRequestSummary = ResourcePrefetchPredictor::URLRequestSummary;
 
   void SetUpCommandLine(base::CommandLine* command_line) override {
     command_line->AppendSwitchASCII(
         switches::kSpeculativeResourcePrefetching,
         switches::kSpeculativeResourcePrefetchingEnabled);
   }
 
   void SetUpOnMainThread() override {
     embedded_test_server()->RegisterRequestHandler(
         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_ =
         ResourcePrefetchPredictorFactory::GetForProfile(browser()->profile());
     ASSERT_TRUE(predictor_);
     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_) {
       if (kv.second.is_no_store || !kv.second.should_be_recorded)
         continue;
       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();
   }
 
   ResourceSummary* AddResource(const GURL& resource_url,
                                content::ResourceType resource_type,
                                net::RequestPriority priority) {
     auto pair_and_whether_inserted =
         resources_.insert(std::make_pair(resource_url, ResourceSummary()));
     EXPECT_TRUE(pair_and_whether_inserted.second) << resource_url
                                                   << " was inserted twice";
@@ skipping 282 matching lines @@
   AddResource(GetURL(kImagePath2), content::RESOURCE_TYPE_IMAGE, net::LOWEST);
   AddResource(GetURL(kStylePath2), content::RESOURCE_TYPE_STYLESHEET,
               net::HIGHEST);
   AddResource(GetURL(kScriptPath2), content::RESOURCE_TYPE_SCRIPT, net::MEDIUM);
   // Included from <iframe src="html_subresources.html"> and not recored.
   AddUnrecordedResources({GetURL(kImagePath), GetURL(kStylePath),
                           GetURL(kScriptPath), GetURL(kFontPath)});
   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