[Offline pages] Adding persistent request queue based on SQLite

components/offline_pages/background/request_queue_store_sql.cc

Index: components/offline_pages/background/request_queue_store_sql.cc
diff --git a/components/offline_pages/background/request_queue_store_sql.cc b/components/offline_pages/background/request_queue_store_sql.cc
new file mode 100644
index 0000000000000000000000000000000000000000..628ed2d3904ee558f90dc01aac514aa8c3ce16c6
--- /dev/null
+++ b/components/offline_pages/background/request_queue_store_sql.cc
@@ -0,0 +1,437 @@
+// 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/background/request_queue_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/sequenced_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "components/offline_pages/background/save_page_request.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 REQUEST_QUEUE_TABLE_NAME "request_queue_v1"
+
+// New columns should be added at the end of the list in order to avoid
+// complicated table upgrade.
+const char kOfflinePagesColumns[] =
+    "(request_id INTEGER PRIMARY KEY NOT NULL,"
+    " creation_time INTEGER NOT NULL,"
+    " activation_time INTEGER NOT NULL DEFAULT 0,"
+    " last_attempt_time INTEGER NOT NULL DEFAULT 0,"
+    " attempt_count INTEGER NOT NULL,"
+    " url VARCHAR NOT NULL,"
+    " client_namespace VARCHAR NOT NULL,"
+    " client_id VARCHAR NOT NULL"
+    ")";
+
+// Defines indices of the columns in a SELECT * FROM query. Should be kept in
+// sync with above.
+enum : int {
+  RQ_REQUEST_ID,
+  RQ_CREATION_TIME,
+  RQ_ACTIVATION_TIME,
+  RQ_LAST_ATTMEPT_TIME,
+  RQ_ATTEMPT_COUNT,
+  RQ_URL,
+  RQ_CLIENT_NAMESPACE,
+  RQ_CLIENT_ID,
+};
+
+enum class RequestExistsResult {
+  SQL_FAILED,
+  REQUEST_EXISTS,
+  REQUEST_DOES_NOT_EXIST,
+};
+
+// This is cloned from //content/browser/appcache/appcache_database.cc
+struct TableInfo {
+  const char* table_name;
+  const char* columns;
+};
+
+const TableInfo kRequestQueueTable{REQUEST_QUEUE_TABLE_NAME,
+                                   kOfflinePagesColumns};
+
+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());

[Scott Hess - ex-Googler, 2016/06/20 19:38:32]

I think I mentioned that this approach is weird in a past CL.  I don't have the
energy to figure out how that played out - but I'll just repeat myself, this
approach is weird.

[fgorski, 2016/06/20 23:42:59]

Hey, if there is a suggestion how to better write this code I'll gladly look
into it, but I didn't find your comment in the previous patch. (looked at like
10 different uploads). Please provide some hints.

+}
+
+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(kRequestQueueTable.table_name)) {
+    if (!CreateTable(db, kRequestQueueTable))

[Scott Hess - ex-Googler, 2016/06/20 19:38:33]

"CREATE TABLE IF NOT EXISTS " should work the same, here.

Are you planning to re-use CreateTable() somewhere else?  Right now you have
CreateSchema() which only appears to exist to wrap CreateTable() in a
transaction.  It would seem like they could just be merged.

[Of course, "CREATE TABLE IF NOT EXISTS " means you wouldn't need a transaction
in the first place until you add indices.]

[fgorski, 2016/06/20 23:43:00]

Done. I am keeping the CreateTable method separate, as we want to deal with
upgrade/indices here.

[Scott Hess - ex-Googler, 2016/06/22 18:31:45]

Since you don't have a MetaTable instance or something else tracking version
information, do you have a plan for upgrades and indices?

I ask because my experience is that if you haven't already done a dry run for a
schema upgrade, then sometimes you will find surprising things when you need
one.  Maybe not in your case, it's just that most of the nasty issues tend to be
schema changes due to the combinatorics of the problem.

Hmm, and a comment for schema upgrades: I've found that our general system of
MetaTable-version-tracking has holes where the code to optimistically create the
current version can interact poorly with the ALTER statements.  I think it's
reasonable to just have schema changes accumulate, so even new users should
start with the original schema and upgrade to current.  This simplifies the set
of paths which can go wrong getting to the final schema, and also makes clear to
readers that those intermediate schema may exist in the wild.

[fgorski, 2016/06/22 23:32:16]

The way we think of doing schema changes is pretty simple:
* Detect that a field is missing/extra
* Rename the old table. (Or move data to tmp)
* Create a new table with proper schema
* Copy data with proper defaulting

We already have one successful upgrade in the old code that you were reviewing.

This works because:
a) We don't expect to have a lot of data (hundreds of records top)
b) We don't have relations between tables so no need to do stuff in place.
c) Indexes should be simple to recreate (once we have them).

