predictors: Refactor resource_prefetch_predictor triggering.

chrome/browser/predictors/resource_prefetch_predictor_unittest.cc

Index: chrome/browser/predictors/resource_prefetch_predictor_unittest.cc
diff --git a/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc b/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc
index c3694699a9c997b73dde2a984c98d250af844fe3..b766a2619a3bb9556d8df2595c24fd9f8f385d5e 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc
@@ -16,6 +16,7 @@
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor_tables.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor_test_util.h"
+#include "chrome/browser/predictors/resource_prefetcher_manager.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/history/core/browser/history_types.h"
@@ -37,6 +38,11 @@ using testing::UnorderedElementsAre;
 
 namespace predictors {
 
+namespace {
+constexpr int kDefaultMode = LoadingPredictorConfig::LEARNING |
+                             LoadingPredictorConfig::PREFETCHING_FOR_EXTERNAL;
+}  // namespace
+
 using URLRequestSummary = ResourcePrefetchPredictor::URLRequestSummary;
 using PageRequestSummary = ResourcePrefetchPredictor::PageRequestSummary;
 using PrefetchDataMap = ResourcePrefetchPredictorTables::PrefetchDataMap;
@@ -174,6 +180,20 @@ class MockResourcePrefetchPredictorObserver : public TestObserver {
            const ResourcePrefetchPredictor::PageRequestSummary& summary));
 };
 
