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

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

nit: sentence case.

[bburns, 2016/03/30 16:54:40]

Done.

+// I can use it inline in other SQL statements below

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

so it can be used.

[bburns, 2016/03/30 16:54:40]

Done.

+#define kOfflinePagesTableName "offlinepages"
+const char kOfflinePagesColumns[] =
+    "(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)";
+
+// 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 {
+  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
+  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 DeleteByOfflineId(sql::Connection* db, int64_t offline_id) {
+  const char sql[] =
+      "DELETE FROM " kOfflinePagesTableName " WHERE offline_id=?";
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, sql));
+  statement.BindInt64(0, offline_id);
+  return db->Execute(sql);
+}
+
+// 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));
+  base::FilePath path(statement->ColumnString(OP_FILE_PATH));

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

From the level db store impl:
#if defined(OS_POSIX)
  path_string = item.file_path.value();
#elif defined(OS_WIN)
  path_string = base::WideToUTF8(item.file_path.value());
#endif

[bburns, 2016/03/30 16:54:39]

Is this really required?  It seems like something due to weirdness in Proto not
anything inherent.

[fgorski, 2016/03/30 17:19:31]

It's actually file path that is weird.

class BASE_EXPORT FilePath {
 public:
#if defined(OS_POSIX)
  // On most platforms, native pathnames are char arrays, and the encoding
  // may or may not be specified.  On Mac OS X, native pathnames are encoded
  // in UTF-8.
  typedef std::string StringType;
#elif defined(OS_WIN)
  // On Windows, for Unicode-aware applications, native pathnames are wchar_t
  // arrays encoded in UTF-16.
  typedef std::wstring StringType;
#endif  // OS_WIN

+  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);
+  return item;
+}
+
+bool InsertOrReplace(sql::Connection* db, const OfflinePageItem& item) {
+  const char sql[] =

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

since it is a const, kSql

[bburns, 2016/03/30 16:54:40]

Done.

+      "INSERT OR REPLACE INTO " kOfflinePagesTableName
+      " (offline_id, online_url, client_namespace, client_id, file_path, "
+      "file_size, creation_time, last_access_time, version)"
+      " VALUES "
+      " (?, ?, ?, ?, ?, ?, ?, ?, ?)";
+
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, sql));
+  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);
+  statement.BindString(4, item.file_path.value());
+  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);
+  return statement.Run();
+}
+}  // anonymous namespace
+
+OfflinePageMetadataStoreSQL::OfflinePageMetadataStoreSQL(
+    scoped_refptr<base::SequencedTaskRunner> background_task_runner,
+    const base::FilePath& path,
+    bool in_memory)
+    : background_task_runner_(background_task_runner),
+      db_file_path_(path),
+      use_in_memory_(in_memory),
+      weak_ptr_factory_(this) {}
+
+OfflinePageMetadataStoreSQL::~OfflinePageMetadataStoreSQL() {}
+
+void OfflinePageMetadataStoreSQL::LoadSync(
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    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 if (!base::CreateDirectory(db_file_path_.DirName())) {

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

since you are logging here, probably makes sense to call
CreateDirectoryAndGetError and log the error.

[bburns, 2016/03/30 16:54:40]

Done.

+    LOG(ERROR) << "Failed to create offline pages db directory.";
+  } else {
+    opened = db_->Open(db_file_path_);
+    if (opened)
+      db_->Preload();
+  }
+  if (!opened) {
+    NotifyLoadResult(callback, runner, STORE_INIT_FAILED,
+                     std::vector<OfflinePageItem>());
+  }
+
+  if (!sql::MetaTable::DoesTableExist(db_.get())) {
+    if (!CreateSchema(meta_table_.get(), db_.get())) {
+      NotifyLoadResult(callback, runner, STORE_INIT_FAILED,
+                       std::vector<OfflinePageItem>());

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

do you intend to stop execution here?
if so return.

Also consider logging appropriate message.

This might also be why the tests are failing, in case you are calling Init twice
on meta_table (and hit DCHECK)

[bburns, 2016/03/30 16:54:40]

Done.

+    }
+  }
+
+  if (!meta_table_->Init(db_.get(), kCurrentVersion, kCompatibleVersion))
+    NotifyLoadResult(callback, runner, STORE_INIT_FAILED,

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

this is a single line, but a multiline body, surround with {}.

Also return if it is appropriate and log

[bburns, 2016/03/30 16:54:40]

Done.

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

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

return missing

[bburns, 2016/03/30 16:54:40]

Done.

+  }
+
+  const char sql[] = "SELECT * FROM " kOfflinePagesTableName;
+
+  sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE, sql));
+
+  std::vector<OfflinePageItem> result;
+  while (statement.Step()) {
+    result.push_back(MakeOfflinePageItem(&statement));
+  }
+
+  if (!statement.Succeeded()) {
+    NotifyLoadResult(callback, runner, STORE_LOAD_FAILED,
+                     std::vector<OfflinePageItem>());
+  } else {
+    NotifyLoadResult(callback, runner, LOAD_SUCCEEDED, result);
+  }
+}
+
+void OfflinePageMetadataStoreSQL::AddOrUpdateOfflinePageSync(
+    const OfflinePageItem& offline_page,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const UpdateCallback& callback) {
+  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) {
+  // 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());
+
+  for (auto offline_id : offline_ids) {
+    if (!DeleteByOfflineId(db_.get(), offline_id)) {
+      runner->PostTask(FROM_HERE, base::Bind(callback, false));
+      return;
+    }
+  }
+
+  if (!transaction.Commit()) {

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

This posts the result twice upon failure, because you didn't return. Also, I
don't like the if here. how about:

bool result = transaction.Commit();  // can be bool success = 
runner->PostTask(FROM_HERE, base::Bind(callback, result));

[bburns, 2016/03/30 16:54:40]

Done.

+    runner->PostTask(FROM_HERE, base::Bind(callback, false));
+  }
+  runner->PostTask(FROM_HERE, base::Bind(callback, true));
+}
+
+void OfflinePageMetadataStoreSQL::Reset(const ResetCallback& callback) {
+  bool success = DropTable(db_.get(), kOfflinePagesTableName);

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

shouldn't DDL be called on the background thread?

Also I don't see a call to Reset in tests, so perhaps there is a chance to
improve there.

[bburns, 2016/03/30 16:54:39]

If you want.  Drop table seemed like it was so fast that there was little reason
to background it.  Your call.

[fgorski, 2016/03/30 17:19:31]

Let's stay consistent with choice of threads. If you put all activity related to
DB on background thread, you will never try to drop table at the same time your
writing to it, so no additional synchronization problems.

[Scott Hess - ex-Googler, 2016/04/12 21:57:23]

While SQLite _can_ be thread-safe in certain ways, there are no thread-safety
guarantees explicitly made by sql/ or third_party/sqlite/ .  Things which work
now may stop working later unexpectedly.

Also, just because it's fast doesn't mean there won't be cases where it's slow. 
This is a transaction, which means an fsync, which means that if it happens on a
system compiling Chrome (or something), it will be slow.

+  callback.Run(success);
+}
+
+void OfflinePageMetadataStoreSQL::NotifyLoadResult(
+    const LoadCallback& callback,

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

I don't mean to argue either order is better:
callback, runner,
or
runner, callback,

but please use it consistently in the code.

[bburns, 2016/03/30 16:54:39]

Done.

+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    LoadStatus status,
+    const std::vector<OfflinePageItem>& result) {
+  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();
+  }
+  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.

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

Good shortcut, but for safety you can post it to the thread, so execution always
yields here.
We've had code in the past that looped itself on the call stack retrying. I
don't expect this problem here, but it also makes things easier to test.

[bburns, 2016/03/30 16:54:40]

Done.

[fgorski, 2016/03/30 17:19:31]

I meant something like:
base::ThreadTaskRunnerHandle::Get()->Post()

as you will not have to involve the background thread at all, but will break
complete this call, before the async return starts.

+    callback.Run(true);
+    return;
+  }
+
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&OfflinePageMetadataStoreSQL::RemoveOfflinePagesSync,
+                 weak_ptr_factory_.GetWeakPtr(), offline_ids,
+                 base::ThreadTaskRunnerHandle::Get(), callback));
+}
+
+}  // namespace offline_pages