predictors: ResourcePrefetchPredictorTables cleanup.

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 349066f077d0b36ffe7496d459482a0859207aeb..43807dbdf47282af2388874c668205f1f74303fd 100644
--- a/chrome/browser/predictors/resource_prefetch_predictor_tables.h
+++ b/chrome/browser/predictors/resource_prefetch_predictor_tables.h
@@ -5,8 +5,7 @@
 #ifndef CHROME_BROWSER_PREDICTORS_RESOURCE_PREFETCH_PREDICTOR_TABLES_H_
 #define CHROME_BROWSER_PREDICTORS_RESOURCE_PREFETCH_PREDICTOR_TABLES_H_
 
-#include <stddef.h>
-
+#include <cstddef>
 #include <map>
 #include <memory>
 #include <string>
@@ -14,13 +13,9 @@
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
-#include "base/memory/ptr_util.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;
@@ -91,6 +86,9 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
   // Sorts the resources by score, decreasing.
   static void SortResources(PrefetchData* data);
 
+  // Computes score of |data|.
+  static float ComputeResourceScore(const ResourceData& data);
+
   // Removes the redirects with more than |max_consecutive_misses| consecutive
   // misses from |data|.
   static void TrimRedirects(RedirectData* data, size_t max_consecutive_misses);
@@ -98,6 +96,12 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
   // The maximum length of the string that can be stored in the DB.
   static constexpr size_t kMaxStringLength = 1024;
 
+ protected:
+  // Protected for testing. Use PredictorDatabase::resource_prefetch_tables()
+  // instead of this constructor.
+  ResourcePrefetchPredictorTables();
+  ~ResourcePrefetchPredictorTables() override;
+
  private:
   // Represents the type of information that is stored in prefetch database.
   enum class PrefetchDataType { RESOURCE, REDIRECT };
@@ -105,16 +109,14 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
   enum class TableOperationType { INSERT, REMOVE };
 
   friend class PredictorDatabaseInternal;
-  friend class MockResourcePrefetchPredictorTables;
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTablesTest,
                            DatabaseVersionIsSet);
   FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTablesTest,
                            DatabaseIsResetWhenIncompatible);
-  FRIEND_TEST_ALL_PREFIXES(ResourcePrefetchPredictorTablesTest,
-                           ComputeResourceScore);
 
-  ResourcePrefetchPredictorTables();
-  ~ResourcePrefetchPredictorTables() override;
+  // Database version. Always increment it when any change is made to the data
+  // schema (including the .proto).
+  static constexpr int kDatabaseVersion = 5;
 
   // Helper functions below help perform functions on the Url and host table
   // using the same code.
@@ -130,22 +132,15 @@ class ResourcePrefetchPredictorTables : public PredictorTableBase {
                         PrefetchDataType data_type,
                         const std::vector<std::string>& keys);
 
-  // Computes score of |data|.
-  static float ComputeResourceScore(const ResourceData& data);
-
-  // PredictorTableBase methods.
+  // PredictorTableBase:
   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 = 5;
-
-  static bool DropTablesIfOutdated(sql::Connection* db);

[Benoit L, 2016/11/04 15:03:10]

Why?

[alexilin, 2016/11/04 15:29:47]

I don't see any reason why they should be static.
It looks more consistent to have these methods as class methods than static one.
They are used only inside the class, they use table names that are database
specific. 
PredictorTableBase::DB() is passed as parameter value in ALL cases. There is no
sense to use other value.
Non-static will be just less surprising here.

[Benoit L, 2016/11/04 15:39:42]

That's a matter of preference here. I prefer to make whatever state a function
uses more explicit, thus making functions static as much as possible.

Generally speaking in cases like this, when correctness, performance or
likelihood of bugs are not affected, I tend to leave the code as it was written,
even if I would have written it differently. And yes, I may write it differently
now, even though I wrote the current (imperfect) version.

[alexilin, 2016/11/04 16:25:59]

Functional programmer detected. :)
Ok, I've got your message. Thanks.

-  static int GetDatabaseVersion(sql::Connection* db);
-  static bool SetDatabaseVersion(sql::Connection* db, int version);
+  bool DropTablesIfOutdated();
+  int GetDatabaseVersion();
+  bool SetDatabaseVersion(int version);
 
-  // Helper to return Statements for cached Statements.
+  // Helper to return cached Statements.
   std::unique_ptr<sql::Statement> GetTableUpdateStatement(
       PrefetchKeyType key_type,
       PrefetchDataType data_type,