Add an API to Pause and Resume background offlining requests.

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
index 264b2acb42734ee9db93c9ceefd8bde8a97f26b6..4786b0b6e3eb9c0774f4d5dd2f1e1931c202c4d4 100644
--- a/components/offline_pages/background/request_queue_store_sql.cc
+++ b/components/offline_pages/background/request_queue_store_sql.cc
@@ -77,6 +77,28 @@ bool DeleteRequestByClientId(sql::Connection* db, const ClientId& client_id) {
   return statement.Run();
 }
 
+bool PauseRequest(sql::Connection* db, const int64_t request_id) {
+  const char kSql[] = "UPDATE " REQUEST_QUEUE_TABLE_NAME
+                      " SET state=?"
+                      " WHERE request_id=?";
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+  statement.BindInt64(
+      0, static_cast<long>(SavePageRequest::RequestState::PAUSED));
+  statement.BindInt64(1, request_id);
+  return statement.Run();
+}
+
+bool ResumeRequest(sql::Connection* db, const int64_t request_id) {
+  const char kSql[] = "UPDATE " REQUEST_QUEUE_TABLE_NAME
+                      " SET state=?"
+                      " WHERE request_id=?";
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+  statement.BindInt64(
+      0, static_cast<long>(SavePageRequest::RequestState::AVAILABLE));
+  statement.BindInt64(1, request_id);
+  return statement.Run();
+}
+
 bool DeleteRequestsByIds(sql::Connection* db,
                          const std::vector<int64_t>& request_ids,
                          int* count) {
@@ -123,6 +145,43 @@ bool DeleteRequestsByClientIds(sql::Connection* db,
   return true;
 }
 
+RequestQueueStore::UpdateStatus PauseRequests(
+    sql::Connection* db, const std::vector<int64_t>& request_ids) {
+  // 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 RequestQueueStore::UpdateStatus::FAILED;
+  for (const auto& request_id : request_ids) {
+    if (!PauseRequest(db, request_id))
+      return RequestQueueStore::UpdateStatus::FAILED;
+  }
+
+  if (!transaction.Commit())
+    return RequestQueueStore::UpdateStatus::FAILED;
+
+  return RequestQueueStore::UpdateStatus::UPDATED;
+}
+
+RequestQueueStore::UpdateStatus ResumeRequests(
+    sql::Connection* db, const std::vector<int64_t>& request_ids) {
+  // 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 RequestQueueStore::UpdateStatus::FAILED;
+
+  for (const auto& request_id : request_ids) {
+    if (!ResumeRequest(db, request_id))
+      return RequestQueueStore::UpdateStatus::FAILED;
+  }
+
+  if (!transaction.Commit())
+    return RequestQueueStore::UpdateStatus::FAILED;
+
+  return RequestQueueStore::UpdateStatus::UPDATED;
+}
+
 // Create a save page request from a SQL result.  Expects complete rows with
 // all columns present.  Columns are in order they are defined in select query
 // in |RequestQueueStore::RequestSync| method.
@@ -290,6 +349,30 @@ void RequestQueueStoreSQL::RemoveRequestsByClientIdSync(
 }
 
 // static
+void RequestQueueStoreSQL::PauseRequestsSync(

[Dmitry Titov, 2016/08/09 23:46:33]

I am not sure the store must have methods like Pause/Resume etc. All of those
are just changing a single data field, 'state'. From Storage pov, it's just
changing state. SO there can be just one single method. This will also reduce
amount of boilerplate.

I would think that RequestQueue also does not have to keep these 2 methods. From
Queue perspective, it's also just a state change.

It looks like only RequestCoordinator has to have those methods, quickly
converting them to something like RequestQueue::ChangeRequestState(..) and
possibly updating the scheduler etc...

[Pete Williamson, 2016/08/10 01:34:10]

Done.

+    sql::Connection* db,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const std::vector<int64_t>& request_ids,
+    const UpdateCallback& callback) {
+  // TODO(fgorski): add UMA metrics here.
+  RequestQueueStore::UpdateStatus status =
+      offline_pages::PauseRequests(db, request_ids);
+  runner->PostTask(FROM_HERE, base::Bind(callback, status));
+}
+
+// static
+void RequestQueueStoreSQL::ResumeRequestsSync(
+    sql::Connection* db,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const std::vector<int64_t>& request_ids,
+    const UpdateCallback& callback) {
+  // TODO(fgorski): add UMA metrics here.
+  RequestQueueStore::UpdateStatus status =
+      offline_pages::ResumeRequests(db, request_ids);
+  runner->PostTask(FROM_HERE, base::Bind(callback, status));
+}
+
+// static
 void RequestQueueStoreSQL::ResetSync(
     sql::Connection* db,
     const base::FilePath& db_file_path,
@@ -303,15 +386,60 @@ void RequestQueueStoreSQL::ResetSync(
   runner->PostTask(FROM_HERE, base::Bind(callback, success));
 }
 
-void RequestQueueStoreSQL::GetRequests(const GetRequestsCallback& callback) {
+bool RequestQueueStoreSQL::CheckDb(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 false;
+  }
+  return true;
+}
+
+bool RequestQueueStoreSQL::CheckDb(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;
+    return false;
   }
+  return true;
+}
+
+bool RequestQueueStoreSQL::CheckDb(const UpdateCallback& callback) {

[Dmitry Titov, 2016/08/09 23:46:33]

The answer to multiple CheckDB functions is to pass a bound callback, like this:
if (!CheckDb(base::Bind(callback, false))
  return;

that will make all parameters simply Closure, right?

[Pete Williamson, 2016/08/10 01:34:10]

Done.

+  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 false;
+  }
+
+  return true;
+}
+
+bool RequestQueueStoreSQL::CheckDb(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 false;
+  }
+
+  return true;
+}
+
+void RequestQueueStoreSQL::GetRequests(const GetRequestsCallback& callback) {
+  DCHECK(db_.get());
+  if (!CheckDb(callback))
+    return;
 
   background_task_runner_->PostTask(
       FROM_HERE, base::Bind(&RequestQueueStoreSQL::GetRequestsSync, db_.get(),
@@ -321,13 +449,8 @@ void RequestQueueStoreSQL::GetRequests(const GetRequestsCallback& 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));
+  if (!CheckDb(callback))
     return;
-  }
 
   background_task_runner_->PostTask(
       FROM_HERE,
@@ -335,21 +458,11 @@ void RequestQueueStoreSQL::AddOrUpdateRequest(const SavePageRequest& request,
                  base::ThreadTaskRunnerHandle::Get(), request, callback));
 }
 