Let me know if you see concerns with that.

+      return false;
+  }
+
+  // TODO(fgorski): Add indices here.
+  return transaction.Commit();
+}
+
+bool DeleteByRequestId(sql::Connection* db, int64_t request_id) {
+  static const char kSql[] =

[Scott Hess - ex-Googler, 2016/06/20 19:38:32]

I don't think you need static here.  The literal string is already a static in
the TEXT section.

[fgorski, 2016/06/20 23:43:00]

Done.

+      "DELETE FROM " REQUEST_QUEUE_TABLE_NAME " WHERE request_id=?";
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+  statement.BindInt64(0, request_id);
+  LOG(ERROR) << "In DeleteRequestById: "
+             << request_id;  // statement.GetSQLStatement();

[Scott Hess - ex-Googler, 2016/06/20 19:38:32]

Drop the logging, or make it VLOG.

Or just drop it.  My experience is that logging in production code exists solely
to confuse build sheriffs.

[fgorski, 2016/06/20 23:42:59]

Done. This code is a leftover from debugging.

+  return statement.Run();
+}
+
+// Create a save page request from a SQL result.  Expects complete rows with
+// all columns present.
+SavePageRequest MakeSavePageRequest(sql::Statement* statement) {

[Scott Hess - ex-Googler, 2016/06/20 19:38:32]

Will const sql::Statement& work, here?  My notion is to make it clear that this
is read-only WRT statement.  AFAICT all of the Column*() methods are const, so
it should work.

[fgorski, 2016/06/20 23:43:00]

Done.

+  int64_t id = statement->ColumnInt64(RQ_REQUEST_ID);
+  GURL url(statement->ColumnString(RQ_URL));

[Scott Hess - ex-Googler, 2016/06/20 19:38:33]

Maybe const all of these.  consting the GURL, ClientId and base::Time values
makes sense as a compiler hint, and once there it makes sense to just const
everything to be consistent.

[fgorski, 2016/06/20 23:42:59]

Done.

+  ClientId client_id(statement->ColumnString(RQ_CLIENT_NAMESPACE),
+                     statement->ColumnString(RQ_CLIENT_ID));
+  int64_t attempt_count = statement->ColumnInt64(RQ_ATTEMPT_COUNT);
+  base::Time creation_time =
+      base::Time::FromInternalValue(statement->ColumnInt64(RQ_CREATION_TIME));
+  base::Time activation_time =
+      base::Time::FromInternalValue(statement->ColumnInt64(RQ_ACTIVATION_TIME));
+
+  SavePageRequest request(id, url, client_id, creation_time, activation_time);
+  base::Time last_attempt_time = base::Time::FromInternalValue(
+      statement->ColumnInt64(RQ_LAST_ATTMEPT_TIME));

[Scott Hess - ex-Googler, 2016/06/20 19:38:32]

Either push last_attempt_time up with the other getters (so that all are in one
place), or pull attempt_count below (so that the variables are initialized just
before use.

Format last_attempt_time consistently with creation_time and activation_time.

That said, last_attempt_time and attempt_count seem pretty obvious from context,
so having temporaries to store them may not add much, since the enum also
describes what you're looking at.

[fgorski, 2016/06/20 23:43:00]

Unfortunately that would not work, because they are different length... and git
cl format does its own job.
Done. Went with this approach.

+  request.set_last_attempt_time(last_attempt_time);
+  request.set_attempt_count(attempt_count);
+
+  return request;
+}
+
+RequestExistsResult RequestExists(sql::Connection* db, int64_t request_id) {
+  const char kSql[] =
+      "SELECT COUNT(*) FROM " REQUEST_QUEUE_TABLE_NAME " WHERE request_id = ?";
+  sql::Statement statement(db->GetUniqueStatement(kSql));
+  statement.BindInt64(0, request_id);
+  if (!statement.Step()) {
+    LOG(ERROR) << "Failed to check if request exists: " << request_id
+               << ", SQL statement: " << statement.GetSQLStatement();

[Scott Hess - ex-Googler, 2016/06/20 19:38:32]

Again, suggest dropping the log line.  Note that if the SQL fails because of a
syntax error of database corruption, there will already be logging.  I believe
that for COUNT(*), those are the only failures that could happen (there should
_always_ be a single result, otherwise).

[fgorski, 2016/06/20 23:43:00]

Done. Again debugging -> I had hard time with getting the Count(*) query to work
and only after a while figured out that it should be using GetUniqueStatement
with Step as opposed to Run...

[Scott Hess - ex-Googler, 2016/06/22 18:31:45]

OK.  I've often thought it would be nice to have a "Get a single result" helper,
but I've always cut bait because I don't want 6 of them to cover the different
types.

[fgorski, 2016/06/22 23:32:16]

Acknowledged. It was just not clear to me how to query for such thing.
I still don't know if it would work if I used GetCachedStatement instead of
Unique.

[Scott Hess - ex-Googler, 2016/06/22 23:46:05]

I would err on the side of unique over cached.  The concept is that you can save
the overhead of re-preparing your queries.  I suspect that most Chromium queries
are against simple schema so the savings is not great, and seldom-enough
executed that the memory-for-speed tradeoff is pretty ambiguous.

+    return RequestExistsResult::SQL_FAILED;
+  }
+  return statement.ColumnInt64(0) ? RequestExistsResult::REQUEST_EXISTS
+                                  : RequestExistsResult::REQUEST_DOES_NOT_EXIST;
+}
+
+RequestQueueStore::UpdateStatus InsertOrReplace(
+    sql::Connection* db,
+    const SavePageRequest& request) {
+  // In order to use the enums in the Bind* methods, keep the order of fields
+  // the same as in the definition/select query.

[Scott Hess - ex-Googler, 2016/06/20 19:38:32]

Did I mention this is weird?  It's like you're building an object-relational
model based entirely on comments.

[fgorski, 2016/06/20 23:43:00]

OK. I'll gladly remove the comment. Is there a suggestion I should apply to the
code?
Should I simply use numbers instead of enums?

+  const char kInsertSql[] =
+      "INSERT OR REPLACE INTO " REQUEST_QUEUE_TABLE_NAME
+      " (request_id, creation_time, activation_time, last_attempt_time, "
+      " attempt_count, url, client_namespace, client_id) "
+      " VALUES "
+      " (?, ?, ?, ?, ?, ?, ?, ?)";
+
+  // Checking if a request exists and adding/updating it will happen in a single
+  // transaction.
+  sql::Transaction transaction(db);
+  if (!transaction.Begin()) {
+    LOG(ERROR) << "Failed to start transaction on InsertOrReplace.";

[Scott Hess - ex-Googler, 2016/06/20 19:38:33]

VLOG or drop the logging.  I think the only way transaction creation will fail
will be things outside of your control (corruption, etc).

I'm not going to comment the rest of the logging, you get the point.

[fgorski, 2016/06/20 23:43:00]

Done.

+    return RequestQueueStore::UpdateStatus::FAILED;
+  }
+
+  RequestExistsResult exists = RequestExists(db, request.request_id());
+  if (exists == RequestExistsResult::SQL_FAILED) {
+    LOG(ERROR) << "Failed to check if request exists: " << request.request_id();
+    return RequestQueueStore::UpdateStatus::FAILED;
+  }
+
+  RequestQueueStore::UpdateStatus status =
+      exists == RequestExistsResult::REQUEST_EXISTS
+          ? RequestQueueStore::UpdateStatus::UPDATED
+          : RequestQueueStore::UpdateStatus::ADDED;

[Scott Hess - ex-Googler, 2016/06/20 19:38:33]

Is tracking whether this was added or replaced really necessary?  I mean like
someone is going to make decisions based on it necessary?  You're paying the
price of an explicit transaction to accomplish this, if you're doing it just in
case someone might use it someday, I'd say just skip the complication and do
only what you actually need now.

[fgorski, 2016/06/20 23:43:00]

Yeah, I don't like it, but that was requested by another developer. I can try
persuasion... (As explained earlier, I figured out that count(*) works
differently, so at least I got that going for me...)

[Scott Hess - ex-Googler, 2016/06/22 18:31:45]

If someone is going to use it, fine.  My main concern is that having the
distinction now maybe means someday someone uses it in an incidental fashion,
and later you find out that you're writing annoying workaround code to support a
use case that nobody ever really intended to support.

[fgorski, 2016/06/22 23:32:16]

We discussed it internally and decided against it at the store level. We'll have
a higher order function that will be used to work that out before calling
insertorupdate.

+
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kInsertSql));
+  statement.BindInt64(RQ_REQUEST_ID, request.request_id());
+  statement.BindString(RQ_URL, request.url().spec());
+  statement.BindString(RQ_CLIENT_NAMESPACE, request.client_id().name_space);
+  statement.BindString(RQ_CLIENT_ID, request.client_id().id);
+  statement.BindInt64(RQ_CREATION_TIME,
+                      request.creation_time().ToInternalValue());
+  statement.BindInt64(RQ_ACTIVATION_TIME,
+                      request.activation_time().ToInternalValue());
+  statement.BindInt64(RQ_LAST_ATTMEPT_TIME,
+                      request.last_attempt_time().ToInternalValue());
+  statement.BindInt64(RQ_ATTEMPT_COUNT, request.attempt_count());
+
+  if (!statement.Run() || !transaction.Commit()) {
+    LOG(ERROR) << "Failed to add/update a request: " << request.request_id();
+    return RequestQueueStore::UpdateStatus::FAILED;
+  }
+
+  return status;
+}
+
+bool InitDatabase(sql::Connection* db, const base::FilePath& path) {
+  db->set_page_size(4096);
+  db->set_cache_size(500);
+  db->set_histogram_tag("BackgroundRequestQueue");
+  db->set_exclusive_locking();
+
+  base::File::Error err;
+  if (!base::CreateDirectoryAndGetError(path.DirName(), &err)) {

[Scott Hess - ex-Googler, 2016/06/20 19:38:33]

Does this need an entire directory for two files (db and journal)?  Is there
going to be other stuff in here?  If there's going to be other stuff in here,
should directory creation be a side effect of opening the database, or should
the client be making those arrangements?

[fgorski, 2016/06/20 23:43:00]

We like things separated and don't mind having a directory here.
Perhaps we'll use it in future to put more stuff in, but for now it is just
these 2 files.

(It makes it easy to blow away the store when debugging/testing)

[Scott Hess - ex-Googler, 2016/06/22 18:31:45]

Acknowledged.

+    LOG(ERROR) << "Failed to create background request queue 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
+
+RequestQueueStoreSQL::RequestQueueStoreSQL(
+    scoped_refptr<base::SequencedTaskRunner> background_task_runner,
+    const base::FilePath& path)
+    : background_task_runner_(std::move(background_task_runner)),
+      db_file_path_(path.AppendASCII("RequestQueue.db")),
+      weak_ptr_factory_(this) {
+  OpenConnection();
+}
+
+RequestQueueStoreSQL::~RequestQueueStoreSQL() {
+  if (db_.get() &&
+      !background_task_runner_->DeleteSoon(FROM_HERE, db_.release())) {
+    DLOG(WARNING) << "SQL database will not be deleted.";
+  }
+}
+
+// static
+void RequestQueueStoreSQL::OpenConnectionSync(
+    sql::Connection* db,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const base::FilePath& path,
+    const base::Callback<void(bool)>& callback) {
+  bool success = InitDatabase(db, path);
+  runner->PostTask(FROM_HERE, base::Bind(callback, success));
+}
+
+// static
+void RequestQueueStoreSQL::GetRequestsSync(
+    sql::Connection* db,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const GetRequestsCallback& callback) {
+  const char kSql[] = "SELECT * FROM " REQUEST_QUEUE_TABLE_NAME;
+
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+
+  std::vector<SavePageRequest> result;
+  while (statement.Step()) {
+    result.push_back(MakeSavePageRequest(&statement));
+  }
+
+  runner->PostTask(FROM_HERE,
+                   base::Bind(callback, statement.Succeeded(), result));
+}
+
+// static
+void RequestQueueStoreSQL::AddOrUpdateRequestSync(
+    sql::Connection* db,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const SavePageRequest& request,
+    const UpdateCallback& callback) {
+  // TODO(fgorski): add UMA metrics here.
+  RequestQueueStore::UpdateStatus status = InsertOrReplace(db, request);
+  runner->PostTask(FROM_HERE, base::Bind(callback, status));
+}
+
+// static
+void RequestQueueStoreSQL::RemoveRequestsSync(
+    sql::Connection* db,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const std::vector<int64_t>& request_ids,
+    const RemoveCallback& callback) {
+  // TODO(fgorski): add UMA metrics here.
+
+  // 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, 0));
+    return;

[Scott Hess - ex-Googler, 2016/06/20 19:38:33]

This boilerplate seems brittle.  Would it make sense to have an inner function
with bool return and pointer to count, and a wrapper to convert that to the
callback?

[fgorski, 2016/06/20 23:42:59]

Done.

+  }
+  int count = 0;

[Scott Hess - ex-Googler, 2016/06/20 19:38:33]

I usually expect countable things to be size_t.

[fgorski, 2016/06/20 23:42:59]

I played with the idea, however:
a) GetLastChangeCount returns int
b) our interface is currently set up to expect int in callback.

