Redirect handling in the resource_prefetch_predictor.

chrome/browser/predictors/resource_prefetch_predictor_tables.h

Index: chrome/browser/predictors/resource_prefetch_predictor_tables.h
diff --git a/chrome/browser/predictors/resource_prefetch_predictor_tables.h b/chrome/browser/predictors/resource_prefetch_predictor_tables.h
index b56ed4cf37d943a9a481f8902cb47b1a42016397..0df43f218628508f0f7eb60460dcd09c75dc18ad 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_tables.h
+++ b/chrome/browser/predictors/resource_prefetch_predictor_tables.h
@@ -28,6 +28,8 @@ class Statement;
 namespace predictors {
 
 using chrome_browser_predictors::ResourceData;
+using chrome_browser_predictors::RedirectData;
+using RedirectStat = chrome_browser_predictors::RedirectData::RedirectStat;

[pasko, 2016/09/22 14:27:09]

why not using chrome_browser_predictors::RedirectData::RedirectStat?

[alexilin, 2016/09/22 16:48:19]

Because compiler doesn't like this: "error: using declaration cannot refer to
class member"

[pasko, 2016/09/26 12:28:15]

Oh, I did not know it, thanks!

[alexilin, 2016/09/26 15:38:28]

Done.

 
 // Interface for database tables used by the ResourcePrefetchPredictor.
 // All methods except the constructor and destructor need to be called on the DB
@@ -36,13 +38,18 @@ using chrome_browser_predictors::ResourceData;
 // 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);
 
+  // Sorts the redirects by score, decreasing.
+  static void SortRedirects(std::vector<RedirectStat>* redirects);

[pasko, 2016/09/22 14:27:09]

typedefs and struct declarations should go before methods, see:

https://google.github.io/styleguide/cppguide.html#Declaration_Order

[alexilin, 2016/09/22 16:48:19]

Ok!
Btw, I don't see nested struct/class in this list.

[pasko, 2016/09/26 12:28:15]

This was confusing to me as well, I would put them in the same category as
'Typedefs and Enums'.

[alexilin, 2016/09/26 15:38:28]

Done.

+
   // 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.
@@ -63,24 +70,41 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
   // 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 DeleteResourceData(const std::vector<std::string>& urls,
+                                  const std::vector<std::string>& hosts);
+
+  // 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 DeleteData(const std::vector<std::string>& urls,
-                  const std::vector<std::string>& hosts);
+  virtual void DeleteRedirectData(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 DeleteRedirectData for convenience.
+  virtual void DeleteSingleRedirectDataPoint(const std::string& key,
+                                             PrefetchKeyType key_type);
 
   // Deletes all data in all the tables.
   virtual void DeleteAllData();
@@ -95,26 +119,36 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
                            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 DeleteDataHelper(PrefetchKeyType key_type,
-                        const std::vector<std::string>& keys);
+  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 DeleteResourceDataHelper(PrefetchKeyType key_type,
+                                const std::vector<std::string>& keys);
+  void DeleteRedirectDataHelper(PrefetchKeyType key_type,

[pasko, 2016/09/22 14:27:09]

these two Delete* methods do the same thing with a minor variation of deleting
from the metadata table. To avoid code duplication it feels better to joing them
into one function:

Something like:
DeleteDataHelper(key_type, value_type, keys)

And:
* either provide bool delete_from_metadata parameter
* or introduce DeleteMetadataHelper(key_type, keys)

up to you on the latter

[alexilin, 2016/09/22 16:48:19]

About all of these helpers... Honestly, I hope in the future to get rid of
PrefetchData structure as well (and use another proto instead of it). Then we
could delete 'resource_url' column from resource table and get unified table
scheme (TEXT, BLOB). After that we could introduce unified helpers that will
take base proto MessageLite class.
But for now I'll change it as you proposed.

+                                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);
+  // Computes score of |data|

[pasko, 2016/09/22 14:27:09]

I guess some sort of merging/rebasing decided to revert your previous change
here. So now I finally provided a useful comment.

[alexilin, 2016/09/22 16:48:19]

Oh nooo! My period is GONE!

[alexilin, 2016/09/26 15:38:28]

Done.

+  static float ComputeResourceScore(const ResourceData& data);
+  static float ComputeRedirectScore(const RedirectStat& data);
 
   // PredictorTableBase methods.
   void CreateTableIfNonExistent() override;
@@ -122,7 +156,7 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
 
   // 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);
@@ -134,11 +168,15 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
   sql::Statement* GetUrlResourceUpdateStatement();
   sql::Statement* GetUrlMetadataDeleteStatement();
   sql::Statement* GetUrlMetadataUpdateStatement();
+  sql::Statement* GetUrlRedirectDeleteStatement();
+  sql::Statement* GetUrlRedirectUpdateStatement();
 
   sql::Statement* GetHostResourceDeleteStatement();
   sql::Statement* GetHostResourceUpdateStatement();
   sql::Statement* GetHostMetadataDeleteStatement();
   sql::Statement* GetHostMetadataUpdateStatement();
+  sql::Statement* GetHostRedirectDeleteStatement();
+  sql::Statement* GetHostRedirectUpdateStatement();
 
   DISALLOW_COPY_AND_ASSIGN(ResourcePrefetchPredictorTables);
 };