predictors: Make ResourcePrefetchPredictor observable.

chrome/browser/predictors/resource_prefetch_predictor.h

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_PREDICTORS_RESOURCE_PREFETCH_PREDICTOR_H_
 #define CHROME_BROWSER_PREDICTORS_RESOURCE_PREFETCH_PREDICTOR_H_
 
 #include <stddef.h>
 
 #include <map>
@@ skipping 82 matching lines @@
 
     bool has_validators;
     bool always_revalidate;
 
     // Initializes a |URLRequestSummary| from a |URLRequest| response.
     // Returns true for success.
     static bool SummarizeResponse(const net::URLRequest& request,
                                   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;
 
   // Thread safe.
   static bool ShouldRecordRequest(net::URLRequest* request,
                                   content::ResourceType resource_type);
   static bool ShouldRecordResponse(net::URLRequest* response);
   static bool ShouldRecordRedirect(net::URLRequest* response);
 
@@ skipping 12 matching lines @@
   // 'ResourcePrefetchPredictorObserver' calls the below functions to inform the
   // predictor of main frame and resource requests. Should only be called if the
   // corresponding Should* functions return true.
   void RecordURLRequest(const URLRequestSummary& request);
   void RecordURLResponse(const URLRequestSummary& response);
   void RecordURLRedirect(const URLRequestSummary& response);
 
   // 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;
 
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, DeleteUrls);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            LazilyInitializeEmpty);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            LazilyInitializeWithData);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            NavigationNotRecorded);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, NavigationUrlInDB);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, NavigationUrlNotInDB);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            NavigationUrlNotInDBAndDBFull);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, RedirectUrlNotInDB);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, RedirectUrlInDB);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, OnMainFrameRequest);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, OnMainFrameRedirect);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            OnSubresourceResponse);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, GetCorrectPLT);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, HandledResourceTypes);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            PopulatePrefetcherRequest);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, GetRedirectEndpoint);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, GetPrefetchData);
+  FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
+                           SetObserverForTesting);
 
   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 {
-   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;
 
   typedef std::map<NavigationID, std::unique_ptr<PageRequestSummary>>
       NavigationMap;
 
   // Returns true if the main page request is supported for prediction.
   static bool IsHandledMainPage(net::URLRequest* request);
 
   // Returns true if the subresource request is supported for prediction.
@@ skipping 33 matching lines @@
   // prefetched.
   bool GetPrefetchData(const GURL& main_frame_url, std::vector<GURL>* urls);
 
   // Returns true iff the |data_map| contains PrefetchData that can be used
   // for a |main_frame_key| and fills |urls| with resources that need to be
   // prefetched.
   bool PopulatePrefetcherRequest(const std::string& main_frame_key,
                                  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();
 
   // Callback for task to read predictor database. Takes ownership of
   // all arguments.
   void CreateCaches(std::unique_ptr<PrefetchDataMap> url_data_map,
                     std::unique_ptr<PrefetchDataMap> host_data_map,
                     std::unique_ptr<RedirectDataMap> url_redirect_data_map,
                     std::unique_ptr<RedirectDataMap> host_redirect_data_map);
 
   // Called during initialization when history is read and the predictor
   // database has been read.
   void OnHistoryAndCacheLoaded();
 
   // Removes data for navigations where the onload never fired. Will cleanup
   // inflight_navigations_.
   void CleanupAbandonedNavigations(const NavigationID& navigation_id);
 
   // Deletes all URLs from the predictor database, the caches and removes all
   // inflight navigations.
   void DeleteAllUrls();
 
   // Deletes data for the input |urls| and their corresponding hosts from the
   // predictor database and caches.
   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
   // predictor database.
   void RemoveOldestEntryInPrefetchDataMap(PrefetchKeyType key_type,
                                           PrefetchDataMap* data_map);
 
   void RemoveOldestEntryInRedirectDataMap(PrefetchKeyType key_type,
                                           RedirectDataMap* data_map);
 
@@ skipping 36 matching lines @@
 
   // Used to connect to HistoryService or register for service loaded
   // notificatioan.
   void ConnectToHistoryService();
 
   // Used for testing to inject mock tables.
   void set_mock_tables(scoped_refptr<ResourcePrefetchPredictorTables> tables) {
     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_;
   scoped_refptr<ResourcePrefetcherManager> prefetch_manager_;
   base::CancelableTaskTracker history_lookup_consumer_;
 
   // Copy of the data in the predictor tables.
   std::unique_ptr<PrefetchDataMap> url_table_cache_;
   std::unique_ptr<PrefetchDataMap> host_table_cache_;
   std::unique_ptr<RedirectDataMap> url_redirect_table_cache_;
   std::unique_ptr<RedirectDataMap> host_redirect_table_cache_;
 
   NavigationMap inflight_navigations_;
 
   ScopedObserver<history::HistoryService, history::HistoryServiceObserver>
       history_service_observer_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictor);
 };
 
 }  // namespace predictors
 
 #endif  // CHROME_BROWSER_PREDICTORS_RESOURCE_PREFETCH_PREDICTOR_H_