Redirect handling in the resource_prefetch_predictor.

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/linked_ptr.h"
 #include "base/memory/weak_ptr.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"
 #include "content/public/common/resource_type.h"
 #include "url/gurl.h"
 
 class PredictorsHandler;
 class Profile;
 
 namespace net {
@@ skipping 104 matching lines @@
   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);
 
   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 {

[pasko, 2016/09/27 16:21:27]

this class is an implementation detail behind resource_prefetch_predictor.h, can
you explain why it is moving to the public interface?

If you just wanted to move implementation of the methods outside of the class
declaration, feel free to do it in the .cc file. Also, seems orthogonal to this
change, so easier to review/land with separate CL.

[alexilin, 2016/09/27 18:11:08]

This declaration is in private section of ResourcePrefetchPredictor class
therefore it isn't a part of the public interface. Or do you want to say that
everything that's located in .h file is a part of public interface in terms of
C++? Well, why do we need access modifiers then?

I moved this implementation detail under the ResourcePrefetchPredictor class
because I can no longer access PageRequestSummary inside GetUrlVisitCountTask
since I moved PageRequestSummary (former Navigation) into private section.
That's why it's in this CL.

[pasko, 2016/09/29 18:08:54]

Ah. Thank you for this detailed explanation! What I tried to say is that
removing implementation details from this header seemed possible, and hence
would have been beneficial for reading it and the related classes.

Since we need to instantiate a std::map<NavigationID,
std::unique_ptr<PageRequestSummary>> at the point of instantiation of
ResourcePrefetchPredictor, we would need a complete type of PageRequestSummary
at that point. This indeed means that we have to declare the complete type in
the header because we have two points of instantiation (factory and test). We
could use the factory in the test and move the factory into
resource_prefetch_predictor.

This latter would probably work, but also would be a large addition to this
change, and perhaps will raise its own questions.

OK, thanks, I learned a lot :)

[pasko, 2016/09/29 18:25:55]

On the other hand .. URLRequestSummary is public already, so it's not a big
shift to make PageRequestSummary public. I think keeping this bloated header
smaller has more benefits than making PageRequestSummary private to
ResourcePrefetchPredictor.

WDYT?

+   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::PrefetchData PrefetchData;
   typedef ResourcePrefetchPredictorTables::PrefetchDataMap PrefetchDataMap;
-  typedef std::map<NavigationID, linked_ptr<std::vector<URLRequestSummary> > >
+  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.
   static bool IsHandledSubresource(net::URLRequest* request,
                                    content::ResourceType resource_type);
 
   // Returns true if the subresource has a supported type.
@@ skipping 34 matching lines @@
   void StartPrefetching(const NavigationID& navigation_id);
 
   // Stops prefetching that may be in progress corresponding to |navigation_id|.
   void StopPrefetching(const NavigationID& navigation_id);
 
   // 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
-  // |url_data_map| and |host_data_map|.
+  // all arguments.
   void CreateCaches(std::unique_ptr<PrefetchDataMap> url_data_map,
-                    std::unique_ptr<PrefetchDataMap> host_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,
-                          const std::vector<URLRequestSummary>& requests);
+                          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);
+
   // Merges resources in |new_resources| into the |data_map| and correspondingly
-  // updates the predictor database.
+  // updates the predictor database. Also calls LearnRedirect if relevant.
   void LearnNavigation(const std::string& key,
                        PrefetchKeyType key_type,
                        const std::vector<URLRequestSummary>& new_resources,
                        size_t max_data_map_size,
-                       PrefetchDataMap* data_map);
+                       PrefetchDataMap* data_map,
+                       const std::string& key_before_redirects,
+                       RedirectDataMap* redirect_map);
+
+  // Updates information about final redirect destination for |key| in
+  // |redirect_map| and correspondingly updates the predictor database

[pasko, 2016/09/27 16:21:27]

nit: .

[alexilin, 2016/09/27 18:11:08]

Done.

+  void LearnRedirect(const std::string& key,
+                     PrefetchKeyType key_type,
+                     const std::string& final_redirect,
+                     size_t max_redirect_map_size,
+                     RedirectDataMap* redirect_map);
 
   // Reports overall page load time.
   void ReportPageLoadTimeStats(base::TimeDelta plt) const;
 
   // Reports page load time for prefetched and not prefetched pages
   void ReportPageLoadTimePrefetchStats(
       base::TimeDelta plt,
       bool prefetched,
       base::Callback<void(int)> report_network_type_callback,
       PrefetchKeyType key_type) const;
@@ skipping 16 matching lines @@
     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_;
 
-  // Map of all the navigations in flight to their resource requests.
-  NavigationMap inflight_navigations_;
-
   // 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_