predictors: Make ResourcePrefetchPredictor observable.

chrome/browser/predictors/resource_prefetch_predictor.h

 // Copyright 2014 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.
 
 #ifndef CHROME_BROWSER_PREDICTORS_RESOURCE_PREFETCH_PREDICTOR_H_
 #define CHROME_BROWSER_PREDICTORS_RESOURCE_PREFETCH_PREDICTOR_H_
 
 #include <stddef.h>
 
 #include <map>
@@ skipping 82 matching lines @@
 
     bool has_validators;
     bool always_revalidate;
 
     // Initializes a |URLRequestSummary| from a |URLRequest| response.
     // Returns true for success.
     static bool SummarizeResponse(const net::URLRequest& request,
                                   URLRequestSummary* summary);
   };
 
+  // Stores the data is learned from single navigation.

[Benoit L, 2016/10/21 17:16:24]

nit: Stores the data learned from a single navigation.

[alexilin, 2016/10/21 18:37:15]

Done.

+  struct PageRequestSummary {
+    explicit PageRequestSummary(const GURL& main_frame_url);
+    ~PageRequestSummary();
+
+    GURL main_frame_url;
+    GURL initial_url;
+
+    // Stores all subresource requests within a single navigation, from initial
+    // main frame request to navigation completion.
+    std::vector<URLRequestSummary> subresource_requests;
+  };
+
   ResourcePrefetchPredictor(const ResourcePrefetchPredictorConfig& config,
                             Profile* profile);
   ~ResourcePrefetchPredictor() override;
 
   // Thread safe.
   static bool ShouldRecordRequest(net::URLRequest* request,
                                   content::ResourceType resource_type);
   static bool ShouldRecordResponse(net::URLRequest* response);
   static bool ShouldRecordRedirect(net::URLRequest* response);
 
@@ skipping 12 matching lines @@
   // 'ResourcePrefetchPredictorObserver' calls the below functions to inform the
   // predictor of main frame and resource requests. Should only be called if the
   // corresponding Should* functions return true.
   void RecordURLRequest(const URLRequestSummary& request);
   void RecordURLResponse(const URLRequestSummary& response);
   void RecordURLRedirect(const URLRequestSummary& response);
 
   // Called when the main frame of a page completes loading.
   void RecordMainFrameLoadComplete(const NavigationID& navigation_id);
 
+  // Starts prefetching if it is enabled and prefetching data exists for the
+  // NavigationID either at the URL or at the host level.
+  void StartPrefetching(const GURL& main_frame_url);
+
+  // Stops prefetching that may be in progress corresponding to |navigation_id|.

[pasko, 2016/10/21 16:47:34]

huh, I did not notice this during one of the previous reviews. These 2 methods
are no longer driven by navigation ids. While we are here, please update the
comments.

[alexilin, 2016/10/21 18:37:15]

Done.

+  void StopPrefetching(const GURL& main_frame_url);
+
  private:
   friend class ::PredictorsHandler;
   friend class ResourcePrefetchPredictorTest;
 
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, DeleteUrls);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            LazilyInitializeEmpty);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            LazilyInitializeWithData);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            NavigationNotRecorded);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, NavigationUrlInDB);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, NavigationUrlNotInDB);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            NavigationUrlNotInDBAndDBFull);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, RedirectUrlNotInDB);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, RedirectUrlInDB);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, OnMainFrameRequest);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, OnMainFrameRedirect);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            OnSubresourceResponse);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, GetCorrectPLT);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, HandledResourceTypes);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            PopulatePrefetcherRequest);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, GetRedirectEndpoint);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, GetPrefetchData);
 
+  // An interface used to notify clients (observers) of this object that data in
+  // the resource prefetch predictor has changed. Register the observer via
+  // ResourcePrefetchPredictor::SetObserverForTesting.

[Benoit L, 2016/10/21 17:16:24]

Can you add a comment specifying on which thread the observer is going to be
called?

[alexilin, 2016/10/21 18:37:14]

Done.

+  class TestObserver {
+   public:
+    virtual ~TestObserver() {}
+    virtual void OnNavigationLearned(size_t url_visit_count,
+                                     const PageRequestSummary& summary) {}
+
+   protected:
+    TestObserver() = default;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(TestObserver);
+  };
+
   enum InitializationState {
     NOT_INITIALIZED = 0,
     INITIALIZING = 1,
     INITIALIZED = 2
   };
 
