predictors: Extract sql key-value tables into separate class.

chrome/browser/predictors/glowplug_key_value_table.h

Index: chrome/browser/predictors/glowplug_key_value_table.h
diff --git a/chrome/browser/predictors/glowplug_key_value_table.h b/chrome/browser/predictors/glowplug_key_value_table.h
new file mode 100644
index 0000000000000000000000000000000000000000..b34a9222987676555104f0891d0d7c4e577391c3
--- /dev/null
+++ b/chrome/browser/predictors/glowplug_key_value_table.h
@@ -0,0 +1,125 @@
+// Copyright 2017 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_GLOWPLUG_KEY_VALUE_TABLE_H_
+#define CHROME_BROWSER_PREDICTORS_GLOWPLUG_KEY_VALUE_TABLE_H_
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include "base/strings/stringprintf.h"
+#include "content/public/browser/browser_thread.h"
+#include "sql/statement.h"
+
+namespace predictors {
+
+namespace internal {
+
+template <typename T>
+void BindDataToStatement(const std::string& key,

[Benoit L, 2017/04/27 15:49:02]

Does this need to be a template?

[alexilin, 2017/04/28 14:54:52]

Alternatives? I could use protobuf::MessageLite instead of T here. Does it sound
good?
Other way is just to inline the code of this function because it's called only
once.

[Benoit L, 2017/04/28 15:12:34]

Thanks, that's better this way. The function is indeed called only once, but
since it's from a template, that adds up.

+                         const T& data,
+                         sql::Statement* statement) {
+  int size = data.ByteSize();
+  DCHECK_GT(size, 0);
+  std::vector<char> proto_buffer(size);
+  data.SerializeToArray(&proto_buffer[0], size);
+
+  statement->BindString(0, key);
+  statement->BindBlob(1, &proto_buffer[0], size);
+}
+
+}  // namespace internal
+
+template <typename T>
+class GlowplugKeyValueTable {
+ public:
+  GlowplugKeyValueTable(const std::string& table_name, sql::Connection* db);
+  virtual void GetAllData(std::map<std::string, T>* data_map) const;

[Benoit L, 2017/04/27 15:49:02]

nit: Why virtual?

[alexilin, 2017/04/28 14:54:52]

Eventually, for testing.
Added a comment, thanks!

+  virtual void UpdateData(const std::string& key, const T& data);
+  virtual void DeleteData(const std::vector<std::string>& keys);
+  virtual void DeleteAllData();
+
+ private:
+  std::string GetSelectAllSql() const {

[Benoit L, 2017/04/27 15:49:02]

Can these methods not be templates?
I mean, can they be outside the template class?

[alexilin, 2017/04/28 14:54:52]

Then I have to move the implementation of these methods to .cc file.
Done.

+    return base::StringPrintf("SELECT * FROM %s", table_name_.c_str());
+  }
+  std::string GetInsertSql() const {
+    return base::StringPrintf("INSERT INTO %s (key, proto) VALUES (?,?)",
+                              table_name_.c_str());
+  }
+  std::string GetDeleteSql() const {
+    return base::StringPrintf("DELETE FROM %s WHERE key=?",
+                              table_name_.c_str());
+  }
+  std::string GetDeleteAllSql() const {
+    return base::StringPrintf("DELETE FROM %s", table_name_.c_str());
+  }
+
+  const std::string table_name_;
+  sql::Connection* db_;
+
+  DISALLOW_COPY_AND_ASSIGN(GlowplugKeyValueTable);
+};
+
+template <typename T>
+GlowplugKeyValueTable<T>::GlowplugKeyValueTable(const std::string& table_name,
+                                                sql::Connection* db)
+    : table_name_(table_name), db_(db) {}
+
+template <typename T>
+void GlowplugKeyValueTable<T>::GetAllData(
+    std::map<std::string, T>* data_map) const {
+  sql::Statement reader(db_->GetUniqueStatement(GetSelectAllSql().c_str()));
+  while (reader.Step()) {
+    auto it = data_map->emplace(reader.ColumnString(0), T()).first;
+    int size = reader.ColumnByteLength(1);
+    const void* blob = reader.ColumnBlob(1);
+    DCHECK(blob);
+    it->second.ParseFromArray(blob, size);
+  }
+}
+
+template <typename T>
+void GlowplugKeyValueTable<T>::UpdateData(const std::string& key,
+                                          const T& data) {
+  db_->BeginTransaction();
+
+  // Delete the older data from the table.

[Benoit L, 2017/04/27 15:49:02]

Thinking about it, why do we delete and then insert?
Couldn't we use "INSERT OR REPLACE"?

Then we don't even need a transaction I believe.

[alexilin, 2017/04/28 16:27:25]

Done.

+  sql::Statement deleter(db_->GetUniqueStatement(GetDeleteSql().c_str()));
+  deleter.BindString(0, key);
+  bool success = deleter.Run();
+
+  if (success) {
+    // Add the new data to the table.
+    sql::Statement inserter(db_->GetUniqueStatement(GetInsertSql().c_str()));
+    ::predictors::internal::BindDataToStatement(key, data, &inserter);
+    success = inserter.Run();
+  }
+
+  if (!success)
+    db_->RollbackTransaction();
+  else
+    db_->CommitTransaction();
+}
+
+template <typename T>
+void GlowplugKeyValueTable<T>::DeleteData(
+    const std::vector<std::string>& keys) {
+  for (const auto& key : keys) {
+    sql::Statement deleter(db_->GetUniqueStatement(GetDeleteSql().c_str()));

[Benoit L, 2017/04/27 15:49:03]

nit: put the statement outside the loop? Otherwise we do N unnecessary SQL
calls.

We could even cache them in the DB class if we think creating them is expensive.
It can be on complex DBs, don't think it should be too bad with SQLite though.

[alexilin, 2017/04/28 14:54:52]

Put the statement outside the loop - done.

+    deleter.BindString(0, key);
+    deleter.Run();
+  }
+}
+
+template <typename T>
+void GlowplugKeyValueTable<T>::DeleteAllData() {
+  sql::Statement deleter(db_->GetUniqueStatement(GetDeleteAllSql().c_str()));
+  deleter.Run();
+}
+
+}  // namespace predictors
+
+#endif  // CHROME_BROWSER_PREDICTORS_GLOWPLUG_KEY_VALUE_TABLE_H_