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/logging.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/sequenced_task_runner.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/thread_task_runner_handle.h"
+#include "components/offline_pages/offline_page_item.h"
+#include "sql/connection.h"
+#include "sql/statement.h"
+#include "sql/transaction.h"
+
+namespace offline_pages {
+
+namespace {
+
+// This is a macro instead of a const so that
+// it can be used inline in other SQL statements below.
+#define OFFLINE_PAGES_TABLE_NAME "offlinepages_v1"
+
+const char kOfflinePagesColumns[] =
+    "(offline_id INTEGER PRIMARY KEY NOT NULL,"
+    " creation_time INTEGER,"
+    " file_size INTEGER,"
+    " version INTEGER,"
+    " last_access_time INTEGER,"
+    " access_count INTEGER,"
+    " status INTEGER,"
+    // a note on this field.  It will be NULL for now, and is reserved for

[Scott Hess - ex-Googler, 2016/05/02 23:33:31]

Being a comment implies that it's a note on this field, so no need to reference
that.  Also, Mark Mentovai's memory in my head requires me to complain about
comments which aren't legitimate sentences with appropriate grammar and
punctuation and stuff.

If you're going to note the un-set fields, I'd note them all above
kOfflinePagesColumns.  Or provide "NOT NULL DEFAULT x" values as appropriate.

[It might be worthwhile to have "NOT NULL" for all the fields you already do
set, to catch statement errors.  Fields should generally only be null if you
intend to actually check that case.  But I think this is a DCHECK-ish style
issue.]

+    // later use.  We will treat NULL as "Unknown" in any subsequent queries
+    // for user_initiated values.
+    " user_initiated INTEGER,"  // this is actually a boolean
+    " client_namespace VARCHAR,"
+    " client_id VARCHAR,"
+    " online_url VARCHAR,"
+    " offline_url VARCHAR,"
+    " file_path VARCHAR"
+    ")";
+
+// This is cloned from //content/browser/appcache/appcache_database.cc
+struct TableInfo {
+  const char* table_name;
+  const char* columns;
+};
+
+const TableInfo kOfflinePagesTable{OFFLINE_PAGES_TABLE_NAME,
+                                   kOfflinePagesColumns};
+
+// This enum is used to define the indices for the columns in each row
+// that hold the different pieces of offline page.
+enum : int {
+  OP_OFFLINE_ID = 0,
+  OP_CREATION_TIME,
+  OP_FILE_SIZE,
+  OP_VERSION,
+  OP_LAST_ACCESS_TIME,
+  OP_ACCESS_COUNT,
+  OP_STATUS,
+  OP_USER_INITIATED,
+  OP_CLIENT_NAMESPACE,
+  OP_CLIENT_ID,
+  OP_ONLINE_URL,
+  OP_OFFLINE_URL,
+  OP_FILE_PATH
+};
+
+bool CreateTable(sql::Connection* db, const TableInfo& table_info) {
+  std::string sql("CREATE TABLE ");
+  sql += table_info.table_name;
+  sql += table_info.columns;
+  return db->Execute(sql.c_str());
+}
+
+bool CreateSchema(sql::Connection* db) {
+  // If you create a transaction but don't Commit() it is automatically
+  // rolled back by its destructor when it falls out of scope.
+  sql::Transaction transaction(db);
+  if (!transaction.Begin())
+    return false;
+  if (db->DoesTableExist(kOfflinePagesTable.table_name))
+    return true;
+
+  if (!CreateTable(db, kOfflinePagesTable))
+    return false;
+
+  // TODO(bburns): Add indices here.
+  return transaction.Commit();
+}
+
+bool DeleteByOfflineId(sql::Connection* db, int64_t offline_id) {
+  static const char kSql[] =
+      "DELETE FROM " OFFLINE_PAGES_TABLE_NAME " WHERE offline_id=?";
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+  statement.BindInt64(0, offline_id);
+  return statement.Run();
+}
+
+// Create an offline page item from a SQL result.  Expects complete rows with
+// all columns present.
+OfflinePageItem MakeOfflinePageItem(sql::Statement* statement) {
+  int64_t id = statement->ColumnInt64(OP_OFFLINE_ID);
+  GURL url(statement->ColumnString(OP_ONLINE_URL));
+  ClientId client_id(statement->ColumnString(OP_CLIENT_NAMESPACE),
+                     statement->ColumnString(OP_CLIENT_ID));
+#if defined(OS_POSIX)
+  base::FilePath path(statement->ColumnString(OP_FILE_PATH));
+#elif defined(OS_WIN)
+  base::FilePath path(base::UTF8ToWide(statement->ColumnString(OP_FILE_PATH)));
+#else
+#error Unknown OS
+#endif
+  int64_t file_size = statement->ColumnInt64(OP_FILE_SIZE);
+  base::Time creation_time =
+      base::Time::FromInternalValue(statement->ColumnInt64(OP_CREATION_TIME));
+
+  OfflinePageItem item(url, id, client_id, path, file_size, creation_time);
+  item.last_access_time = base::Time::FromInternalValue(
+      statement->ColumnInt64(OP_LAST_ACCESS_TIME));
+  item.version = statement->ColumnInt(OP_VERSION);
+  item.access_count = statement->ColumnInt(OP_ACCESS_COUNT);
+  return item;
+}
+
+bool InsertOrReplace(sql::Connection* db, const OfflinePageItem& item) {
+  const char kSql[] =
+      "INSERT OR REPLACE INTO " OFFLINE_PAGES_TABLE_NAME
+      " (offline_id, online_url, client_namespace, client_id, file_path, "
+      "file_size, creation_time, last_access_time, version, access_count)"
+      " VALUES "
+      " (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
+
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+  statement.BindInt64(0, item.offline_id);
+  statement.BindString(1, item.url.spec());
+  statement.BindString(2, item.client_id.name_space);
+  statement.BindString(3, item.client_id.id);
+#if defined(OS_POSIX)
+  std::string path_string = item.file_path.value();
+#elif defined(OS_WIN)
+  std::string path_string = base::WideToUTF8(item.file_path.value());

[Scott Hess - ex-Googler, 2016/05/02 23:33:31]

WRT the OS_* pattern, gotta catch all the cases!

+#endif
+  statement.BindString(4, path_string);
+  statement.BindInt64(5, item.file_size);
+  statement.BindInt64(6, item.creation_time.ToInternalValue());
+  statement.BindInt64(7, item.last_access_time.ToInternalValue());
+  statement.BindInt(8, item.version);
+  statement.BindInt(9, item.access_count);
+  return statement.Run();
+}
+
+bool InitDatabase(sql::Connection* db, base::FilePath path) {
+  db->set_page_size(4096);
+  db->set_cache_size(500);
+  db->set_histogram_tag("OfflinePageMetadata");
+  db->set_exclusive_locking();
+
+  base::File::Error err;
+  if (!base::CreateDirectoryAndGetError(path.DirName(), &err)) {
+    LOG(ERROR) << "Failed to create offline pages db directory: "
+               << base::File::ErrorToString(err);
+    return false;
+  }
+  if (!db->Open(path)) {
+    LOG(ERROR) << "Failed to open database";
+    return false;
+  }
+  db->Preload();
+
+  return CreateSchema(db);
+}
+
+}  // anonymous namespace
+
+OfflinePageMetadataStoreSQL::OfflinePageMetadataStoreSQL(
+    scoped_refptr<base::SequencedTaskRunner> background_task_runner,
+    const base::FilePath& path)
+    : background_task_runner_(std::move(background_task_runner)),
+      db_file_path_(path.AppendASCII("OfflinePages.db")) {}
+
+OfflinePageMetadataStoreSQL::~OfflinePageMetadataStoreSQL() {
+  if (db_.get() &&
+      !background_task_runner_->DeleteSoon(FROM_HERE, db_.release())) {
+    DLOG(WARNING) << "SQL database will not be deleted.";
+  }
+}
+
+void OfflinePageMetadataStoreSQL::LoadSync(
+    sql::Connection* db,
+    const base::FilePath& path,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const LoadCallback& callback) {
+  if (!InitDatabase(db, path)) {
+    NotifyLoadResult(runner, callback, STORE_INIT_FAILED,
+                     std::vector<OfflinePageItem>());
+    return;
+  }
+
+  const char kSql[] = "SELECT * FROM " OFFLINE_PAGES_TABLE_NAME;
+
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+
+  std::vector<OfflinePageItem> result;
+  while (statement.Step()) {
+    result.push_back(MakeOfflinePageItem(&statement));
+  }
+
+  if (statement.Succeeded()) {
+    NotifyLoadResult(runner, callback, LOAD_SUCCEEDED, result);
+  } else {
+    NotifyLoadResult(runner, callback, STORE_LOAD_FAILED,
+                     std::vector<OfflinePageItem>());
+  }
+}
+
+void OfflinePageMetadataStoreSQL::AddOrUpdateOfflinePageSync(
+    const OfflinePageItem& offline_page,
+    sql::Connection* db,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const UpdateCallback& callback) {
+  // TODO(bburns): add UMA metrics here (and for levelDB).
+  bool ok = InsertOrReplace(db, offline_page);
+  runner->PostTask(FROM_HERE, base::Bind(callback, ok));
+}
+
+void OfflinePageMetadataStoreSQL::RemoveOfflinePagesSync(
+    const std::vector<int64_t>& offline_ids,
+    sql::Connection* db,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const UpdateCallback& callback) {
+  // TODO(bburns): add UMA metrics here (and for levelDB).
+
+  // If you create a transaction but don't Commit() it is automatically
+  // rolled back by its destructor when it falls out of scope.
+  sql::Transaction transaction(db);
+  if (!transaction.Begin()) {
+    runner->PostTask(FROM_HERE, base::Bind(callback, false));
+    return;
+  }
+  for (auto offline_id : offline_ids) {
+    if (!DeleteByOfflineId(db, offline_id)) {
+      runner->PostTask(FROM_HERE, base::Bind(callback, false));
+      return;
+    }
+  }
+
+  bool success = transaction.Commit();
+  runner->PostTask(FROM_HERE, base::Bind(callback, success));
+}
+
+void OfflinePageMetadataStoreSQL::ResetSync(
+    sql::Connection* db,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const ResetCallback& callback) {
+  const char kSql[] = "DELETE * FROM " OFFLINE_PAGES_TABLE_NAME;
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+  bool result = statement.Run();
+  if (!result) {
+    delete db;
+  }

[Scott Hess - ex-Googler, 2016/05/02 23:33:31]

Optional {}.  But the delete should not be conditional in the first place,
unless you're going to post it to someone else in case of error.  The site
posting the task already released the reference.

+  runner->PostTask(FROM_HERE, base::Bind(callback, result));
+}
+
+void OfflinePageMetadataStoreSQL::NotifyLoadResult(
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const LoadCallback& callback,
+    LoadStatus status,
+    const std::vector<OfflinePageItem>& result) {
+  // TODO(bburns): Switch to SQL specific UMA metrics.
+  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;
+  }
+  runner->PostTask(FROM_HERE, base::Bind(callback, status, result));
+}
+
+void OfflinePageMetadataStoreSQL::Load(const LoadCallback& callback) {
+  db_.reset(new sql::Connection());
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&OfflinePageMetadataStoreSQL::LoadSync, db_.get(),
+                 db_file_path_, base::ThreadTaskRunnerHandle::Get(), callback));
+}
+
+void OfflinePageMetadataStoreSQL::AddOrUpdateOfflinePage(
+    const OfflinePageItem& offline_page,
+    const UpdateCallback& callback) {
+  DCHECK(db_.get());

[Scott Hess - ex-Googler, 2016/05/02 23:33:31]

Previously, SetInMemoryDatabaseForTesting() used DCHECK(db_.get() == nullptr). 
Personally I am fine with the above as is, I'm just calling it out in case
someone previously made suggestions which led to that formulation.  Also below,
if you need a change.

+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&OfflinePageMetadataStoreSQL::AddOrUpdateOfflinePageSync,
+                 offline_page, db_.get(), base::ThreadTaskRunnerHandle::Get(),
+                 callback));
+}
+
+void OfflinePageMetadataStoreSQL::RemoveOfflinePages(
+    const std::vector<int64_t>& offline_ids,
+    const UpdateCallback& callback) {
+  DCHECK(db_.get());
+
+  if (offline_ids.empty()) {
+    // Nothing to do, but post a callback instead of calling directly
+    // to preserve the async style behavior to prevent bugs.
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  base::Bind(callback, true));
+    return;
+  }
+
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&OfflinePageMetadataStoreSQL::RemoveOfflinePagesSync,
+                 offline_ids, db_.get(), base::ThreadTaskRunnerHandle::Get(),
+                 callback));
+}
+
+void OfflinePageMetadataStoreSQL::Reset(const ResetCallback& callback) {
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&OfflinePageMetadataStoreSQL::ResetSync, db_.release(),
+                 base::ThreadTaskRunnerHandle::Get(), callback));
+}
+
+}  // namespace offline_pages