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..08796fdc212f74347092f58fd4df3d13d625eb95 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.h
+++ b/chrome/browser/predictors/resource_prefetch_predictor.h
@@ -100,6 +100,40 @@ class ResourcePrefetchPredictor
                                   URLRequestSummary* summary);
   };
 
+  // Stores the data learned from a single navigation.
+  struct PageRequestSummary {
+    explicit PageRequestSummary(const GURL& main_frame_url);
+    PageRequestSummary(const PageRequestSummary& other);
+    ~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;
+  };
+
+  // An interface used to notify that data in the resource prefetch predictor
+  // has changed. All methods are invoked on the UI thread.
+  class TestDelegate {

[pasko, 2016/10/26 12:05:40]

can this be moved outside of ResourcePrefetchPredictor? Accessing fields of the
ResourcePrefetchPredictor is not necessary for the TestObserver (and even for
delegate), so it would be good to keep this separation explicit.

[pasko, 2016/10/26 12:05:41]

I realized that this class can be forward-declared in
resource_prefetch_predictor.h and declared for realz in
resource_prefetch_predictor_test_util.h.

This would require including resource_prefetch_predictor_test_util.h into
resource_prefetch_predictor.cc, which is arguably not the nest thing to do. Up
to you/lizeb@. If you guys like to keep this header extra clean of testing
details, move to another header.

(forward-declaring nested classes is also possible, look:
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...)

[alexilin, 2016/10/26 13:09:59]

I'll try to advocate existing implementation.
For now we have nice naming ResourcePrefetchPredictor::TestObserver.  It's
actually common practice in Chromium to put observers/delegates (even
TestObservers) inside the main class. 
Declaring TestObserver in resource_prefetch_predictor.h outside the main class
looks polluting for predictors namespace. I support 'one header / one class'
point of view.
We could declare it inside resource_prefetch_predictor_test_util.h but when we
should define it then? All definitions for functions inside
resource_prefetch_predictor_test_util.h are in
resource_prefetch_predictor_test_util.cc and this file is included only for
unit_tests build. So we will have to include it in chrome build that will be
more expensive and excessive.
Another option that I could admit is to create new .h .cc specially for
TestObserver. Though there already is ResourcePrefetchPredictorObserver class
that's doing not related things and creation of
ResorucePrefetchPredictorTestObserver could be confusing for readers. 

[pasko, 2016/10/26 13:48:35]

I would argue that this long name is used only in a couple of .cc files, which
keeps the value of the name small. The name predictors::TestObserver is good
enough IMO.
Being a common practice does not necessarily mean it is a good practice for this
case. I do not see how consistency with this practice will make the code more
readable. Consistency is good to maintain within a file, a component, but when
it reaches Chromium-wide code, it is not really of a big value. For example, we
constantly break consistency by using new C++ features in the new code, but not
updating the old code accordingly.

A good way to discover why observers are often nested classes is to ask
chromium-dev@, there we can argue on what the best practice is. For now I cannot
see such topic raised.
predictors namespace is not large, no worries about that
I think it is a good tradeoff to: (a) have two classes in one file, and (b)
keeping the maximum separation between them. A common practice is actually to
create an abstract class (in a separate file), then have an Impl class with
common functionality (one line of code for us, in two files), and then two
derived classes from that. For the functionality we have I think it would be a
gross overkill (and we discussed this offline AFAIR).

I am still convinced that moving this class out makes the code easier to read by
not exposing private members of ResourcePrefetchPredictor to TestObserver.

[alexilin, 2016/10/26 15:22:01]

Done.

+   public:
+    virtual ~TestDelegate();

[pasko, 2016/10/26 12:05:41]

A comment would be worth adding IMO:
// De-registers itself from |predictor_| on destruction.

[alexilin, 2016/10/26 13:09:59]

Done.

+
+    virtual void OnNavigationLearned(size_t url_visit_count,
+                                     const PageRequestSummary& summary) {}
+
+   protected:
+    // |predictor| must be non-NULL and has to outlive the TestDelegate.
+    // Also the predictor must not have a TestDelegate set.
+    explicit TestDelegate(ResourcePrefetchPredictor* predictor);
+
+   private:
+    ResourcePrefetchPredictor* predictor_;
+
+    DISALLOW_COPY_AND_ASSIGN(TestDelegate);
+  };
+
   ResourcePrefetchPredictor(const ResourcePrefetchPredictorConfig& config,
                             Profile* profile);
   ~ResourcePrefetchPredictor() override;
@@ -132,6 +166,19 @@ 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
+  // |main_frame_url| 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
+  // |main_frame_url|.
+  void StopPrefetching(const GURL& main_frame_url);
+
+  // Sets the |delegate| to be notified when the resource prefetch predictor
+  // data changes. Previously registered delegate will be discarded. Call
+  // this with nullptr parameter to de-register delegate.
+  void SetDelegateForTesting(TestDelegate* delegate);
+
  private:
   friend class ::PredictorsHandler;
   friend class ResourcePrefetchPredictorTest;
@@ -166,48 +213,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 +266,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 +294,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
@@ -357,6 +352,7 @@ class ResourcePrefetchPredictor
   }
 
   Profile* const profile_;
+  TestDelegate* delegate_;
   ResourcePrefetchPredictorConfig const config_;
   InitializationState initialization_state_;
   scoped_refptr<ResourcePrefetchPredictorTables> tables_;