Redirect handling in the resource_prefetch_predictor.

chrome/browser/predictors/resource_prefetch_predictor_tables.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_TABLES_H_
 #define CHROME_BROWSER_PREDICTORS_RESOURCE_PREFETCH_PREDICTOR_TABLES_H_
 
 #include <stddef.h>
 
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/time/time.h"
 #include "chrome/browser/predictors/predictor_table_base.h"
 #include "chrome/browser/predictors/resource_prefetch_common.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor.pb.h"
 #include "content/public/common/resource_type.h"
 #include "net/base/request_priority.h"
 #include "url/gurl.h"
 
 namespace sql {
 class Statement;
 }
 
 namespace predictors {
 
-using chrome_browser_predictors::ResourceData;
+// From resource_prefetch_predictor.proto.
+using RedirectStat = RedirectData_RedirectStat;
 
 // Interface for database tables used by the ResourcePrefetchPredictor.
 // All methods except the constructor and destructor need to be called on the DB
 // thread.
 //
 // Currently manages:
 //  - UrlResourceTable - resources per Urls.
 //  - UrlMetadataTable - misc data for Urls (like last visit time).
+//  - UrlRedirectTable - redirects per Urls.
 //  - HostResourceTable - resources per host.
 //  - HostMetadataTable - misc data for hosts.
+//  - HostRedirectTable - redirects per host.
 class ResourcePrefetchPredictorTables : public PredictorTableBase {
  public:
-  // Sorts the resources by score, decreasing.
-  static void SortResources(std::vector<ResourceData>* resources);
-
   // Aggregated data for a Url or Host. Although the data differs slightly, we
   // store them in the same structure, because most of the fields are common and
   // it allows us to use the same functions.
   struct PrefetchData {
     PrefetchData(PrefetchKeyType key_type, const std::string& primary_key);
     PrefetchData(const PrefetchData& other);
     ~PrefetchData();
 
     bool is_host() const { return key_type == PREFETCH_KEY_TYPE_HOST; }
 
     // Is the data a host as opposed to a Url?
     PrefetchKeyType key_type;  // Not const to be able to assign.
     std::string primary_key;   // is_host() ? host : main frame url.
 
     base::Time last_visit;
     std::vector<ResourceData> resources;
   };
   // Map from primary key to PrefetchData for the key.
   typedef std::map<std::string, PrefetchData> PrefetchDataMap;
 
+  // Map from primary key to RedirectData for the key.
+  typedef std::map<std::string, RedirectData> RedirectDataMap;
+
   // Returns data for all Urls and Hosts.
   virtual void GetAllData(PrefetchDataMap* url_data_map,
-                          PrefetchDataMap* host_data_map);
+                          PrefetchDataMap* host_data_map,
+                          RedirectDataMap* url_redirect_data_map,
+                          RedirectDataMap* host_redirect_data_map);
 
   // Updates data for a Url and a host. If either of the |url_data| or
-  // |host_data| has an empty primary key, it will be ignored.
-  // Note that the Urls and primary key in |url_data| and |host_data| should be
-  // less than |kMaxStringLength| in length.
+  // |host_data| or |url_redirect_data| or |host_redirect_data| has an empty
+  // primary key, it will be ignored.
+  // Note that the Urls and primary key in |url_data|, |host_data|,
+  // |url_redirect_data| and |host_redirect_data| should be less than
+  // |kMaxStringLength| in length.
   virtual void UpdateData(const PrefetchData& url_data,
-                          const PrefetchData& host_data);
+                          const PrefetchData& host_data,
+                          const RedirectData& url_redirect_data,
+                          const RedirectData& host_redirect_data);
 
   // Delete data for the input |urls| and |hosts|.
-  virtual void DeleteData(const std::vector<std::string>& urls,
-                  const std::vector<std::string>& hosts);
+  virtual void DeleteResourceData(const std::vector<std::string>& urls,
+                                  const std::vector<std::string>& hosts);
 
-  // Wrapper over DeleteData for convenience.
-  virtual void DeleteSingleDataPoint(const std::string& key,
-                                     PrefetchKeyType key_type);
+  // Wrapper over DeleteResourceData for convenience.
+  virtual void DeleteSingleResourceDataPoint(const std::string& key,
+                                             PrefetchKeyType key_type);
+
+  // Delete data for the input |urls| and |hosts|.
+  virtual void DeleteRedirectData(const std::vector<std::string>& urls,
+                                  const std::vector<std::string>& hosts);
+
+  // Wrapper over DeleteRedirectData for convenience.
+  virtual void DeleteSingleRedirectDataPoint(const std::string& key,
+                                             PrefetchKeyType key_type);
 
   // Deletes all data in all the tables.
   virtual void DeleteAllData();
 
+  // Sorts the resources by score, decreasing.
+  static void SortResources(std::vector<ResourceData>* resources);
+
+  // Sorts the redirects by score, decreasing.
+  static void SortRedirects(std::vector<RedirectStat>* redirects);
+
   // The maximum length of the string that can be stored in the DB.
   static constexpr size_t kMaxStringLength = 1024;
 
  private:
