predictors: Make ResourcePrefetchPredictor observable.

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 61d34a59d449d71f1b48b2fa67a8e32863b74181..804ced897c589a9930dbe9fd93427e0e2721cdbd 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc
+++ b/chrome/browser/predictors/resource_prefetch_predictor_unittest.cc
@@ -35,9 +35,10 @@ using testing::UnorderedElementsAre;
 
 namespace predictors {
 
-typedef ResourcePrefetchPredictor::URLRequestSummary URLRequestSummary;
-typedef ResourcePrefetchPredictorTables::PrefetchDataMap PrefetchDataMap;
-typedef ResourcePrefetchPredictorTables::RedirectDataMap RedirectDataMap;
+using URLRequestSummary = ResourcePrefetchPredictor::URLRequestSummary;
+using PageRequestSummary = ResourcePrefetchPredictor::PageRequestSummary;
+using PrefetchDataMap = ResourcePrefetchPredictorTables::PrefetchDataMap;
+using RedirectDataMap = ResourcePrefetchPredictorTables::RedirectDataMap;
 
 scoped_refptr<net::HttpResponseHeaders> MakeResponseHeaders(
     const char* headers) {
@@ -153,6 +154,18 @@ class MockResourcePrefetchPredictorTables
   ~MockResourcePrefetchPredictorTables() { }
 };
 
+class MockResourcePrefetchPredictorObserver
+    : public ResourcePrefetchPredictor::TestObserver {
+ public:
+  MockResourcePrefetchPredictorObserver(ResourcePrefetchPredictor* predictor)
+      : TestObserver(predictor) {}
+
+  MOCK_METHOD2(
+      OnNavigationLearned,
+      void(size_t url_visit_count,
+           const ResourcePrefetchPredictor::PageRequestSummary& summary));
+};
+
 class ResourcePrefetchPredictorTest : public testing::Test {
  public:
   ResourcePrefetchPredictorTest();
@@ -184,6 +197,17 @@ class ResourcePrefetchPredictorTest : public testing::Test {
     return navigation_id;
   }
 
+  PageRequestSummary CreatePageRequestSummary(
+      const std::string& main_frame_url,
+      const std::string& initial_url,
+      const std::vector<URLRequestSummary>& subresource_requests) {
+    GURL main_frame_gurl(main_frame_url);
+    PageRequestSummary summary(main_frame_gurl);
+    summary.initial_url = GURL(initial_url);
+    summary.subresource_requests = subresource_requests;
+    return summary;
+  }
+
   URLRequestSummary CreateURLRequestSummary(
       int process_id,
       int render_frame_id,
@@ -264,6 +288,8 @@ class ResourcePrefetchPredictorTest : public testing::Test {
     config.mode |= ResourcePrefetchPredictorConfig::HOST_LEARNING;
     predictor_.reset(new ResourcePrefetchPredictor(config, profile_.get()));
     predictor_->set_mock_tables(mock_tables_);
+    mock_observer_.reset(new StrictMock<MockResourcePrefetchPredictorObserver>(

[pasko, 2016/10/24 17:59:38]

If there is a ResetPredictor() somewhere in the test, what happens with an
EXPECT_CALL(*mock_observer_.get(), ...) that was issued before it?

The approach with mock_tables_ seems to be working because this object is not
swapped, just re-attached to the new ResourcePrefetchPredictor on every
ResetPredictor(). Seems like consistency here would eliminate unpleasant
surprises. Would be nice.

[pasko, 2016/10/24 17:59:38]

It would be nice to avoid this having a 'global' mock_observer_ for all tests
with complicated resetting, registration and deregistration, friendship between
the TestObserver and the ResourcePrefetchPredictor.

I suspect it is not really important to hook into LearnNavigation between resets
of the ResourcePrefetchPredictor (not sure here, there could be reasons, let me
know).

We could instead have a std::unique_ptr<MockTestObserver> in each test that
needs the hook, and the hook would de-register itself in its destructor. The
constructor for such observer _could_ take expected learnings as a parameter,
which can be EXPECT-ed in the destructor. This still has the flexibility of
.reset() on the observer to perform the checks and installing the new observer.
This way the two classes would avoid tight coupling: for the predictor, the
observer is just a pointer - minimal code in production.

[alexilin, 2016/10/25 11:38:09]

Yeah, it's a kind of dangerous situation.
Let's try to rewrite it.

All control happens in a single thread so there won't be any race conditions
between resets.

I don't want reinvent EXPECT_CALL and pass expected arguments as parameters of
the mock class constructor. 

[pasko, 2016/10/25 14:05:40]

this latter sgtm

[alexilin, 2016/10/25 15:10:58]

Done.

+        predictor_.get()));
   }
 
   void InitializeSampleData();
@@ -274,6 +300,8 @@ class ResourcePrefetchPredictorTest : public testing::Test {
 
   std::unique_ptr<ResourcePrefetchPredictor> predictor_;
   scoped_refptr<StrictMock<MockResourcePrefetchPredictorTables> > mock_tables_;
+  std::unique_ptr<StrictMock<MockResourcePrefetchPredictorObserver>>
+      mock_observer_;
 
   PrefetchDataMap test_url_data_;
   PrefetchDataMap test_host_data_;
@@ -479,7 +507,8 @@ TEST_F(ResourcePrefetchPredictorTest, LazilyInitializeWithData) {
 
 // Single navigation but history count is low, so should not record.
 TEST_F(ResourcePrefetchPredictorTest, NavigationNotRecorded) {
-  AddUrlToHistory("http://www.google.com", 1);
+  const int kVisitCount = 1;
+  AddUrlToHistory("https://www.google.com", kVisitCount);
 
   URLRequestSummary main_frame =
       CreateURLRequestSummary(1, 1, "http://www.google.com");
@@ -506,6 +535,13 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationNotRecorded) {
       content::RESOURCE_TYPE_SCRIPT, net::MEDIUM, "text/javascript", false);
   predictor_->RecordURLResponse(resource3);
 
+  EXPECT_CALL(
+      *mock_observer_.get(),
+      OnNavigationLearned(kVisitCount,
+                          CreatePageRequestSummary(
+                              "https://www.google.com", "http://www.google.com",
+                              {resource1, resource2, resource3})));
+
   PrefetchData host_data = CreatePrefetchData("www.google.com");
   InitializeResourceData(host_data.add_resources(),
                          "https://google.com/style1.css",
@@ -528,41 +564,49 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationNotRecorded) {
 // Single navigation that will be recorded. Will check for duplicate
 // resources and also for number of resources saved.
 TEST_F(ResourcePrefetchPredictorTest, NavigationUrlNotInDB) {
-  AddUrlToHistory("http://www.google.com", 4);
+  const int kVisitCount = 4;
+  AddUrlToHistory("http://www.google.com", kVisitCount);
 
   URLRequestSummary main_frame =
       CreateURLRequestSummary(1, 1, "http://www.google.com");
   predictor_->RecordURLRequest(main_frame);
   EXPECT_EQ(1U, predictor_->inflight_navigations_.size());
 
-  URLRequestSummary resource1 = CreateURLRequestSummary(
+  std::vector<URLRequestSummary> resources;
+  resources.push_back(CreateURLRequestSummary(
       1, 1, "http://www.google.com", "http://google.com/style1.css",
-      content::RESOURCE_TYPE_STYLESHEET, net::MEDIUM, "text/css", false);
-  predictor_->RecordURLResponse(resource1);
-  URLRequestSummary resource2 = CreateURLRequestSummary(
+      content::RESOURCE_TYPE_STYLESHEET, net::MEDIUM, "text/css", false));
+  predictor_->RecordURLResponse(resources.back());
+  resources.push_back(CreateURLRequestSummary(
       1, 1, "http://www.google.com", "http://google.com/script1.js",
-      content::RESOURCE_TYPE_SCRIPT, net::MEDIUM, "text/javascript", false);
-  predictor_->RecordURLResponse(resource2);
-  URLRequestSummary resource3 = CreateURLRequestSummary(
+      content::RESOURCE_TYPE_SCRIPT, net::MEDIUM, "text/javascript", false));
+  predictor_->RecordURLResponse(resources.back());
+  resources.push_back(CreateURLRequestSummary(
       1, 1, "http://www.google.com", "http://google.com/script2.js",
-      content::RESOURCE_TYPE_SCRIPT, net::MEDIUM, "text/javascript", false);
-  predictor_->RecordURLResponse(resource3);
-  URLRequestSummary resource4 = CreateURLRequestSummary(
+      content::RESOURCE_TYPE_SCRIPT, net::MEDIUM, "text/javascript", false));
+  predictor_->RecordURLResponse(resources.back());
+  resources.push_back(CreateURLRequestSummary(
       1, 1, "http://www.google.com", "http://google.com/script1.js",
-      content::RESOURCE_TYPE_SCRIPT, net::MEDIUM, "text/javascript", true);
-  predictor_->RecordURLResponse(resource4);
-  URLRequestSummary resource5 = CreateURLRequestSummary(
+      content::RESOURCE_TYPE_SCRIPT, net::MEDIUM, "text/javascript", true));
+  predictor_->RecordURLResponse(resources.back());
+  resources.push_back(CreateURLRequestSummary(
       1, 1, "http://www.google.com", "http://google.com/image1.png",
-      content::RESOURCE_TYPE_IMAGE, net::MEDIUM, "image/png", false);
-  predictor_->RecordURLResponse(resource5);
-  URLRequestSummary resource6 = CreateURLRequestSummary(
+      content::RESOURCE_TYPE_IMAGE, net::MEDIUM, "image/png", false));
+  predictor_->RecordURLResponse(resources.back());
+  resources.push_back(CreateURLRequestSummary(
       1, 1, "http://www.google.com", "http://google.com/image2.png",
-      content::RESOURCE_TYPE_IMAGE, net::MEDIUM, "image/png", false);
-  predictor_->RecordURLResponse(resource6);
-  URLRequestSummary resource7 = CreateURLRequestSummary(
+      content::RESOURCE_TYPE_IMAGE, net::MEDIUM, "image/png", false));
+  predictor_->RecordURLResponse(resources.back());
+  resources.push_back(CreateURLRequestSummary(
       1, 1, "http://www.google.com", "http://google.com/style2.css",
-      content::RESOURCE_TYPE_STYLESHEET, net::MEDIUM, "text/css", true);
-  predictor_->RecordURLResponse(resource7);
+      content::RESOURCE_TYPE_STYLESHEET, net::MEDIUM, "text/css", true));
+  predictor_->RecordURLResponse(resources.back());
+
+  EXPECT_CALL(*mock_observer_.get(),
+              OnNavigationLearned(
+                  kVisitCount, CreatePageRequestSummary("http://www.google.com",
+                                                        "http://www.google.com",
+                                                        resources)));
 
   PrefetchData url_data = CreatePrefetchData("http://www.google.com/");
   InitializeResourceData(url_data.add_resources(),
@@ -589,14 +633,15 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlNotInDB) {
               UpdateData(empty_resource_data_, host_data, empty_redirect_data_,
                          empty_redirect_data_));
 
-  predictor_->OnNavigationComplete(main_frame.navigation_id);
+  predictor_->RecordMainFrameLoadComplete(main_frame.navigation_id);
   profile_->BlockUntilHistoryProcessesPendingRequests();
 }
 
 // Tests that navigation is recorded correctly for URL already present in
 // the database cache.
 TEST_F(ResourcePrefetchPredictorTest, NavigationUrlInDB) {
-  AddUrlToHistory("http://www.google.com", 4);
+  const int kVisitCount = 4;
+  AddUrlToHistory("http://www.google.com", kVisitCount);
 
   EXPECT_CALL(*mock_tables_.get(),
               GetAllData(Pointee(ContainerEq(PrefetchDataMap())),
@@ -616,34 +661,41 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlInDB) {
   predictor_->RecordURLRequest(main_frame);
   EXPECT_EQ(1U, predictor_->inflight_navigations_.size());
 
-  URLRequestSummary resource1 = CreateURLRequestSummary(
+  std::vector<URLRequestSummary> resources;
+  resources.push_back(CreateURLRequestSummary(
       1, 1, "http://www.google.com", "http://google.com/style1.css",
-      content::RESOURCE_TYPE_STYLESHEET, net::MEDIUM, "text/css", false);
-  predictor_->RecordURLResponse(resource1);
-  URLRequestSummary resource2 = CreateURLRequestSummary(
+      content::RESOURCE_TYPE_STYLESHEET, net::MEDIUM, "text/css", false));
+  predictor_->RecordURLResponse(resources.back());
+  resources.push_back(CreateURLRequestSummary(
       1, 1, "http://www.google.com", "http://google.com/script1.js",
-      content::RESOURCE_TYPE_SCRIPT, net::MEDIUM, "text/javascript", false);
-  predictor_->RecordURLResponse(resource2);
-  URLRequestSummary resource3 = CreateURLRequestSummary(
+      content::RESOURCE_TYPE_SCRIPT, net::MEDIUM, "text/javascript", false));
+  predictor_->RecordURLResponse(resources.back());
+  resources.push_back(CreateURLRequestSummary(
       1, 1, "http://www.google.com", "http://google.com/script2.js",
-      content::RESOURCE_TYPE_SCRIPT, net::MEDIUM, "text/javascript", false);
-  predictor_->RecordURLResponse(resource3);
-  URLRequestSummary resource4 = CreateURLRequestSummary(
+      content::RESOURCE_TYPE_SCRIPT, net::MEDIUM, "text/javascript", false));
+  predictor_->RecordURLResponse(resources.back());
+  resources.push_back(CreateURLRequestSummary(
       1, 1, "http://www.google.com", "http://google.com/script1.js",
-      content::RESOURCE_TYPE_SCRIPT, net::MEDIUM, "text/javascript", true);
-  predictor_->RecordURLResponse(resource4);
-  URLRequestSummary resource5 = CreateURLRequestSummary(
+      content::RESOURCE_TYPE_SCRIPT, net::MEDIUM, "text/javascript", true));
+  predictor_->RecordURLResponse(resources.back());
+  resources.push_back(CreateURLRequestSummary(
       1, 1, "http://www.google.com", "http://google.com/image1.png",
-      content::RESOURCE_TYPE_IMAGE, net::MEDIUM, "image/png", false);
-  predictor_->RecordURLResponse(resource5);
-  URLRequestSummary resource6 = CreateURLRequestSummary(
+      content::RESOURCE_TYPE_IMAGE, net::MEDIUM, "image/png", false));
+  predictor_->RecordURLResponse(resources.back());
+  resources.push_back(CreateURLRequestSummary(
       1, 1, "http://www.google.com", "http://google.com/image2.png",
-      content::RESOURCE_TYPE_IMAGE, net::MEDIUM, "image/png", false);
-  predictor_->RecordURLResponse(resource6);
-  URLRequestSummary resource7 = CreateURLRequestSummary(
+      content::RESOURCE_TYPE_IMAGE, net::MEDIUM, "image/png", false));
+  predictor_->RecordURLResponse(resources.back());
+  resources.push_back(CreateURLRequestSummary(
       1, 1, "http://www.google.com", "http://google.com/style2.css",
-      content::RESOURCE_TYPE_STYLESHEET, net::MEDIUM, "text/css", true);
-  predictor_->RecordURLResponse(resource7);
+      content::RESOURCE_TYPE_STYLESHEET, net::MEDIUM, "text/css", true));
+  predictor_->RecordURLResponse(resources.back());
+
+  EXPECT_CALL(*mock_observer_.get(),
+              OnNavigationLearned(
+                  kVisitCount, CreatePageRequestSummary("http://www.google.com",
+                                                        "http://www.google.com",
+                                                        resources)));
 
   PrefetchData url_data = CreatePrefetchData("http://www.google.com/");
   InitializeResourceData(url_data.add_resources(),
@@ -685,13 +737,14 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlInDB) {
               UpdateData(empty_resource_data_, host_data, empty_redirect_data_,
                          empty_redirect_data_));
 
-  predictor_->OnNavigationComplete(main_frame.navigation_id);
+  predictor_->RecordMainFrameLoadComplete(main_frame.navigation_id);
   profile_->BlockUntilHistoryProcessesPendingRequests();
 }
 
 // Tests that a URL is deleted before another is added if the cache is full.
 TEST_F(ResourcePrefetchPredictorTest, NavigationUrlNotInDBAndDBFull) {
-  AddUrlToHistory("http://www.nike.com/", 4);
+  const int kVisitCount = 4;
+  AddUrlToHistory("http://www.nike.com/", kVisitCount);
 
   EXPECT_CALL(*mock_tables_.get(),
               GetAllData(Pointee(ContainerEq(PrefetchDataMap())),
@@ -720,6 +773,12 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlNotInDBAndDBFull) {
       content::RESOURCE_TYPE_IMAGE, net::MEDIUM, "image/png", false);
   predictor_->RecordURLResponse(resource2);
 
+  EXPECT_CALL(*mock_observer_.get(),
+              OnNavigationLearned(
+                  kVisitCount, CreatePageRequestSummary(
+                                   "http://www.nike.com", "http://www.nike.com",
+                                   {resource1, resource2})));
+
   EXPECT_CALL(*mock_tables_.get(),
               DeleteSingleResourceDataPoint("http://www.google.com/",
                                             PREFETCH_KEY_TYPE_URL));
@@ -744,12 +803,13 @@ TEST_F(ResourcePrefetchPredictorTest, NavigationUrlNotInDBAndDBFull) {
               UpdateData(empty_resource_data_, host_data, empty_redirect_data_,
                          empty_redirect_data_));
 
-  predictor_->OnNavigationComplete(main_frame.navigation_id);
+  predictor_->RecordMainFrameLoadComplete(main_frame.navigation_id);
   profile_->BlockUntilHistoryProcessesPendingRequests();
 }
 
 TEST_F(ResourcePrefetchPredictorTest, RedirectUrlNotInDB) {
-  AddUrlToHistory("https://facebook.com/google", 4);
+  const int kVisitCount = 4;
+  AddUrlToHistory("https://facebook.com/google", kVisitCount);
 
   URLRequestSummary fb1 = CreateURLRequestSummary(1, 1, "http://fb.com/google");
   predictor_->RecordURLRequest(fb1);
@@ -763,6 +823,13 @@ TEST_F(ResourcePrefetchPredictorTest, RedirectUrlNotInDB) {
   predictor_->RecordURLRedirect(fb3);
   NavigationID fb_end = CreateNavigationID(1, 1, "https://facebook.com/google");
 
+  EXPECT_CALL(
+      *mock_observer_.get(),
+      OnNavigationLearned(kVisitCount, CreatePageRequestSummary(
+                                           "https://facebook.com/google",
+                                           "http://fb.com/google",
+                                           std::vector<URLRequestSummary>())));
+
   // Since the navigation hasn't resources, corresponding entry
   // in resource table will be deleted.
   EXPECT_CALL(*mock_tables_.get(),
@@ -792,7 +859,8 @@ TEST_F(ResourcePrefetchPredictorTest, RedirectUrlNotInDB) {
 // Tests that redirect is recorded correctly for URL already present in
 // the database cache.
 TEST_F(ResourcePrefetchPredictorTest, RedirectUrlInDB) {
-  AddUrlToHistory("https://facebook.com/google", 4);
+  const int kVisitCount = 7;
+  AddUrlToHistory("https://facebook.com/google", kVisitCount);
 
   EXPECT_CALL(*mock_tables_.get(),
               GetAllData(Pointee(ContainerEq(PrefetchDataMap())),
@@ -818,6 +886,13 @@ TEST_F(ResourcePrefetchPredictorTest, RedirectUrlInDB) {
   predictor_->RecordURLRedirect(fb3);
   NavigationID fb_end = CreateNavigationID(1, 1, "https://facebook.com/google");
 
+  EXPECT_CALL(
+      *mock_observer_.get(),
+      OnNavigationLearned(kVisitCount, CreatePageRequestSummary(
+                                           "https://facebook.com/google",
+                                           "http://fb.com/google",
+                                           std::vector<URLRequestSummary>())));
+
   // Oldest entries in tables will be superseded and deleted.
   EXPECT_CALL(*mock_tables_.get(),
               DeleteSingleRedirectDataPoint("bbc.com", PREFETCH_KEY_TYPE_HOST));
@@ -1498,4 +1573,36 @@ TEST_F(ResourcePrefetchPredictorTest, GetPrefetchData) {
   EXPECT_THAT(urls, UnorderedElementsAre(GURL(font_url)));
 }
 
+TEST_F(ResourcePrefetchPredictorTest, SetObserverForTesting) {
+  std::unique_ptr<ResourcePrefetchPredictor> predictor =
+      base::MakeUnique<ResourcePrefetchPredictor>(predictor_->config_,
+                                                  predictor_->profile_);
+
+  // Test that observer is created correctly.
+  MockResourcePrefetchPredictorObserver observer1(predictor.get());
+  EXPECT_EQ(observer1.predictor_, predictor.get());
+  EXPECT_EQ(predictor->observer_, &observer1);
+
+  // Test that observer is superseded correctly.
+  MockResourcePrefetchPredictorObserver observer2(predictor.get());
+  EXPECT_EQ(observer2.predictor_, predictor.get());
+  EXPECT_EQ(predictor->observer_, &observer2);
+  EXPECT_EQ(observer1.predictor_, nullptr);
+
+  // Test that observer imforms predictor about its death.

[pasko, 2016/10/24 17:59:38]

nit: s/imforms/informs/

[alexilin, 2016/10/25 11:38:09]

Done.

+  {
+    MockResourcePrefetchPredictorObserver observer3(predictor.get());
+    EXPECT_EQ(observer3.predictor_, predictor.get());
+    EXPECT_EQ(predictor->observer_, &observer3);
+  }
+  EXPECT_EQ(predictor->observer_, nullptr);
+
+  // Test that predictor informs observer about its death.
+  MockResourcePrefetchPredictorObserver observer4(predictor.get());
+  EXPECT_EQ(observer4.predictor_, predictor.get());
+  EXPECT_EQ(predictor->observer_, &observer4);
+  predictor.reset();
+  EXPECT_EQ(observer4.predictor_, nullptr);
+}
+
 }  // namespace predictors