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 "base/thread_task_runner_handle.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;
+
+// This is a #define instead of a const so that

[jianli, 2016/03/31 23:40:54]

nit: #define => macro

[bburns, 2016/04/01 17:12:56]

Done.

+// it can be used inline in other SQL statements below.
+#define kOfflinePagesTableName "offlinepages"

[jianli, 2016/03/31 23:40:54]

Add an empty line to separate from other constants. Also consider name it as
OFFLINE_PAGES_TABLE_NAME to differentiate with constants.

[bburns, 2016/04/01 17:12:56]

Done.

+const char kOfflinePagesColumns[] =
+    "(offline_id INTEGER NOT NULL,"
+    " 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,"
+    " UNIQUE(offline_id))";
+
+// This is cloned from //content/browser/appcache/appcache_database.cc
+struct TableInfo {
+  const char* table_name;
+  const char* columns;
+};
+
+const TableInfo kOfflinePagesTable{kOfflinePagesTableName,
+                                   kOfflinePagesColumns};
+
+enum : int {

[jianli, 2016/03/31 23:40:54]

Please comment on this enum.

[bburns, 2016/04/01 17:12:56]

Done.

+  OP_OFFLINE_ID = 0,
+  OP_CLIENT_NAMESPACE,
+  OP_CLIENT_ID,
+  OP_ONLINE_URL,
+  OP_OFFLINE_URL,
+  OP_VERSION,
+  OP_CREATION_TIME,
+  OP_FILE_PATH,
+  OP_FILE_SIZE,
+  OP_LAST_ACCESS_TIME,
+  OP_ACCESS_COUNT,
+  OP_STATUS,
+  OP_USER_INITIATED
+};
+
+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) {
+  // 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 (!meta_table->Init(db, kCurrentVersion, kCompatibleVersion))
+    return false;
+
+  if (!CreateTable(db, kOfflinePagesTableName, kOfflinePagesColumns))
+    return false;
+
+  // TODO(bburns): indexes here

[jianli, 2016/03/31 23:40:54]

nit: end with period

[bburns, 2016/04/01 17:12:56]

Done.

+  return transaction.Commit();
+}
+
+bool DeleteByOfflineId(sql::Connection* db, int64_t offline_id) {
+  const char kSql[] =
+      "DELETE FROM " kOfflinePagesTableName " 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 = base::FilePath(statement->ColumnString(OP_FILE_PATH));

[jianli, 2016/03/31 23:40:54]

Can we use AppendASCII to simply the logic here?

[bburns, 2016/04/01 17:12:56]

Simplified into just a declaration.

+#elif defined(OS_WIN)
+  base::FilePath path =
+      base::FilePath(base::UTF8ToWide(statement->ColumnString(OP_FILE_PATH)));
+#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->ColumnInt64(OP_VERSION);

[jianli, 2016/03/31 23:40:54]

Should this be ColumnInt?

[bburns, 2016/04/01 17:12:56]

Done.

+  item.access_count = statement->ColumnInt64(OP_ACCESS_COUNT);
+  return item;
+}
+
+bool InsertOrReplace(sql::Connection* db, const OfflinePageItem& item) {
+  const char kSql[] =
+      "INSERT OR REPLACE INTO " kOfflinePagesTableName
+      " (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);
+  std::string path_string;
+#if defined(OS_POSIX)
+  path_string = item.file_path.value();

[jianli, 2016/03/31 23:40:54]

Is it better to call AsUTF8Unsafe?

[bburns, 2016/04/01 17:12:55]

This is a copy of the code in OfflinePageMetadataImpl, so I prefer consistency. 
If we want to make this change, let's do it in both places in a different CL.

+#elif defined(OS_WIN)
+  path_string = base::WideToUTF8(item.file_path.value());
+#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.BindInt64(8, item.version);

[jianli, 2016/03/31 23:40:55]

Should this be int?

[bburns, 2016/04/01 17:12:56]

Done.

+  statement.BindInt64(9, item.access_count);
+  return statement.Run();
+}

[jianli, 2016/03/31 23:40:55]

nit: add an empty line

[bburns, 2016/04/01 17:12:56]

Done.

+}  // 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.AppendASCII("OfflinePages.db")),
+      use_in_memory_(false),
+      weak_ptr_factory_(this) {}
+
+OfflinePageMetadataStoreSQL::~OfflinePageMetadataStoreSQL() {}

[jianli, 2016/03/31 23:40:54]

This is invoked from main thread from which we cannot destruct db objects.

[bburns, 2016/04/01 17:12:56]

why not?  It's just an in-memory object...

[jianli, 2016/04/01 20:57:59]

When sql::Connection object is destructed, Connection::CloseInternal will be
called to close the database if not yet, which will calls AssertIOAllowed to
make sure the work is done from IO thread. So I think you will get an assert if
running debug build.

[bburns, 2016/04/01 22:51:44]

Done.

