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..098a1674af6950145fec459f7636605775d8a24e 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.h
+++ b/chrome/browser/predictors/resource_prefetch_predictor.h
@@ -100,6 +100,48 @@ class ResourcePrefetchPredictor
                                   URLRequestSummary* summary);
   };
 
+  // Stores the data learned from a single navigation.
+  struct PageRequestSummary {

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

struct vs. class:

the style guide says: "structs should be used for passive objects that carry
data, and may have associated constants, but lack any functionality other than
access/setting the data members."

This one contains GURL and vector, which are not "passive objects", so the
overall object therefore is not "passive". Hence, it should be treated as a
class (with extra boilerplate, .. true .., but it is valuable to describe what
is allowed to be done on this class, and what is not allowed. Otherwise it is
hard to follow in the code whether the vector of request is ever changed after
the object creation.

Seems that is only being moved, and a copy-c-tor added with one more field. In
this case feel free to add a TODO and do it in another change.

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

I have another opinion. PageRequestSummary should be considered as struct
because it only holds data. No matter that its fields is not PODs or non-class
types, PageRequestSummary itself is used only to keep and pass data accross. It
even doesn't have set/get accessors to be as simple as possible.
For example, look at URLRequestSummary struct above or
ResourcePrefetchPredictorConfig
(https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefe...)
or FrameNavigateParams
(https://cs.chromium.org/chromium/src/content/public/common/frame_navigate_par...)

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

OK, you are right, it is indeed common that non-trivial objects are struct
members. This makes it sometimes complex to reason about these structs (for
instance, the insides of GURL are memory-managed non-trivially), but you
convinced me that these pitfalls are commonly expected in the codebase. No need
to change it then.

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

Done.

+    explicit PageRequestSummary(const GURL& main_frame_url);
+    PageRequestSummary(const PageRequestSummary& other);

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

hm, just noticed: public copy-constructor is error-prone, can we avoid it
please? this is especially worrisome because the object holds a vector (hence it
is likely not cheap to copy).

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

It will be hard to avoid this because of following compiler error:
"error: [chromium-style] Complex class/struct needs an explicit out-of-line copy
constructor."
This class has already had implicit public copy constructor before that.
The other thing we can do is DISALLOW_COPY_AND_ASSIGN, but I think that it will
be a little bit strange to have uncopyable struct.

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

Oh, makes sense, thank you for explaining. I forgot about this error. Also, I
presume we need this copy constructor to be able to store these structs in a
vector.

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

Done.

+    ~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 clients (observers) of this object that data in
+  // the resource prefetch predictor has changed. All methods need to be
+  // called on the UI thread.
+  class TestObserver {
+   public:
+    virtual ~TestObserver();
+
+    virtual void OnNavigationLearned(size_t url_visit_count,
+                                     const PageRequestSummary& summary) {}

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

I think a single mock would be enough to hook into all methods.

Then it would be cleaner if TestObserver were just pure virtual, and the mock
could manage its lifetime and know about the way the ResourcePrefetchPredictor
works. The point of having a single observer injected by a raw pointer was to
free the ResourcePrefetchPredictor from knowing too much about the observers. So
let's keep it this way?

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

IIUC, you suggested to move all implementation details from TestObserver to mock
class in resource_prefetch_predictor_unittest.cc
But I'll have different implementations for unit tests and browser tests. It
seems like we need to have lifetime management in both of them.
Please, explain me your idea in more details. Thanks.

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

Oops. I forgot that the de-registration logic still is preferable to be in this
class. So it seems that this model would be better:

1. TestObserver de-registers itself from the ResourcePrefetchPredictor in d-tor
2. tests guarantee that ResourcePrefetchPredictor always outlives the
TestObserver
3. discussed offline: the mock for browser tests would be different, hence
de-registering makes sense to put in some common class
4. discussed offline: if the second test observer is needed in a test, the first
will be .reset() prior to registering the 2nd one

Also, please just forward-declare the TestObserver here, and define it in some
place for test utilities.

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

Everything done except the last one.
Discussed offline: forward-declare thing would't work because
ResourcePrefetchPredictor calls OnNavigationLearned method. If we move
declaration to test utilities file then TestObserver class won't be resolved in
non-test build.

+
+   protected:
+    // Observer is tied to a single ResourcePrefetchPredictor for its entire

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

it is not perfectly clear what 'its' refers to, ResourcePrefetchPredictor or
TestObserver

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

Done.

+    // lifetime. However it could be displaced by another observer and then it
+    // won't receive any notifications.
+    explicit TestObserver(ResourcePrefetchPredictor* predictor);
+
+   private:
+    friend class ResourcePrefetchPredictor;
+    FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
+                             SetObserverForTesting);
+
+    void ResetResourcePrefetchPredictor();
+
+    ResourcePrefetchPredictor* predictor_;
+
+    DISALLOW_COPY_AND_ASSIGN(TestObserver);
+  };
+
   ResourcePrefetchPredictor(const ResourcePrefetchPredictorConfig& config,
                             Profile* profile);
   ~ResourcePrefetchPredictor() override;
@@ -132,6 +174,14 @@ 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);
+
  private:
   friend class ::PredictorsHandler;
   friend class ResourcePrefetchPredictorTest;
@@ -159,6 +209,8 @@ class ResourcePrefetchPredictor
                            PopulatePrefetcherRequest);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, GetRedirectEndpoint);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, GetPrefetchData);
+  FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
+                           SetObserverForTesting);
 
   enum InitializationState {
     NOT_INITIALIZED = 0,
@@ -166,48 +218,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 +271,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 +299,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
@@ -356,7 +356,12 @@ class ResourcePrefetchPredictor
     tables_ = tables;
   }
 
+  // Sets the |observer| to be notified when the resource prefetch predictor
+  // data changes. Previously registered observer will be discarded.
+  void SetObserverForTesting(TestObserver* observer);
+
   Profile* const profile_;
+  TestObserver* observer_;
   ResourcePrefetchPredictorConfig const config_;
   InitializationState initialization_state_;
   scoped_refptr<ResourcePrefetchPredictorTables> tables_;