+// Does nothing when a prefetch starts.
+class MockResourcePrefetcherManager : public ResourcePrefetcherManager {
+ public:
+  using ResourcePrefetcherManager::ResourcePrefetcherManager;
+  MOCK_METHOD2(MaybeAddPrefetch,
+               void(const GURL& main_frame_url, const std::vector<GURL>& urls));
+
+ protected:
+  ~MockResourcePrefetcherManager() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockResourcePrefetcherManager);
+};
+
 class ResourcePrefetchPredictorTest : public testing::Test {
  public:
   ResourcePrefetchPredictorTest();
@@ -227,9 +247,17 @@ class ResourcePrefetchPredictorTest : public testing::Test {
     base::RunLoop loop;
     loop.RunUntilIdle();  // Runs the DB lookup.
     profile_->BlockUntilHistoryProcessesPendingRequests();
+
+    mock_prefetch_manager_ = nullptr;
+    if (predictor_->prefetch_manager_) {
+      mock_prefetch_manager_ = new StrictMock<MockResourcePrefetcherManager>(
+          predictor_.get(), predictor_->config_,
+          predictor_->profile_->GetRequestContext());
+      predictor_->set_mock_resource_prefetcher_manager(mock_prefetch_manager_);
+    }
   }
 
-  void ResetPredictor() {
+  void ResetPredictor(int mode = kDefaultMode) {

[alexilin, 2017/05/23 11:01:39]

It changes the current behavior of tests. Why not leave the default mode and
enable PREFETCHING if it's needed? 

[Benoit L, 2017/05/29 16:45:14]

Done.

     LoadingPredictorConfig config;
     config.max_urls_to_track = 3;
     config.max_hosts_to_track = 2;
@@ -241,9 +269,10 @@ class ResourcePrefetchPredictorTest : public testing::Test {
     config.is_url_learning_enabled = true;
     config.is_manifests_enabled = true;
     config.is_origin_learning_enabled = true;
+    config.mode = mode;
 
-    config.mode |= LoadingPredictorConfig::LEARNING;
-    predictor_.reset(new ResourcePrefetchPredictor(config, profile_.get()));
+    predictor_ =
+        base::MakeUnique<ResourcePrefetchPredictor>(config, profile_.get());
     predictor_->set_mock_tables(mock_tables_);
   }
 
@@ -261,6 +290,8 @@ class ResourcePrefetchPredictorTest : public testing::Test {
 
   std::unique_ptr<ResourcePrefetchPredictor> predictor_;
   scoped_refptr<StrictMock<MockResourcePrefetchPredictorTables>> mock_tables_;
+  scoped_refptr<StrictMock<MockResourcePrefetcherManager>>
+      mock_prefetch_manager_;
 
   PrefetchDataMap test_url_data_;
   PrefetchDataMap test_host_data_;
@@ -2095,24 +2126,16 @@ TEST_F(ResourcePrefetchPredictorTest,
 }
 
 TEST_F(ResourcePrefetchPredictorTest, TestPrefetchingDurationHistogram) {
-  // Prefetching duration for an url without resources in the database

[alexilin, 2017/05/23 11:01:39]

Why changing this test?
We wanted to record prefetching duration even when prefetching is disabled.

-  // shouldn't be recorded.
-  const std::string main_frame_url = "http://google.com/?query=cats";
-  predictor_->StartPrefetching(GURL(main_frame_url), HintOrigin::EXTERNAL);
-  predictor_->StopPrefetching(GURL(main_frame_url));
-  histogram_tester_->ExpectTotalCount(
-      internal::kResourcePrefetchPredictorPrefetchingDurationHistogram, 0);
-
   // Fill the database to record a duration.
-  PrefetchData google = CreatePrefetchData("google.com", 1);
-  InitializeResourceData(
-      google.add_resources(), "https://cdn.google.com/script.js",
-      content::RESOURCE_TYPE_SCRIPT, 10, 0, 1, 2.1, net::MEDIUM, false, false);
-  predictor_->host_table_cache_->insert(
-      std::make_pair(google.primary_key(), google));
+  for (const auto& kv : test_host_data_)

[alexilin, 2017/05/23 11:01:39]

nit:
predictor_->host_table_cache_ = test_host_data_;

+    predictor_->host_table_cache_->insert(kv);
 
-  predictor_->StartPrefetching(GURL(main_frame_url), HintOrigin::EXTERNAL);
-  predictor_->StopPrefetching(GURL(main_frame_url));
+  using testing::_;
+  EXPECT_CALL(*mock_prefetch_manager_.get(), MaybeAddPrefetch(_, _));
+
+  const GURL url = GURL("http://www.facebook.com/cats");
+  predictor_->StartPrefetching(url, HintOrigin::EXTERNAL);
+  predictor_->StopPrefetching(url);
   histogram_tester_->ExpectTotalCount(
       internal::kResourcePrefetchPredictorPrefetchingDurationHistogram, 1);
 }
@@ -2170,4 +2193,34 @@ TEST_F(ResourcePrefetchPredictorTest, TestRecordFirstContentfulPaint) {
   profile_->BlockUntilHistoryProcessesPendingRequests();
 }
 
+TEST_F(ResourcePrefetchPredictorTest, TestCanPrefetchUrlForOrigin) {
+  for (const auto& kv : test_host_data_)

[alexilin, 2017/05/23 11:01:39]

ditto

+    predictor_->host_table_cache_->insert(kv);
+
+  const GURL url = GURL("http://www.facebook.com/cats");
+  EXPECT_TRUE(predictor_->CanPrefetchUrlForOrigin(url, HintOrigin::EXTERNAL));
+
+  // Unknown host.
+  EXPECT_FALSE(predictor_->CanPrefetchUrlForOrigin(GURL("https://unknown.com"),
+                                                   HintOrigin::EXTERNAL));
+  // Wrong origin.
+  EXPECT_FALSE(
+      predictor_->CanPrefetchUrlForOrigin(url, HintOrigin::NAVIGATION));
+
+  // Prefetching not enabled.
+  EXPECT_CALL(*mock_tables_.get(),
+              GetAllData(Pointee(ContainerEq(PrefetchDataMap())),
+                         Pointee(ContainerEq(PrefetchDataMap())),
+                         Pointee(ContainerEq(RedirectDataMap())),
+                         Pointee(ContainerEq(RedirectDataMap())),
+                         Pointee(ContainerEq(ManifestDataMap())),
+                         Pointee(ContainerEq(OriginDataMap()))));
+  ResetPredictor(LoadingPredictorConfig::LEARNING);
+  InitializePredictor();
+  for (const auto& kv : test_host_data_)

[alexilin, 2017/05/23 11:01:39]

ditto

+    predictor_->host_table_cache_->insert(kv);
+
+  EXPECT_FALSE(predictor_->CanPrefetchUrlForOrigin(url, HintOrigin::EXTERNAL));
+}
+
 }  // namespace predictors