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/error_delegate_util.h"

[Scott Hess - ex-Googler, 2016/05/02 20:36:18]

Don't think you're using this anywhere.

[bburns, 2016/05/02 21:44:18]

Done.

+#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,"
+    " user_initiated INTEGER,"  // this is actually a boolean

[Scott Hess - ex-Googler, 2016/05/02 20:36:18]

Will this be used later?  If the boolean-ness is really considered subtle,
suggest you enumerate the values you're using for true and false (I don't think
it's subtle).  Also, since you aren't setting it right now, it's going to be
NULL in the database.

Your INSERT OR REPLACE will also leave [status] NULL, and [offline_url]. 
ColumnInt() will transform NULL to 0, and ColumnString() will transform NULL to
"", so it's probably safe for this code as-is, but if you ever do SQL queries
against those columns, NULL may not be what you want (for instance, "WHERE
user_initiated<>1" will succeed for values 0 and NULL, but if you say "WHERE
user_initiated=0" it will not succeed for NULL).

[bburns, 2016/05/02 21:44:18]

Thanks for the explanation. We do anticipate using this in the future.  For now
I added a note capturing your comment.

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

Do you intend the same for [status] and [offline_url]?  Mostly I ask because
this is the kind of thing that in my experience leads to weirdo edge cases. 
Nobody writes their queries expecting NULL.  But in the end I think this is a
style issue.

[bburns, 2016/05/03 17:15:55]

Done.

+    " 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)) {

[Scott Hess - ex-Googler, 2016/05/02 20:36:18]

Either provide optional {}, or don't, but don't be inconsistent within a file.

I prefer to always show the {}, but I've gotten enough pushback in reviews that
I wouldn't recommend it.  AFAICT the safest route is no {} unless either the
body is more than one line or the if() is more than one line.

[bburns, 2016/05/02 21:44:18]

Ah, you're running up against my personal preferences vs. the team's
preferences, its subconcious on my part.  fixed.

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

:-).  My theory has always been to just have the {} because that way I don't
have to ever think about whether the code is right or wrong.  And I find it
makes code appropriately less dense.  But much/most of Chromium disagrees, so
... *shrug*.

[bburns, 2016/05/03 17:15:55]

Acknowledged.

+    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)));
+#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 20:36:18]

Man, I hate this pattern.  But I have nothing for you WRT resolving it (maybe it
comes up enough to warrant Path versions in sql::Statement).  But I would put
#else with an #error to protect against typos in your OS_ references.

[bburns, 2016/05/02 21:44:18]

Done.

+#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,
+                  bool use_in_memory) {
+  db->set_page_size(4096);
+  db->set_cache_size(500);
+  db->set_histogram_tag("OfflinePageMetadata");

[Scott Hess - ex-Googler, 2016/05/02 20:36:18]

Missing histograms.xml change.

If you're worried about Yet Another OWNER syndrome, I've found the histograms
owners to be very responsive.

[bburns, 2016/05/02 21:44:17]

Done.

+  db->set_exclusive_locking();
+
+  if (use_in_memory) {
+    if (!db->OpenInMemory()) {
+      return false;
+    }

[Scott Hess - ex-Googler, 2016/05/02 20:36:18]

Another case to change if you decide to omit optional {}.

[bburns, 2016/05/02 21:44:18]

Done.

+  } else {
+    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();
+  }
+
+  CreateSchema(db);

[Scott Hess - ex-Googler, 2016/05/02 20:36:18]

Why have CreateSchema() return a flag that you're just burying the result?

I'm willing to be relaxed about actually implementing error-handling at this
point, but in that case please log a tracking bug and a TODO referencing the
tracking bug (probably here would be reasonable).

[bburns, 2016/05/02 21:44:18]

This was an oversight.  fixed.

+  return true;
+}
+
+}  // anonymous namespace
+
+OfflinePageMetadataStoreSQL::OfflinePageMetadataStoreSQL(
+    scoped_refptr<base::SequencedTaskRunner> background_task_runner,
+    const base::FilePath& path)
+    : background_task_runner_(background_task_runner),

