initial add of SQL based storage

components/offline_pages/offline_page_metadata_store_sql.cc

+// Copyright 2016 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.
+
+#include "components/offline_pages/offline_page_metadata_store_sql.h"
+
+#include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/location.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/sequenced_task_runner.h"
+#include "components/offline_pages/offline_page_item.h"
+#include "sql/connection.h"
+#include "sql/error_delegate_util.h"
+#include "sql/meta_table.h"
+#include "sql/statement.h"
+#include "sql/transaction.h"
+
+namespace offline_pages {
+
+namespace {
+
+const int kCurrentVersion = 1;
+const int kCompatibleVersion = 1;
+
+const char kTableName[] = "offlinepages";

[fgorski, 2016/03/24 16:27:54]

kOfflinePagesTable(Name)

[bburns, 2016/03/25 23:07:37]

Done.

+const char kColumns[] =

[fgorski, 2016/03/24 16:27:55]

kOfflinePagesTableColumns

[bburns, 2016/03/25 23:07:38]

Done.

+    "(offline_id INTEGER PRIMARY_KEY,"
+    " client_namespace VARCHAR(256),"
+    " client_id VARCHAR(256),"
+    " online_url VARCHAR(2048),"
+    " offline_url VARCHAR(2048),"
+    " version INTEGER,"
+    " creation_time INTEGER,"
+    " file_path VARCHAR(1024),"
+    " file_size INTEGER,"
+    " last_access_time INTEGER,"
+    " access_count INTEGER,"
+    " status INTEGER,"
+    " user_initiated BOOLEAN)";
+
+#define OFFLINE_ID 0

[fgorski, 2016/03/24 16:27:55]

Add table prefix, e.g. OP_

It will come in handy when we have multiple tables.

(Alternatively add a todo in case we need to disambiguate in the future).

[Dmitry Titov, 2016/03/24 23:22:20]

I believe we this form is preferable lately in Chrome:

enum class int { OFFLIE_ID = 0, ... 

[bburns, 2016/03/25 23:07:37]

Done.

[bburns, 2016/03/25 23:07:38]

Done.

+#define CLIENT_NAMESPACE 1
+#define CLIENT_ID 2
+#define ONLINE_URL 3
+#define OFFLINE_URL 4
+#define VERSION 5
+#define CREATION_TIME 6
+#define FILE_PATH 7
+#define FILE_SIZE 8
+#define LAST_ACCESS 9
+#define ACCESS_COUNT 10
+#define STATUS 11
+#define USER_INITIATED 12
+
+bool CreateTable(sql::Connection* db,
+                 const char* table_name,
+                 const char* columns) {
+  std::string sql("CREATE TABLE ");
+  sql += table_name;
+  sql += columns;
+  return db->Execute(sql.c_str());
+}
+
+bool CreateSchema(sql::MetaTable* meta_table, sql::Connection* db) {
+  sql::Transaction transaction(db);
+  if (!transaction.Begin())
+    return false;
+
+  if (!meta_table->Init(db, kCurrentVersion, kCompatibleVersion))
+    return false;
+
+  if (!CreateTable(db, kTableName, kColumns))
+    return false;
+
+  // TODO: indexes here

[Dmitry Titov, 2016/03/24 23:22:20]

// TODO(bburns): ....

[bburns, 2016/03/25 23:07:37]

Done.

+  return transaction.Commit();
+}
+
+bool DropTable(sql::Connection* db, const char* table_name) {
+  std::string sql("DROP TABLE ");
+  sql += table_name;
+  return db->Execute(sql.c_str());
+}
+
+bool DeleteOfflineId(sql::Connection* db, int64_t offline_id) {

[fgorski, 2016/03/24 16:27:55]

DeleteByOfflineId

[bburns, 2016/03/25 23:07:37]

Done.

+  std::string sql("DELETE FROM ");

[fgorski, 2016/03/24 16:27:54]

this whole statement should be a const with " offline_id = ?"
apply BindInt64 in this method.

[bburns, 2016/03/25 23:07:38]

Done.

+  sql += kTableName;
+  sql += " WHERE offline_id=";
+  sql += offline_id;
+  return db->Execute(sql.c_str());
+}
+
+OfflinePageItem MakeOfflinePageItem(sql::Statement* statement) {
+  int64_t id = statement->ColumnInt64(OFFLINE_ID);
+  GURL url(statement->ColumnString(ONLINE_URL));
+  ClientId client_id(statement->ColumnString(CLIENT_NAMESPACE),
+                     statement->ColumnString(CLIENT_ID));
+  base::FilePath path(statement->ColumnString(FILE_PATH));
+  int64_t file_size = statement->ColumnInt64(FILE_SIZE);
+  base::Time creation =

[Dmitry Titov, 2016/03/24 23:22:20]

creationTime

[bburns, 2016/03/25 23:07:37]

Done.

+      base::Time::FromDoubleT(statement->ColumnInt64(CREATION_TIME));

[fgorski, 2016/03/24 16:27:54]

base::Time::FromInternalValue(statement->ColumnInt64(CREATION_TIME));

[bburns, 2016/03/25 23:07:37]

Done.

+
+  return OfflinePageItem(url, id, client_id, path, file_size, creation);
+}
+
+bool InsertOrReplace(sql::Connection* db, const OfflinePageItem& item) {
+  std::string sql = "INSERT OR REPLACE INTO ";

[fgorski, 2016/03/24 16:27:55]

extract as constant the whole sql statement.

[bburns, 2016/03/25 23:07:37]

Done.

+  sql += kTableName;
+  sql +=
+      " (offline_id, online_url, client_namespace, client_id, file_path, "
+      "file_size, creation_time)"

[fgorski, 2016/03/24 16:27:55]

what about other fields?

[bburns, 2016/03/25 23:07:38]

added a couple more.  The database is actually a superset of what is in
offline_page_item.h right now.  We'll update that as part of a subsequent
change.

+      " VALUES "
+      " (?, ?, ?, ?, ?, ?, ?)";
+
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, sql.c_str()));
+  statement.BindInt64(OFFLINE_ID, item.offline_id);
+  statement.BindString(ONLINE_URL, item.url.spec());
+  statement.BindString(CLIENT_NAMESPACE, item.client_id.name_space);
+  statement.BindString(CLIENT_ID, item.client_id.id);
+  statement.BindString(FILE_PATH, item.file_path.value());
+  statement.BindInt64(FILE_SIZE, item.file_size);
+  statement.BindInt64(CREATION_TIME, (int64_t)item.creation_time.ToDoubleT());

[fgorski, 2016/03/24 16:27:55]

item.creation_time.ToInternalValue();

[bburns, 2016/03/25 23:07:37]

Done.

+
+  return statement.Run();
+}
+
+}  // anonymous namespace
+
+OfflinePageMetadataStoreSQL::OfflinePageMetadataStoreSQL(
+    scoped_refptr<base::SequencedTaskRunner> background_task_runner,
+    const base::FilePath& path)
+    : background_task_runner_(background_task_runner),
+      db_file_path_(path),
+      weak_ptr_factory_(this) {}
+
+OfflinePageMetadataStoreSQL::~OfflinePageMetadataStoreSQL() {}
+
+void OfflinePageMetadataStoreSQL::LoadSync(const LoadCallback& callback) {
+  // TODO: should this be async?

[fgorski, 2016/03/24 16:27:54]

nit: TODO(bburns): ;)