+  // Represents the type of information that is stored in prefetch database.
+  enum PrefetchDataType {
+    PREFETCH_DATA_TYPE_RESOURCE,

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

there is a new shiny c++11 way to do it:
enum class PrefetchDataType {
  RESOURCE,
  REDIRECT,
  METADATA
};

Reference them like: PrefetchDataType::RESOURCE.

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

Done.

+    PREFETCH_DATA_TYPE_REDIRECT,
+    PREFETCH_DATA_TYPE_METADATA
+  };
+
+  enum TableOperationType {
+    TABLE_OPERATION_TYPE_INSERT,
+    TABLE_OPERATION_TYPE_DELETE
+  };
+
   friend class PredictorDatabaseInternal;
   friend class MockResourcePrefetchPredictorTables;
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTablesTest,
                            DatabaseVersionIsSet);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTablesTest,
                            DatabaseIsResetWhenIncompatible);
-  FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTablesTest, ComputeScore);
+  FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTablesTest,
+                           ComputeResourceScore);
 
   ResourcePrefetchPredictorTables();
   ~ResourcePrefetchPredictorTables() override;
 
   // Helper functions below help perform functions on the Url and host table
   // using the same code.
-  void GetAllDataHelper(PrefetchKeyType key_type,
-                        PrefetchDataMap* data_map,
-                        std::vector<std::string>* to_delete);
-  bool UpdateDataHelper(const PrefetchData& data);
+  void GetAllResourceDataHelper(PrefetchKeyType key_type,
+                                PrefetchDataMap* data_map,
+                                std::vector<std::string>* to_delete);
+  void GetAllRedirectDataHelper(PrefetchKeyType key_type,
+                                RedirectDataMap* redirect_map);
+  bool UpdateResourceDataHelper(PrefetchKeyType key_type,
+                                const PrefetchData& data);
+  bool UpdateRedirectDataHelper(PrefetchKeyType key_type,
+                                const RedirectData& data);
   void DeleteDataHelper(PrefetchKeyType key_type,
+                        PrefetchDataType data_type,
                         const std::vector<std::string>& keys);
 
   // Returns true if the strings in the |data| are less than |kMaxStringLength|
   // in length.
   static bool StringsAreSmallerThanDBLimit(const PrefetchData& data);
+  static bool StringsAreSmallerThanDBLimit(const RedirectData& data);
 
   // Computes score of |data|.
-  static float ComputeScore(const ResourceData& data);
+  static float ComputeResourceScore(const ResourceData& data);
+  static float ComputeRedirectScore(const RedirectStat& data);
 
   // PredictorTableBase methods.
   void CreateTableIfNonExistent() override;
   void LogDatabaseStats() override;
 
   // Database version. Always increment it when any change is made to the data
   // schema (including the .proto).
-  static constexpr int kDatabaseVersion = 2;
+  static constexpr int kDatabaseVersion = 3;
 
   static bool DropTablesIfOutdated(sql::Connection* db);
   static int GetDatabaseVersion(sql::Connection* db);
   static bool SetDatabaseVersion(sql::Connection* db, int version);
 
-  // Helpers to return Statements for cached Statements. The caller must take
+  // Helper to return Statements for cached Statements. The caller must take
   // ownership of the return Statements.
-  sql::Statement* GetUrlResourceDeleteStatement();
-  sql::Statement* GetUrlResourceUpdateStatement();
-  sql::Statement* GetUrlMetadataDeleteStatement();
-  sql::Statement* GetUrlMetadataUpdateStatement();
+  sql::Statement* GetTableUpdateStatement(PrefetchKeyType key_type,

[Benoit L, 2016/09/27 15:19:34]

Why not return a unique_ptr<> right away here?
Then there is no need for a comment about ownership, and the call sites wrap
these in unique_ptr anyway.

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

Great! Besides, it meets object ownership convention.
Done.

+                                          PrefetchDataType data_type,
+                                          TableOperationType op_type);
 
-  sql::Statement* GetHostResourceDeleteStatement();
-  sql::Statement* GetHostResourceUpdateStatement();
-  sql::Statement* GetHostMetadataDeleteStatement();
-  sql::Statement* GetHostMetadataUpdateStatement();
+  static const char* GetTableUpdateStatementTemplate(

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

this does not need to be exposed in the interface

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

Do you suggest to move all private static functions out of class declaration?
Because there is no difference between them.

The same thing: I don't think function that's declared in class private section
is "exposed". Convince me if I'm wrong. 
Of course, if we talk about dependence on the content of .h file, then we don't
want to recompile all dependent units after changes in this function. But this
is also true for all content of private section.

+      TableOperationType op_type,
+      PrefetchDataType data_type);
+  static const char* GetTableUpdateStatementTableName(
+      PrefetchKeyType key_type,
+      PrefetchDataType data_type);
 
   DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictorTables);
 };
 
 }  // namespace predictors
 
 #endif  // CHROME_BROWSER_PREDICTORS_RESOURCE_PREFETCH_PREDICTOR_TABLES_H_