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..fd0f9446dcb45f829f6e5dab5a206d3ad151967c 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor.h
+++ b/chrome/browser/predictors/resource_prefetch_predictor.h
@@ -100,6 +100,19 @@ class ResourcePrefetchPredictor
                                   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;
@@ -132,6 +145,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|.

[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;
@@ -160,52 +180,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.

[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:
-    typedef ResourcePrefetchPredictor::URLRequestSummary URLRequestSummary;
-    typedef ResourcePrefetchPredictor::PageRequestSummary PageRequestSummary;
-    typedef base::Callback<void(size_t,  // Visit count.
-                                const NavigationID&,
-                                const PageRequestSummary&)>
-        VisitInfoCallback;
+    virtual ~TestObserver() {}
+    virtual void OnNavigationLearned(size_t url_visit_count,
+                                     const PageRequestSummary& summary) {}
 
-    GetUrlVisitCountTask(const NavigationID& navigation_id,
-                         std::unique_ptr<PageRequestSummary> summary,
-                         VisitInfoCallback callback);
-
-    bool RunOnDBThread(history::HistoryBackend* backend,
-                       history::HistoryDatabase* db) override;
-
-    void DoneRunOnMainThread() override;
+   protected:
+    TestObserver() = default;
 
    private:
-    ~GetUrlVisitCountTask() override;
-
-    int visit_count_;
-    NavigationID navigation_id_;
-    std::unique_ptr<PageRequestSummary> summary_;
-    VisitInfoCallback callback_;
+    DISALLOW_COPY_AND_ASSIGN(TestObserver);
+  };
 
-    DISALLOW_COPY_AND_ASSIGN(GetUrlVisitCountTask);
+  enum InitializationState {
+    NOT_INITIALIZED = 0,
+    INITIALIZING = 1,
+    INITIALIZED = 2
   };
 
   typedef ResourcePrefetchPredictorTables::PrefetchDataMap PrefetchDataMap;
@@ -261,15 +255,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 +283,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 +340,13 @@ class ResourcePrefetchPredictor
     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_;