[bburns, 2016/03/25 23:07:37]

Removed.

+  bool opened = false;
+  db_.reset(new sql::Connection);
+  meta_table_.reset(new sql::MetaTable);
+
+  if (use_in_memory_) {
+    opened = db_->OpenInMemory();
+  } else if (!base::CreateDirectory(db_file_path_.DirName())) {
+    LOG(ERROR) << "Failed to create appcache directory.";

[fgorski, 2016/03/24 16:27:54]

I admire your Copy-Paste Fu ;)

also, I actually like how kTables are defined in appcache_database.cc perhaps we
should get closer to that model. 

[bburns, 2016/03/25 23:07:38]

Done.

+  } else {
+    opened = db_->Open(db_file_path_);
+    if (opened)
+      db_->Preload();
+  }
+  if (!opened) {
+    NotifyLoadResult(callback, STORE_INIT_FAILED,
+                     std::vector<OfflinePageItem>());
+  }
+
+  if (!sql::MetaTable::DoesTableExist(db_.get())) {
+    if (!CreateSchema(meta_table_.get(), db_.get())) {
+      NotifyLoadResult(callback, STORE_INIT_FAILED,
+                       std::vector<OfflinePageItem>());
+    }
+  }
+
+  if (!meta_table_->Init(db_.get(), kCurrentVersion, kCompatibleVersion))
+    NotifyLoadResult(callback, STORE_INIT_FAILED,

[fgorski, 2016/03/24 16:27:54]

seems like we will not be able to distinguish these 4 cases using UMA.

2 ways to go about it:
* Define a separate UMA for SQL store
* Extend existing LoadStatus.

Best to ask Jian Li which is better. Worst case he will consult with UMA folks.

[bburns, 2016/03/25 23:07:37]

This is still TBD, but will add before final review.

[fgorski, 2016/03/29 06:50:18]

At least put a visible TODO for now, please

+                     std::vector<OfflinePageItem>());
+
+  if (meta_table_->GetCompatibleVersionNumber() > kCurrentVersion) {
+    LOG(WARNING) << "Offline database is too new.";
+    NotifyLoadResult(callback, STORE_INIT_FAILED,
+                     std::vector<OfflinePageItem>());
+  }
+
+  std::string sql("SELECT * FROM ");