-// TODO(petewil): This is unused, since request_coordinator doesn't keep
-// request_ids, and neither do clients.  Plan is to remove this API in a future
-// changelist.  If we do discover a need to keep it, name it
-// RemoveRequestsByRequestId to be more parallell with RemoveRequestsByClientId.
 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));
+  if (!CheckDb(callback))
     return;
-  }
 
   background_task_runner_->PostTask(
       FROM_HERE,
@@ -375,13 +488,35 @@ void RequestQueueStoreSQL::RemoveRequestsByClientId(
                  base::ThreadTaskRunnerHandle::Get(), client_ids, callback));
 }
 
+void RequestQueueStoreSQL::PauseRequests(
+    const std::vector<int64_t>& request_ids,
+    const UpdateCallback& callback) {
+  if (!CheckDb(callback))
+    return;
+
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&RequestQueueStoreSQL::PauseRequestsSync, db_.get(),
+                 base::ThreadTaskRunnerHandle::Get(), request_ids, callback));
+}
+
+void RequestQueueStoreSQL::ResumeRequests(
+    const std::vector<int64_t>& request_ids,
+    const UpdateCallback& callback) {
+  if (!CheckDb(callback)) {
+    return;
+  }
+
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&RequestQueueStoreSQL::ResumeRequestsSync, 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));
+  if (!CheckDb(callback)) {
     return;
   }