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 {
+    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;
 
   // 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);
+
+  // 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;
 
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest, DeleteUrls);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            LazilyInitializeEmpty);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
                            LazilyInitializeWithData);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTest,
@@ skipping 14 matching lines @@
                            PopulatePrefetcherRequest);
   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 {
-   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 37 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;
   }
 
   Profile* const profile_;
+  TestDelegate* delegate_;
   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_