[Scott Hess - ex-Googler, 2016/05/02 20:36:18]

Maybe std::move().  For most structures I wouldn't care, but scoped_refptr
implies a lock somewhere, or some other sort of delicate code.

[bburns, 2016/05/02 21:44:18]

Done.

+      db_file_path_(path.AppendASCII("OfflinePages.db")),
+      use_in_memory_(false) {}
+
+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,
+    bool in_memory,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const LoadCallback& callback) {
+  if (!InitDatabase(db, path, in_memory)) {
+    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) {
+  DCHECK(db);

[Scott Hess - ex-Googler, 2016/05/02 20:36:18]

This DCHECK is probably redundant.  If you feel that it is important for
purposes of providing a sensible heads-up, then I'd put it in
AddOrUpdateOfflinePage() before posting the task in the first place.

[bburns, 2016/05/02 21:44:17]

Done.  (this DCHECK was requested by a different reviewer...)

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

I'm fine with the DCHECK (others might not be), mostly it just seems like here
it will require searching for the caller if you ever see it.

[bburns, 2016/05/03 17:15:55]

Acknowledged.

+  // 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) {
+  DCHECK(db);

[Scott Hess - ex-Googler, 2016/05/02 20:36:18]

Likewise here, I think.

[bburns, 2016/05/02 21:44:18]

Done.

+  // 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,

[Scott Hess - ex-Googler, 2016/05/02 20:36:18]

This is taking ownership.  If the delete fails, the caller can't do anything
about it, as they already released their reference.  Probably best to just make
it a unique_ptr<> and drop the explicit delete.

[Then you might be able to just pass the statement.Run() directly to
base::Bind().]

[bburns, 2016/05/02 21:44:18]

It turns out unique_ptr and base::Bind don't get along (at least I couldn't
convince them to) I think this is because it attempts to naively copy the struct
without using std::move and so unique_ptr barfs because there is no copy
constructor.

I did clean up the code somewhat.

I'm open to greater C++-fu than mine...

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

It's a little funky, you use base::Passed(&db_) to base::Bind().  See
internal::PassedWrapper in base/bind_helpers.h .  Here's a Google Groups thread
which references this:
  
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/roY78iTblYc

Personally, I find base::Passed(std::move(db_)) easier to parse.  std::move() to
say "STL, shift the reference" and base::Passed() to say "base::Bind(), this
isn't an accident".  But I don't see that case being used, whereas I do see the
& usage (for instance BindTest.BindMoveOnlyVector in bind_unittest.cc).

Just to be clear, I'm pushing the move semantics here even though it adds some
clunkiness in a bid to reduce the chance for leaks deleting the db.  Having
ownership shift via base::Bind() will delete the database automatically for this
function, and will also delete the database in some cases where the task is
entirely cancelled (and other cases, like the thread is already terminated,
already must allow for leaks). 

[bburns, 2016/05/03 17:15:55]

Done.

+    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));
+  if (!statement.Run()) {
+    runner->PostTask(FROM_HERE, base::Bind(callback, false));
+    return;
+  }
+  delete db;
+
+  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;
+  }
+  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_, use_in_memory_,
+                            base::ThreadTaskRunnerHandle::Get(), callback));
+}
+
+void OfflinePageMetadataStoreSQL::AddOrUpdateOfflinePage(
+    const OfflinePageItem& offline_page,
+    const UpdateCallback& callback) {
+  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) {
+  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));
+}
+
+void OfflinePageMetadataStoreSQL::SetInMemoryDatabaseForTesting(
+    bool in_memory) {
+  // Must be called prior to Load(...)
+  DCHECK(db_.get() == nullptr);
+
+  use_in_memory_ = in_memory;
+}
+
+}  // namespace offline_pages