-  // Stores information about inflight navigations.
-  struct PageRequestSummary {
-    explicit PageRequestSummary(const GURL& initial_url);
-    ~PageRequestSummary();
-
-    GURL initial_url;
-
-    // Stores all subresources requests within a single navigation, from initial
-    // main frame request to navigation completion.
-    std::vector<URLRequestSummary> subresource_requests;
-  };
-
-  // Used to fetch the visit count for a URL from the History database.
-  class GetUrlVisitCountTask : public history::HistoryDBTask {
-   public:
-    typedef ResourcePrefetchPredictor::URLRequestSummary URLRequestSummary;
-    typedef ResourcePrefetchPredictor::PageRequestSummary PageRequestSummary;
-    typedef base::Callback<void(size_t,  // Visit count.
-                                const NavigationID&,
-                                const PageRequestSummary&)>
-        VisitInfoCallback;
-
-    GetUrlVisitCountTask(const NavigationID& navigation_id,
-                         std::unique_ptr<PageRequestSummary> summary,
-                         VisitInfoCallback callback);
-
-    bool RunOnDBThread(history::HistoryBackend* backend,
-                       history::HistoryDatabase* db) override;
-
-    void DoneRunOnMainThread() override;
-
-   private:
-    ~GetUrlVisitCountTask() override;
-
-    int visit_count_;
-    NavigationID navigation_id_;
-    std::unique_ptr<PageRequestSummary> summary_;
-    VisitInfoCallback callback_;
-
-    DISALLOW_COPY_AND_ASSIGN(GetUrlVisitCountTask);
-  };
-
   typedef ResourcePrefetchPredictorTables::PrefetchDataMap PrefetchDataMap;
   typedef ResourcePrefetchPredictorTables::RedirectDataMap RedirectDataMap;
 
   typedef std::map<NavigationID, std::unique_ptr<PageRequestSummary>>
       NavigationMap;
 
   // Returns true if the main page request is supported for prediction.
   static bool IsHandledMainPage(net::URLRequest* request);
 
   // Returns true if the subresource request is supported for prediction.
@@ skipping 33 matching lines @@
   // prefetched.
   bool GetPrefetchData(const GURL& main_frame_url, std::vector<GURL>* urls);
 
   // Returns true iff the |data_map| contains PrefetchData that can be used
   // for a |main_frame_key| and fills |urls| with resources that need to be
   // prefetched.
   bool PopulatePrefetcherRequest(const std::string& main_frame_key,
                                  const PrefetchDataMap& data_map,
                                  std::vector<GURL>* urls);
 
- public:
-  // Starts prefetching if it is enabled and prefetching data exists for the
-  // NavigationID either at the URL or at the host level.
-  void StartPrefetching(const GURL& main_frame_url);
-
-  // Stops prefetching that may be in progress corresponding to |navigation_id|.
-  void StopPrefetching(const GURL& main_frame_url);
-
- private:
   // Starts initialization by posting a task to the DB thread to read the
   // predictor database.
   void StartInitialization();
 
   // Callback for task to read predictor database. Takes ownership of
   // all arguments.
   void CreateCaches(std::unique_ptr<PrefetchDataMap> url_data_map,
                     std::unique_ptr<PrefetchDataMap> host_data_map,
                     std::unique_ptr<RedirectDataMap> url_redirect_data_map,
                     std::unique_ptr<RedirectDataMap> host_redirect_data_map);
 
   // Called during initialization when history is read and the predictor
   // database has been read.
   void OnHistoryAndCacheLoaded();
 
   // Removes data for navigations where the onload never fired. Will cleanup
   // inflight_navigations_.
   void CleanupAbandonedNavigations(const NavigationID& navigation_id);
 
   // Deletes all URLs from the predictor database, the caches and removes all
   // inflight navigations.
   void DeleteAllUrls();
 
   // Deletes data for the input |urls| and their corresponding hosts from the
   // predictor database and caches.
   void DeleteUrls(const history::URLRows& urls);
 
   // Callback for GetUrlVisitCountTask.
