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>
 #include <memory>
 #include <set>
 #include <string>
 #include <vector>
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
 #include "base/scoped_observer.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "base/time/time.h"
 #include "chrome/browser/predictors/resource_prefetch_common.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor_tables.h"
 #include "chrome/browser/predictors/resource_prefetcher.h"
 #include "components/history/core/browser/history_db_task.h"
 #include "components/history/core/browser/history_service_observer.h"
 #include "components/history/core/browser/history_types.h"
 #include "components/keyed_service/core/keyed_service.h"
@@ skipping 64 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 information about inflight navigation.
+  struct PageRequestSummary {

[pasko, 2016/10/21 12:12:53]

What is the value of having PageRequestSummary in this class? I see a drawback:
it is a longer name.

[alexilin, 2016/10/21 13:38:04]

We could extract the URLRequestSummary class from ResourcePrefetchPredictor
declaration too. And move it to resource_prefetch_common.h. Or keep in this
header. WDYT?

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

Frankly I do not really understand the purpose of resource_prefetch_common.h.
Moving the declaration there won't make PageRequestSummary be available in less
files, hence probably won't increase overall maintainability, so it would be
better to stay in resource_prefetch_predictor because that's where the
declaration is mostly used.

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

Ok, it turned out that I can't take PageRequestSummary from
ResourcePrefetchPredictorClass without taking out URLRequestSummary that leads
to changes in many files. Maybe in other CL?

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

Ah, right. OK.

+    explicit PageRequestSummary(const GURL& main_frame_url);
+    ~PageRequestSummary();
+
+    GURL main_frame_url;
+    GURL initial_url;
+
+    // Stores all subresources requests within a single navigation, from initial

[pasko, 2016/10/21 12:12:52]

nit: s/subresources/subresource/

(I know, I know, this is not being changed)

[alexilin, 2016/10/21 13:38:04]

Done.

+    // 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. Register the observer via
+  // ResourcePrefetchPredictor::AddObserver.
+  class Observer {
+   public:
+    virtual void OnNavigationLearned(size_t url_visit_count,
+                                     const PageRequestSummary& summary) {}
+
+   protected:
+    virtual ~Observer() {}
+  };

[pasko, 2016/10/21 12:12:52]

private:
  DISALLOW_COPY_AND_ASSIGN(Observer);

[alexilin, 2016/10/21 13:38:04]

Done.

+
   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
+  // NavigationID either at the URL or at the host level.
+  void StartPrefetching(const GURL& main_frame_url);

[pasko, 2016/10/21 12:12:52]

thanks for making only one public section, looks clearer now

[alexilin, 2016/10/21 13:38:04]

Acknowledged.

+
+  // Stops prefetching that may be in progress corresponding to |navigation_id|.
+  void StopPrefetching(const GURL& main_frame_url);
+
+  // Adds an |observer| to be notified when the resource prefetch predictor data
+  // changes.
+  void AddObserver(Observer* observer);
+
+  // Removes |observer| from the observer list.
+  void RemoveObserver(Observer* observer);
+
  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 42 matching lines @@
   void set_mock_tables(scoped_refptr<ResourcePrefetchPredictorTables> tables) {
     tables_ = tables;
   }
 
   Profile* const profile_;
   ResourcePrefetchPredictorConfig const config_;
   InitializationState initialization_state_;
   scoped_refptr<ResourcePrefetchPredictorTables> tables_;
   scoped_refptr<ResourcePrefetcherManager> prefetch_manager_;
   base::CancelableTaskTracker history_lookup_consumer_;
+  base::ObserverList<Observer> observers_;

[pasko, 2016/10/21 12:12:52]

Do we need many observers or just one would be enough for our tests?

Can you list the idea of observers that are going to be introduced?

[alexilin, 2016/10/21 13:38:04]

I see only one application point for these observers for now, it's browser
tests. 
From offline discussion: it was decided to replace list of observers by
reference to single observer and to mark that it's observer for testing.

 
   // 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_