predictors: Make ResourcePrefetchPredictor observable.

chrome/browser/predictors/resource_prefetch_predictor.h

Index: chrome/browser/predictors/resource_prefetch_predictor.h
diff --git a/chrome/browser/predictors/resource_prefetch_predictor.h b/chrome/browser/predictors/resource_prefetch_predictor.h
index 6c238fb336f3e9049c5d7766aa603c4d1898384c..167666694c2b7aeae390738abc987a1e7e58e0fa 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.h
+++ b/chrome/browser/predictors/resource_prefetch_predictor.h
@@ -16,6 +16,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
 #include "base/scoped_observer.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "base/time/time.h"
@@ -100,6 +101,31 @@ class ResourcePrefetchPredictor
                                   URLRequestSummary* summary);
   };
 
+  // Stores information about inflight navigation.
+  struct PageRequestSummary {

[pasko, 2016/10/21 12:12:53]

What is the value of having PageRequestSummary in this class? I see a drawback:
it is a longer name.

[alexilin, 2016/10/21 13:38:04]

We could extract the URLRequestSummary class from ResourcePrefetchPredictor
declaration too. And move it to resource_prefetch_common.h. Or keep in this
header. WDYT?

[pasko, 2016/10/21 14:41:35]

Frankly I do not really understand the purpose of resource_prefetch_common.h.
Moving the declaration there won't make PageRequestSummary be available in less
files, hence probably won't increase overall maintainability, so it would be
better to stay in resource_prefetch_predictor because that's where the
declaration is mostly used.

[alexilin, 2016/10/21 16:04:18]

Ok, it turned out that I can't take PageRequestSummary from
ResourcePrefetchPredictorClass without taking out URLRequestSummary that leads
to changes in many files. Maybe in other CL?

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

Ah, right. OK.

+    explicit PageRequestSummary(const GURL& main_frame_url);
+    ~PageRequestSummary();
+
+    GURL main_frame_url;
+    GURL initial_url;
+
+    // Stores all subresources requests within a single navigation, from initial

[pasko, 2016/10/21 12:12:52]

nit: s/subresources/subresource/

(I know, I know, this is not being changed)

[alexilin, 2016/10/21 13:38:04]

Done.

+    // main frame request to navigation completion.
+    std::vector<URLRequestSummary> subresource_requests;
+  };
+
+  // An interface used to notify clients (observers) of this object that data in
+  // the resource prefetch predictor has changed. Register the observer via
+  // ResourcePrefetchPredictor::AddObserver.
+  class Observer {
+   public:
+    virtual void OnNavigationLearned(size_t url_visit_count,
+                                     const PageRequestSummary& summary) {}
+
+   protected:
+    virtual ~Observer() {}
+  };

[pasko, 2016/10/21 12:12:52]

private:
  DISALLOW_COPY_AND_ASSIGN(Observer);

[alexilin, 2016/10/21 13:38:04]

Done.

+
   ResourcePrefetchPredictor(const ResourcePrefetchPredictorConfig& config,
                             Profile* profile);
   ~ResourcePrefetchPredictor() override;
@@ -132,6 +158,20 @@ class ResourcePrefetchPredictor
   // 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);

[pasko, 2016/10/21 12:12:52]

thanks for making only one public section, looks clearer now

[alexilin, 2016/10/21 13:38:04]

Acknowledged.

+
+  // Stops prefetching that may be in progress corresponding to |navigation_id|.
+  void StopPrefetching(const GURL& main_frame_url);
+
+  // Adds an |observer| to be notified when the resource prefetch predictor data
+  // changes.
+  void AddObserver(Observer* observer);
+
+  // Removes |observer| from the observer list.
+  void RemoveObserver(Observer* observer);
+
  private:
   friend class ::PredictorsHandler;
   friend class ResourcePrefetchPredictorTest;
@@ -166,48 +206,6 @@ class ResourcePrefetchPredictor
     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;
 
@@ -261,15 +259,6 @@ class ResourcePrefetchPredictor
                                  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();
@@ -298,8 +287,7 @@ class ResourcePrefetchPredictor
   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
@@ -362,6 +350,7 @@ class ResourcePrefetchPredictor
   scoped_refptr<ResourcePrefetchPredictorTables> tables_;
   scoped_refptr<ResourcePrefetcherManager> prefetch_manager_;
   base::CancelableTaskTracker history_lookup_consumer_;
+  base::ObserverList<Observer> observers_;

[pasko, 2016/10/21 12:12:52]

Do we need many observers or just one would be enough for our tests?

Can you list the idea of observers that are going to be introduced?

[alexilin, 2016/10/21 13:38:04]

I see only one application point for these observers for now, it's browser
tests. 
From offline discussion: it was decided to replace list of observers by
reference to single observer and to mark that it's observer for testing.

 
   // Copy of the data in the predictor tables.
   std::unique_ptr<PrefetchDataMap> url_table_cache_;