-  void OnVisitCountLookup(size_t visit_count,
-                          const NavigationID& navigation_id,
+  void OnVisitCountLookup(size_t url_visit_count,
                           const PageRequestSummary& summary);
 
   // Removes the oldest entry in the input |data_map|, also deleting it from the
   // predictor database.
   void RemoveOldestEntryInPrefetchDataMap(PrefetchKeyType key_type,
                                           PrefetchDataMap* data_map);
 
   void RemoveOldestEntryInRedirectDataMap(PrefetchKeyType key_type,
                                           RedirectDataMap* data_map);
 
@@ skipping 36 matching lines @@
 
   // Used to connect to HistoryService or register for service loaded
   // notificatioan.
   void ConnectToHistoryService();
 
   // Used for testing to inject mock tables.
   void set_mock_tables(scoped_refptr<ResourcePrefetchPredictorTables> tables) {
     tables_ = tables;
   }
 
+  // Sets the |observer| to be notified when the resource prefetch predictor
+  // data changes. This observer needs to be removed before it is destructed by
+  // calling this function with nullptr parameter.
+  void SetObserverForTesting(TestObserver* observer);

[Benoit L, 2016/10/21 17:16:24]

Stupid question: can we use a unique_ptr<> to hold the observer?
This way we don't have to worry about lifetime issues, and reseting the observer
cleans up the previous one.

[pasko, 2016/10/21 17:38:22]

Not stupid at all.

It's hard to say for me without seeing how tests would use it. I was lazy to
think about proper ownership and thought that having a raw pointer here would
maximally move the ownership complexity towards tests (which would be not
complex at all, I suspect). I more often find testing-only related injections to
happen via raw pointers. So that would be a weak argument towards consistency.

Also, what if a class inherits from several observers? It would not be good if
each of the notification interfaces owned the object.

But your point is good: grabbing the observer via unique_ptr makes the ownership
super clear, no need to have lengthy explanations in comments. Seems usable as
well.

I am OK either way.

[alexilin, 2016/10/21 18:37:15]

Well, it's not great to transfer the observer ownership to
ResourcePrefetchPredictor from the side of browser tests. And then it won't be
observer pattern rather I just pass callback to the class. I don't say that it's
bad but this is not the thing that I would like to have in browser tests.
I inspired
https://cs.chromium.org/chromium/src/content/public/browser/web_contents_obse...
class and made the similar thing. Please, check out updates.

To illustrate browser test's point of view here is sketchy implementation of
observer that I want to have.

class ResourcePrefetchPredictorTestObserver : public
ResourcePrefetchPredictor::TestObserver {
 public:
  explicit ResourcePrefetchPredictorTestObserver(ResourcePrefetchPredictor*
predictor)
      : TestObserver(predictor),
        message_loop_runner_(new content::MessageLoopRunner) {}

  void Wait() { message_loop_runner_->Run(); }

  protected:
  // ResourcePrefetchPredictor::Observer
  void OnNavigationLearned(size_t url_visit_count, const
ResourcePrefetchPredictor::PageRequestSummary& summary) override {
      message_loop_runner_->Quit();
  }

  private:
    scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
    DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictorTestObserver);
};

And example of use:

ResourcePrefetchPredictorTestObserver observer(predictor_);
ui_test_utils::NavigateToURL(browser(), url);
observer.Wait();

[Benoit L, 2016/10/24 02:13:56]

Ok, thank you for the clarification. Observers usually use ObserverList, but
that's likely overkill here. Note that WebContentsObserver unregisters itself in
the destructor.

+
   Profile* const profile_;
+  TestObserver* observer_;
   ResourcePrefetchPredictorConfig const config_;
   InitializationState initialization_state_;
   scoped_refptr<ResourcePrefetchPredictorTables> tables_;
   scoped_refptr<ResourcePrefetcherManager> prefetch_manager_;
   base::CancelableTaskTracker history_lookup_consumer_;
 
   // Copy of the data in the predictor tables.
   std::unique_ptr<PrefetchDataMap> url_table_cache_;
   std::unique_ptr<PrefetchDataMap> host_table_cache_;
   std::unique_ptr<RedirectDataMap> url_redirect_table_cache_;
   std::unique_ptr<RedirectDataMap> host_redirect_table_cache_;
 
   NavigationMap inflight_navigations_;
 
   ScopedObserver<history::HistoryService, history::HistoryServiceObserver>
       history_service_observer_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictor);
 };
 
 }  // namespace predictors
 
 #endif  // CHROME_BROWSER_PREDICTORS_RESOURCE_PREFETCH_PREDICTOR_H_