[Offline pages] Extracting AddRequest method in RequestQueueStore

components/offline_pages/background/request_queue_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/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 {
 
+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"
 const bool kUserRequested = true;
 
 bool CreateRequestQueueTable(sql::Connection* db) {
   const char kSql[] = "CREATE TABLE IF NOT EXISTS " REQUEST_QUEUE_TABLE_NAME
                       " (request_id INTEGER PRIMARY KEY NOT NULL,"
                       " creation_time INTEGER NOT NULL,"
                       " activation_time INTEGER NOT NULL DEFAULT 0,"
@@ skipping 68 matching lines @@
       " state, url, client_namespace, client_id"
       " FROM " REQUEST_QUEUE_TABLE_NAME " WHERE request_id=?";
 
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt64(0, request_id);
 
   statement.Run();
   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();
   for (int64_t request_id : request_ids) {
     results.push_back(std::make_pair(
         request_id, RequestQueue::UpdateRequestResult::STORE_FAILURE));
   }
 }
 
 RequestQueue::UpdateRequestResult DeleteRequestById(sql::Connection* db,
@@ skipping 153 matching lines @@
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
 
   std::vector<std::unique_ptr<SavePageRequest>> requests;
   while (statement.Step())
     requests.push_back(MakeSavePageRequest(statement));
 
   runner->PostTask(FROM_HERE, base::Bind(callback, statement.Succeeded(),
                                          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,
                             const RequestQueueStore::UpdateCallback& callback) {
   // TODO(fgorski): add UMA metrics here.
   RequestQueueStore::UpdateStatus status = InsertOrReplace(db, request);
   runner->PostTask(FROM_HERE, base::Bind(callback, status));
 }
 
 void RemoveRequestsSync(sql::Connection* db,
@@ skipping 75 matching lines @@
   DCHECK(db_.get());
   std::vector<std::unique_ptr<SavePageRequest>> requests;
   if (!CheckDb(base::Bind(callback, false, base::Passed(&requests))))
     return;
 
   background_task_runner_->PostTask(
       FROM_HERE, base::Bind(&GetRequestsSync, db_.get(),
                             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());
   if (!CheckDb(base::Bind(callback, UpdateStatus::FAILED)))
     return;
 
   background_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&AddOrUpdateRequestSync, db_.get(),
                  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) {
   // Set up a failed set of results in case we fail the DB check.
   RequestQueue::UpdateMultipleRequestResults results;
   for (int64_t request_id : request_ids) {
     results.push_back(std::make_pair(
         request_id, RequestQueue::UpdateRequestResult::STORE_FAILURE));
   }
 
@@ skipping 58 matching lines @@
 }
 
 void RequestQueueStoreSQL::OnResetDone(const ResetCallback& callback,
                                        bool success) {
   // Complete connection initialization post reset.
   OnOpenConnectionDone(success);
   callback.Run(success);
 }
 
 }  // namespace offline_pages