Provide API in RequestCoordinator to remove results by client ID.

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 a1be28c893cfc63947c4eaef29bbd604b76ea2f2..3ad447c02834e3da7826bb822bb63504cfc7cc8d 100644
--- a/components/offline_pages/background/request_queue_store_sql.cc
+++ b/components/offline_pages/background/request_queue_store_sql.cc
@@ -8,6 +8,7 @@
 #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"
@@ -55,6 +56,18 @@ bool DeleteRequestById(sql::Connection* db, int64_t request_id) {
   return statement.Run();
 }
 
+bool DeleteRequestByClientId(sql::Connection* db, const ClientId client_id) {
+  const char kSql[] =
+      "DELETE FROM " REQUEST_QUEUE_TABLE_NAME
+      " WHERE client_namespace=? AND client_id=?";
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+  statement.BindString(0, client_id.name_space);
+  statement.BindString(1, client_id.id);
+  DVLOG(2) << kSql << " client_namespace " << client_id.name_space
+           << " client_id " << client_id.id;
+  return statement.Run();
+}
+
 bool DeleteRequestsByIds(sql::Connection* db,
                          const std::vector<int64_t>& request_ids,
                          int* count) {
@@ -78,6 +91,29 @@ bool DeleteRequestsByIds(sql::Connection* db,
   return true;
 }
 
+bool DeleteRequestsByClientIds(sql::Connection* db,
+                         const std::vector<ClientId>& client_ids,
+                         int* count) {
+  DCHECK(count);
+  // 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;
+
+  *count = 0;
+  for (auto client_id : client_ids) {
+    if (!DeleteRequestByClientId(db, client_id))
+      return false;
+    *count += db->GetLastChangeCount();
+  }
+
+  if (!transaction.Commit())
+    return false;
+
+  return true;
+}
+
 // 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.
@@ -94,6 +130,12 @@ SavePageRequest MakeSavePageRequest(const sql::Statement& statement) {
   const ClientId client_id(statement.ColumnString(6),
                            statement.ColumnString(7));
 
+  DVLOG(2) << "making save page request - id " << id
+           << " url " << url
+           << " client_id " << client_id.name_space << "-" << client_id.id
+           << " creation time " << creation_time
+           << " user requested " << kUserRequested;
+
   SavePageRequest request(
       id, url, client_id, creation_time, activation_time, kUserRequested);
   request.set_last_attempt_time(last_attempt_time);
@@ -216,6 +258,21 @@ void RequestQueueStoreSQL::RemoveRequestsSync(
     runner->PostTask(FROM_HERE, base::Bind(callback, false, 0));
 }
 
+
+// static
+void RequestQueueStoreSQL::RemoveRequestsByClientIdSync(
+    sql::Connection* db,
+    scoped_refptr<base::SingleThreadTaskRunner> runner,
+    const std::vector<ClientId>& client_ids,
+    const RemoveCallback& callback) {
+  // TODO(fgorski): add UMA metrics here.
+  int count = 0;
+  if (DeleteRequestsByClientIds(db, client_ids, &count))

[dougarnett, 2016/08/01 18:28:35]

Some naming inconsistencies here with Remove/Delete and ByClientId/ByClientIds.
I wonder if at least try to make the ByClientId[s] part consistent.

[dougarnett, 2016/08/02 00:12:39]

That is, either never *"ByClientIds" or else make RequestQueue
RemoveRequestsByClientId() => RemoveRequestsByClientIds() to match style here

[Pete Williamson, 2016/08/02 00:35:08]

My plan here is to just remove the "RemoveRequests" in a future checkin.  TODO
added (but I didn't rename it, since it will be going away).

+    runner->PostTask(FROM_HERE, base::Bind(callback, true, count));
+  else
+    runner->PostTask(FROM_HERE, base::Bind(callback, false, 0));
+}
+
 // static
 void RequestQueueStoreSQL::ResetSync(
     sql::Connection* db,
@@ -280,6 +337,23 @@ void RequestQueueStoreSQL::RemoveRequests(
                  base::ThreadTaskRunnerHandle::Get(), request_ids, callback));
 }
 
+void RequestQueueStoreSQL::RemoveRequestsByClientId(
+    const std::vector<ClientId>& client_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;
+  }
+
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&RequestQueueStoreSQL::RemoveRequestsByClientIdSync, db_.get(),
+                 base::ThreadTaskRunnerHandle::Get(), client_ids, callback));
+}
+
 void RequestQueueStoreSQL::Reset(const ResetCallback& callback) {
   DCHECK(db_.get());
   if (!db_.get()) {