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..7b2ce2fea5cf6c6b18610d8dbee26b189101f5a7 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"

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

not needed any more?

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

Done.
I still don't know how to keep list of includes nicely.
This is the thing in C++ I don't like most of all.

 #include "base/scoped_observer.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "base/time/time.h"
@@ -100,6 +101,19 @@ class ResourcePrefetchPredictor
                                   URLRequestSummary* summary);
   };
 
+  // Stores information about inflight navigation.
+  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;
@@ -132,6 +146,13 @@ 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);
+
+  // Stops prefetching that may be in progress corresponding to |navigation_id|.
+  void StopPrefetching(const GURL& main_frame_url);
+
  private:
   friend class ::PredictorsHandler;
   friend class ResourcePrefetchPredictorTest;
@@ -160,52 +181,26 @@ class ResourcePrefetchPredictor
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, GetRedirectEndpoint);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, GetPrefetchData);
 
-  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 {
+  // 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.
+  class Observer {

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

s/Observer/TestObserver/

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

Done.

    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);
+    virtual void OnNavigationLearned(size_t url_visit_count,
+                                     const PageRequestSummary& summary) {}
 
-    bool RunOnDBThread(history::HistoryBackend* backend,
-                       history::HistoryDatabase* db) override;
-
-    void DoneRunOnMainThread() override;
+   protected:
+    Observer() = default;

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

if this is to disallow instantiating this class, then there is another way:

virtual void OnNavigationLearned(...) = 0;

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

No, this is for enabling default constructor explicitly because the explicit
declaration of copy constructor (from DISALLOW_COPY_AND_ASSIGN) inhibits the
implicit generation of default constructor.
It could be made public but I don't see a reason for it.
I don't want to force all subclasses to implement all observer methods (well, we
have only one for now) similarly to WebContentsObserver so that's why I don't
want to add abstract methods.

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

Ah, oh, I did not know DISALLOW_COPY_AND_ASSIGN prevents implicit default
constructor. Well, C++ ..

Now it makes sense, thanks!

+    virtual ~Observer() {}

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

if the destructor is protected, then it becomes problematic to use this class
with std::unique_ptr<>

I'd suggest public, and should go before the method declarations.

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

Done.

 
    private:
-    ~GetUrlVisitCountTask() override;
-
-    int visit_count_;
-    NavigationID navigation_id_;
-    std::unique_ptr<PageRequestSummary> summary_;
-    VisitInfoCallback callback_;
+    DISALLOW_COPY_AND_ASSIGN(Observer);
+  };
 
-    DISALLOW_COPY_AND_ASSIGN(GetUrlVisitCountTask);
+  enum InitializationState {
+    NOT_INITIALIZED = 0,
+    INITIALIZING = 1,
+    INITIALIZED = 2
   };
 
   typedef ResourcePrefetchPredictorTables::PrefetchDataMap PrefetchDataMap;
@@ -261,15 +256,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 +284,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 +341,15 @@ class ResourcePrefetchPredictor
     tables_ = tables;
   }
 
+  // Sets an |observer| to be notified when the resource prefetch predictor data

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

s/an/the/ or: s/an//

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

Done.

+  // changes. Use only for testing.

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

Better omit the "Use only for testing" comment because there is a Chrome-wide
convention to name methods with suffix "ForTesting" only if those are not called
in production.

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

Done.

+  void SetObserverForTesting(Observer* observer);
+
+  // Removes |observer| from the observer list. Use only for testing.
+  void RemoveObserverForTesting(Observer* observer);

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

providing the same observer for removal looks a little inconvenient as an API,
the user has to keep the pointer for removal.

How about just having SetObserverForTesting(...) where providing a nullptr
indicates that the observer is removed?

In the comment we also need to write that the observer needs to be removed
before it is destructed, otherwise we risk getting use-after-free in tests.

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

Your statement is contradictory a little bit. You say that it's inconvenient for
users to keep pointer for the observer but then you say that user has to
explicitly remove observer before it is destructed. So user anyway should be
aware of connection between observer destruction and it's removing.

Probably, it'd be better to force all observers to be scoped, i.e. explicitly
define follow constructor and desctructor:

TestObserver(ResourcePrefetchPredictor* predictor) {
  predictor->AddObserver(this);
}

virtual ~TestObserver() {
  predictor->RemoveObserver(this);
}

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

This scoped observer would be useful if we are sure that
ResourcePrefetchPredictor will always outlive the observer, which I was not sure
about. We can also add a ScopedObserverForTesting, just like ScopedObserver<>,
but instead of AddObserver it would call AddObserverForTesting, and such. But
yeah, it seems that observers that are always scoped are quite usable for us.

I was not sure how the tests are structured, will they be creating and
destroying ResourcePrefetchPredictor? I suppose not, then yeah, keeping the
observer scoped helps. But there still seems to be no value in checking that the
observer is the same on removal, checking that there is no observer at
AddObserver is sufficient protection against mistakes IMO.

+
   Profile* const profile_;
+  Observer* observer_;
   ResourcePrefetchPredictorConfig const config_;
   InitializationState initialization_state_;
   scoped_refptr<ResourcePrefetchPredictorTables> tables_;