Are you OK with this code sticking to int, at least for now?

[Scott Hess - ex-Googler, 2016/06/22 18:31:45]

OK.  I think the generic ruling is something like "use int or size_t".  Mostly
I'm just used to STL containers using size_t, and this is iterating an STL
container.

[fgorski, 2016/06/22 23:32:16]

OK. Until we change the interface to be size_t I want to keep it as int. I can
bring it to other developer's attention, but for now this will work for us.

+  for (auto request_id : request_ids) {
+    RequestExistsResult exists = RequestExists(db, request_id);
+    if (exists == RequestExistsResult::SQL_FAILED) {
+      runner->PostTask(FROM_HERE, base::Bind(callback, false, 0));
+      return;
+    }
+    if (exists == RequestExistsResult::REQUEST_DOES_NOT_EXIST)
+      continue;
+
+    ++count;
+    if (!DeleteByRequestId(db, request_id)) {
+      runner->PostTask(FROM_HERE, base::Bind(callback, false, 0));
+      return;
+    }

[Scott Hess - ex-Googler, 2016/06/20 19:38:33]

The contents of this loop seem overly complicated.  You could just DELETE and
test GetLastChangeCount() to see how many rows were affected.

[fgorski, 2016/06/20 23:43:00]

Done. Brilliant idea and it is on the connection not statement, so I can keep
DeletebyRequestId the way it is.

+  }
+
+  if (!transaction.Commit()) {
+    runner->PostTask(FROM_HERE, base::Bind(callback, false, 0));
+    return;
+  }
+
+  runner->PostTask(FROM_HERE, base::Bind(callback, true, count));
+}
+
+// static
+void RequestQueueStoreSQL::ResetSync(
+    std::unique_ptr<sql::Connection> db,
+    const base::FilePath& db_file_path,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const ResetCallback& callback) {
+  // This method deletes the content of the whole store. It should be used with
+  // caution, e.g. when recovering from situation, in which the contents of the

[Scott Hess - ex-Googler, 2016/06/20 19:38:32]

"when recovering from situation" doesn't really parse for me.

[fgorski, 2016/06/20 23:42:59]

I am not sure what exactly you didn't parse, but I removed it.

+  // store does not make sense and cannot be converted to reasonable requests.
+  db->Close();
+  bool success = base::DeleteFile(db_file_path, /* recursive */ false);

[Scott Hess - ex-Googler, 2016/06/20 19:38:33]

Don't use DeleteFile().  Use sql::Connection::Delete().

That said, better would probably be
  db->Raze();    // Clear all data from database.
  db->Close();

If you want to also delete after, that's fine :-).

Note that there have in the past been cases where delete returning success
didn't actually mean delete worked.  Many of those cases have been fixed, but I
think anything which really depends on the return code from delete is probably
broken or going to be broken.

Also, AFAICT
Reset->ResetSync->OnResetDone->OpenConnection->OpenConnectionSync->InitDatabase->OpenConnectionDone,
which means multiple thread hops.  Is there any particular reason not to just
have ResetSync call InitDatabase and send that success to OnResetDone?

[fgorski, 2016/06/20 23:43:00]

Done.
OK skipped deleting the file and went with Raze()
Done. Had to implement it twice. Got to:
Reset---(hop to background)-->ResetSync-->InitDatabase---(hop to
foreground)-->OnResetDone ---> OpenConnectionDone (called to clean up if we
failed).

[Scott Hess - ex-Googler, 2016/06/22 18:31:44]

Given the other point about this class managing a directory containing a
database, rather than a database, I guess you maybe end up with a distinction
between "Clear the data from the database" and "Delete the database".  Raze() is
aiming for the first of those, if you later decide to go the delete route, or
add a delete function, IMHO delete the directory as a whole, not the files
within it.

[That said, delete is hairy on Windows.  You can have problems with virus
scanners and other malware holding handles to files and preventing the delete
from working.]

[fgorski, 2016/06/22 23:32:16]

The only thing I would be worried about is when the file is corrupted and I
cannot open it. In that case I'd like to blow away the whole folder and start
over. Otherwise (and for now) deleting the content of DB works.

+  runner->PostTask(FROM_HERE, base::Bind(callback, success));
+}
+
+void RequestQueueStoreSQL::GetRequests(const GetRequestsCallback& callback) {
+  DCHECK(db_.get());
+  if (!db_.get()) {
+    // 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, false, std::vector<SavePageRequest>()));
+    return;
+  }
+
+  background_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&RequestQueueStoreSQL::GetRequestsSync, db_.get(),
+                            base::ThreadTaskRunnerHandle::Get(), callback));
+}
+
+void RequestQueueStoreSQL::AddOrUpdateRequest(const SavePageRequest& request,
+                                              const UpdateCallback& callback) {
+  DCHECK(db_.get());
+  if (!db_.get()) {
+    // 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, UpdateStatus::FAILED));
+    return;
+  }
+
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&RequestQueueStoreSQL::AddOrUpdateRequestSync, db_.get(),
+                 base::ThreadTaskRunnerHandle::Get(), request, callback));
+}
+
+void RequestQueueStoreSQL::RemoveRequests(
+    const std::vector<int64_t>& request_ids,
+    const RemoveCallback& callback) {
+  DCHECK(db_.get());
+  if (!db_.get()) {
+    // 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, false, 0));
+    return;
+  }
+
+  if (request_ids.empty()) {
+    // Nothing to do, but post a callback instead of calling directly
+    // to preserve the async style behavior to prevent bugs.

[Scott Hess - ex-Googler, 2016/06/20 19:38:32]

Should there be a DCHECK for this case?

Also, IMHO this case isn't the same as the !db_.get() case.  RemoveRequestsSync
cannot operate with a NULL db handle.  But it will work just fine with an empty
request vector, returning the exact result you're returning below.  So I don't
think it's worthwhile to short-circuit this case.

[fgorski, 2016/06/20 23:43:00]

No that is OK.
Done.

+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(callback, true, 0));
+    return;
+  }
+
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&RequestQueueStoreSQL::RemoveRequestsSync, db_.get(),
+                 base::ThreadTaskRunnerHandle::Get(), request_ids, callback));
+}
+
+void RequestQueueStoreSQL::Reset(const ResetCallback& callback) {
+  DCHECK(db_.get());
+  if (!db_.get()) {
+    // 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, false));
+    return;
+  }
+
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&RequestQueueStoreSQL::ResetSync, base::Passed(&db_),
+                 db_file_path_, base::ThreadTaskRunnerHandle::Get(),
+                 base::Bind(&RequestQueueStoreSQL::OnResetDone,
+                            weak_ptr_factory_.GetWeakPtr(), callback)));
+}
+
+void RequestQueueStoreSQL::OpenConnection() {
+  DCHECK(!db_);
+  db_.reset(new sql::Connection());
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&RequestQueueStoreSQL::OpenConnectionSync, db_.get(),
+                 base::ThreadTaskRunnerHandle::Get(), db_file_path_,
+                 base::Bind(&RequestQueueStoreSQL::OnOpenConnectionDone,
+                            weak_ptr_factory_.GetWeakPtr())));
+}
+
+void RequestQueueStoreSQL::OnOpenConnectionDone(bool success) {
+  DCHECK(db_.get());
+
+  // Unfortunately we were not able to open DB connection.
+  if (!success) {
+    LOG(ERROR) << "Database creation fialed.";

[Scott Hess - ex-Googler, 2016/06/20 19:38:33]

"fialed", though if the logging were removed entirely that wouldn't be an issue
:-).

[fgorski, 2016/06/20 23:43:00]

Done.

+    db_.reset();
+  }
+
+  // If reset callback is set this opening followed a reset. Inform the caller
+  // about the success.

[Scott Hess - ex-Googler, 2016/06/20 19:38:33]

If you bundled the background processing together as suggested earlier, I don't
think you'd need this callback storage.

[fgorski, 2016/06/20 23:42:59]

OK. I managed to get rid of reset_callback_.

+  if (!reset_callback_.is_null()) {
+    reset_callback_.Run(success);
+    reset_callback_.Reset();
+  }
+}
+
+void RequestQueueStoreSQL::OnResetDone(const ResetCallback& callback,
+                                       bool success) {
+  if (!success) {
+    callback.Run(false);
+    return;
+  }
+
+  DCHECK(reset_callback_.is_null());
+  reset_callback_ = callback;
+  OpenConnection();
+}
+
+}  // namespace offline_pages