[fgorski, 2016/03/24 16:27:54]

extract const char [] 

[bburns, 2016/03/25 23:07:37]

Done.

+  sql += kTableName;
+
+  sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, sql.c_str()));
+
+  std::vector<OfflinePageItem> result;
+  while (statement.Step()) {
+    result.push_back(MakeOfflinePageItem(&statement));
+  }
+
+  if (!statement.Succeeded()) {
+    NotifyLoadResult(callback, STORE_INIT_FAILED,

[fgorski, 2016/03/24 16:27:55]

STORE_LOAD_FAILED?

[bburns, 2016/03/25 23:07:37]

Done.

+                     std::vector<OfflinePageItem>());
+  }

[fgorski, 2016/03/24 16:27:55]

When do we notify of LOAD_SUCCEEDED?

[bburns, 2016/03/25 23:07:37]

Done.

+}
+
+void OfflinePageMetadataStoreSQL::AddOrUpdateOfflinePageSync(
+    const OfflinePageItem& offline_page,
+    const UpdateCallback& callback) {
+  bool ok = InsertOrReplace(db_.get(), offline_page);
+  callback.Run(ok);

[fgorski, 2016/03/24 16:27:54]

you are on the background_task_runner_ here I don't think this is a valid place
to call a callback. I made a comment later where you should work out proper post
back.

[bburns, 2016/03/25 23:07:38]

Done.

+}
+
+void OfflinePageMetadataStoreSQL::RemoveOfflinePagesSync(
+    const std::vector<int64_t>& offline_ids,
+    const UpdateCallback& callback) {
+  sql::Transaction transaction(db_.get());
+
+  for (auto offline_id : offline_ids) {
+    if (!DeleteOfflineId(db_.get(), offline_id)) {

[fgorski, 2016/03/24 16:27:54]

will this cancel the whole transaction if nothing got deleted?

[bburns, 2016/03/25 23:07:37]

Fixed with an early exit.

+      callback.Run(false);
+      return;
+    }
+  }
+
+  if (!transaction.Commit()) {
+    callback.Run(false);
+  }
+  callback.Run(true);
+}
+
+void OfflinePageMetadataStoreSQL::Reset(const ResetCallback& callback) {
+  bool success = DropTable(db_.get(), kTableName);
+  callback.Run(success);
+}
+
+void OfflinePageMetadataStoreSQL::NotifyLoadResult(
+    const LoadCallback& callback,
+    LoadStatus status,
+    const std::vector<OfflinePageItem>& result) {

[fgorski, 2016/03/24 16:27:55]

probably worth adding UMA histogram for load time.

Also worth doing that for other operations in both cases (LevelDB and SQLite)

[bburns, 2016/03/25 23:07:37]

As above working on getting UMA together.

+  UMA_HISTOGRAM_ENUMERATION("OfflinePages.LoadStatus", status,
+                            OfflinePageMetadataStore::LOAD_STATUS_COUNT);
+  if (status == LOAD_SUCCEEDED) {
+    UMA_HISTOGRAM_COUNTS("OfflinePages.SavedPageCount", result.size());
+  } else {
+    DVLOG(1) << "Offline pages database loading failed: " << status;
+    db_.reset();
+  }
+  callback.Run(status, result);
+}
+
+void OfflinePageMetadataStoreSQL::Load(const LoadCallback& callback) {
+  background_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&OfflinePageMetadataStoreSQL::LoadSync,
+                            weak_ptr_factory_.GetWeakPtr(), callback));

[fgorski, 2016/03/24 16:27:54]

applies to next 3 calls: which thread is callback going to be posted to?

[bburns, 2016/03/25 23:07:37]

Done.

+}
+
+void OfflinePageMetadataStoreSQL::AddOrUpdateOfflinePage(
+    const OfflinePageItem& offline_page,
+    const UpdateCallback& callback) {
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&OfflinePageMetadataStoreSQL::AddOrUpdateOfflinePageSync,
+                 weak_ptr_factory_.GetWeakPtr(), offline_page, callback));
+}
+
+void OfflinePageMetadataStoreSQL::RemoveOfflinePages(
+    const std::vector<int64_t>& offline_ids,
+    const UpdateCallback& callback) {
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&OfflinePageMetadataStoreSQL::RemoveOfflinePagesSync,
+                 weak_ptr_factory_.GetWeakPtr(), offline_ids, callback));
+}
+
+}  // namespace offline_pages