+
+void OfflinePageMetadataStoreSQL::LoadSync(
+    scoped_refptr<base::SingleThreadTaskRunner> runner,

[jianli, 2016/03/31 23:40:54]

Rename runner to foreground_runner to improve readability

[bburns, 2016/04/01 17:12:56]

I disagree with this.  The code doesn't care which runner is being used, it
simply uses it.

If you feel strongly, I will change it.

+    const LoadCallback& callback) {
+  bool opened = false;
+  db_.reset(new sql::Connection);
+  meta_table_.reset(new sql::MetaTable);
+
+  if (use_in_memory_) {
+    opened = db_->OpenInMemory();
+  } else {
+    base::File::Error err;
+    if (!base::CreateDirectoryAndGetError(db_file_path_.DirName(), &err)) {
+      LOG(ERROR) << "Failed to create offline pages db directory: "
+                 << base::File::ErrorToString(err);
+    } else {
+      opened = db_->Open(db_file_path_);
+      if (opened)
+        db_->Preload();
+    }
+  }
+  if (!opened) {
+    LOG(ERROR) << "Failed to open database";
+    NotifyLoadResult(runner, callback, STORE_INIT_FAILED,
+                     std::vector<OfflinePageItem>());
+    return;
+  }
+
+  if (!sql::MetaTable::DoesTableExist(db_.get())) {
+    if (!CreateSchema(meta_table_.get(), db_.get())) {
+      LOG(ERROR) << "Failed to create schema";
+      NotifyLoadResult(runner, callback, STORE_INIT_FAILED,
+                       std::vector<OfflinePageItem>());
+      return;
+    }
+  } else if (!meta_table_->Init(db_.get(), kCurrentVersion,
+                                kCompatibleVersion)) {
+    LOG(ERROR) << "Failed to initialize database";
+    NotifyLoadResult(runner, callback, STORE_INIT_FAILED,
+                     std::vector<OfflinePageItem>());
+    return;
+  }
+
+  if (meta_table_->GetCompatibleVersionNumber() > kCurrentVersion) {
+    LOG(WARNING) << "Offline database is too new.";
+    NotifyLoadResult(runner, callback, STORE_INIT_FAILED,
+                     std::vector<OfflinePageItem>());
+    return;
+  }
+
+  const char kSql[] = "SELECT * FROM " kOfflinePagesTableName;
+
+  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, STORE_LOAD_FAILED,

[jianli, 2016/03/31 23:40:54]

nit: swap then part and else part for better readability

[bburns, 2016/04/01 17:12:56]

Done.

+                     std::vector<OfflinePageItem>());
+  } else {
+    NotifyLoadResult(runner, callback, LOAD_SUCCEEDED, result);
+  }
+}
+
+void OfflinePageMetadataStoreSQL::AddOrUpdateOfflinePageSync(
+    const OfflinePageItem& offline_page,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const UpdateCallback& callback) {

[jianli, 2016/03/31 23:40:54]

Add DCHECK for db_.

[bburns, 2016/04/01 17:12:56]

Done.

+  // TODO(bburns): add UMA metrics here (and for levelDB)

[jianli, 2016/03/31 23:40:54]

nit: end comment with period

[bburns, 2016/04/01 17:12:56]

Done.

+  bool ok = InsertOrReplace(db_.get(), offline_page);
+  runner->PostTask(FROM_HERE, base::Bind(callback, ok));
+}
+
+void OfflinePageMetadataStoreSQL::RemoveOfflinePagesSync(
+    const std::vector<int64_t>& offline_ids,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const UpdateCallback& callback) {
+  // TODO(bburns): add UMA metrics here (and for levelDB)

[jianli, 2016/03/31 23:40:54]

ditto

[bburns, 2016/04/01 17:12:56]

Done.

+
+  // 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_.get());
+  if (!transaction.Begin()) {
+    runner->PostTask(FROM_HERE, base::Bind(callback, false));

[jianli, 2016/03/31 23:40:54]

Do we want to bail out now?

[bburns, 2016/04/01 17:12:56]

Done.

+  }
+  for (auto offline_id : offline_ids) {
+    if (!DeleteByOfflineId(db_.get(), 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(
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const ResetCallback& callback) {
+  meta_table_.reset(NULL);
+  db_.reset(NULL);

[jianli, 2016/03/31 23:40:54]

We need to wipe out the database.

[bburns, 2016/04/01 17:12:56]

Done.

+  weak_ptr_factory_.InvalidateWeakPtrs();
+
+  runner->PostTask(FROM_HERE, base::Bind(callback, true));
+}
+
+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;
+    db_.reset();

[jianli, 2016/03/31 23:40:54]

Do we also need to reset meta_table_?

[bburns, 2016/04/01 17:12:56]

Done.

+  }
+  runner->PostTask(FROM_HERE, base::Bind(callback, status, result));
+}
+
+void OfflinePageMetadataStoreSQL::Load(const LoadCallback& callback) {
+  background_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&OfflinePageMetadataStoreSQL::LoadSync,
+                            weak_ptr_factory_.GetWeakPtr(),
+                            base::ThreadTaskRunnerHandle::Get(), callback));
+}
+
+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,
+                 base::ThreadTaskRunnerHandle::Get(), callback));
+}
+
+void OfflinePageMetadataStoreSQL::RemoveOfflinePages(
+    const std::vector<int64_t>& offline_ids,
+    const UpdateCallback& callback) {
+  if (offline_ids.size() == 0) {
+    // 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,
+                 weak_ptr_factory_.GetWeakPtr(), offline_ids,
+                 base::ThreadTaskRunnerHandle::Get(), callback));
+}
+
+void OfflinePageMetadataStoreSQL::Reset(const ResetCallback& callback) {
+  background_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&OfflinePageMetadataStoreSQL::ResetSync,
+                            weak_ptr_factory_.GetWeakPtr(),
+                            base::ThreadTaskRunnerHandle::Get(), callback));
+}
+
+void OfflinePageMetadataStoreSQL::SetInMemoryDatabaseForTesting(
+    bool in_memory) {
+  // Must be called prior to Load(...)
+  DCHECK(db_.get() == NULL);
+
+  use_in_memory_ = in_memory;
+}
+
+}  // namespace offline_pages