[Offline pages] Extracting AddRequest method in RequestQueueStore

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 908516979b02260248e911a998c68e824cddd455..9e7d29f3fdb71a4b0e882fcc4500b9d495e331a2 100644
--- a/components/offline_pages/background/request_queue_store_sql.cc
+++ b/components/offline_pages/background/request_queue_store_sql.cc
@@ -20,6 +20,8 @@ namespace offline_pages {
 
 namespace {
 
+using UpdateStatus = RequestQueueStore::UpdateStatus;
+
 // 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"
@@ -108,6 +110,36 @@ std::unique_ptr<SavePageRequest> GetOneRequest(sql::Connection* db,
   return MakeSavePageRequest(statement);
 }
 
+ItemActionStatus Insert(sql::Connection* db, const SavePageRequest& request) {

[Pete Williamson, 2016/09/21 00:00:08]

Nit - should we place this just above InsertOrReplace to maintain the function
ordering scheme in place already in the file?

[fgorski, 2016/09/21 05:07:50]

Done. To be honest I am not very fond of the current ordering. It feels rather
accidental.

+  const char kSql[] =
+      "INSERT OR IGNORE INTO " REQUEST_QUEUE_TABLE_NAME
+      " (request_id, creation_time, activation_time,"

[Pete Williamson, 2016/09/21 00:00:08]

I remember DewittJ suggesting in the OfflinePageModel that we use a
parameterized form of this statement to make it harder to get things out of
order - would that also make sense here?

[fgorski, 2016/09/21 05:07:50]

No, update will have a different form. Please compare to the update statement in
the OPM, or wait for the next patch.

+      " last_attempt_time, started_attempt_count, completed_attempt_count,"
+      " state, url, client_namespace, client_id)"
+      " VALUES "
+      " (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)";
+
+  sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
+  statement.BindInt64(0, request.request_id());
+  statement.BindInt64(1, request.creation_time().ToInternalValue());
+  statement.BindInt64(2, request.activation_time().ToInternalValue());
+  statement.BindInt64(3, request.last_attempt_time().ToInternalValue());
+  statement.BindInt64(4, request.started_attempt_count());
+  statement.BindInt64(5, request.completed_attempt_count());
+  statement.BindInt64(6, static_cast<int64_t>(request.request_state()));
+  statement.BindString(7, request.url().spec());
+  statement.BindString(8, request.client_id().name_space);
+  statement.BindString(9, request.client_id().id);
+
+  // TODO(fgorski): This will be an ItemActionStatus, with ALREADY_EXISTS as a
+  // return value for second if.

[Pete Williamson, 2016/09/21 00:00:08]

Is this TODO already TODONE?  If not, add a bit more explanation, please.

[fgorski, 2016/09/21 05:07:50]

Done.

+  if (!statement.Run())
+    return ItemActionStatus::STORE_ERROR;
+  if (db->GetLastChangeCount() == 0)
+    return ItemActionStatus::ALREADY_EXISTS;
+  return ItemActionStatus::SUCCESS;
+}
+
 void BuildFailedResultList(const std::vector<int64_t>& request_ids,
                            RequestQueue::UpdateMultipleRequestResults results) {
   results.clear();
@@ -281,6 +313,14 @@ void GetRequestsSync(sql::Connection* db,
                                          base::Passed(&requests)));
 }
 
+void AddRequestSync(sql::Connection* db,
+                    scoped_refptr<base::SingleThreadTaskRunner> runner,
+                    const SavePageRequest& request,
+                    const RequestQueueStore::AddCallback& callback) {

[Pete Williamson, 2016/09/21 00:00:08]

Should we also have a TODO for UMA here?

[fgorski, 2016/09/21 05:07:50]

Done.

+  ItemActionStatus status = Insert(db, request);
+  runner->PostTask(FROM_HERE, base::Bind(callback, status));
+}
+
 void AddOrUpdateRequestSync(sql::Connection* db,
                             scoped_refptr<base::SingleThreadTaskRunner> runner,
                             const SavePageRequest& request,
@@ -376,6 +416,17 @@ void RequestQueueStoreSQL::GetRequests(const GetRequestsCallback& callback) {
                             base::ThreadTaskRunnerHandle::Get(), callback));
 }
 
+void RequestQueueStoreSQL::AddRequest(const SavePageRequest& request,
+                                      const AddCallback& callback) {
+  if (!CheckDb(base::Bind(callback, ItemActionStatus::STORE_ERROR)))
+    return;
+
+  background_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&AddRequestSync, db_.get(),
+                 base::ThreadTaskRunnerHandle::Get(), request, callback));
+}
+
 void RequestQueueStoreSQL::AddOrUpdateRequest(const SavePageRequest& request,
                                               const UpdateCallback& callback) {
   DCHECK(db_.get());
@@ -388,7 +439,7 @@ void RequestQueueStoreSQL::AddOrUpdateRequest(const SavePageRequest& request,
                  base::ThreadTaskRunnerHandle::Get(), request, callback));
 }
 
-// RemoveRequestsByRequestId to be more parallell with RemoveRequestsByClientId.
+// RemoveRequestsByRequestId to be more parallel with RemoveRequestsByClientId.

[Pete Williamson, 2016/09/21 00:00:08]

Should we add "TODO" to this comment?

[fgorski, 2016/09/21 05:07:51]

Done. It should be actually gone. Thanks for pointing out a more obvious problem
with that statement than the typo I was fixing.

 void RequestQueueStoreSQL::RemoveRequests(
     const std::vector<int64_t>& request_ids,
     const RemoveCallback& callback) {