Use a vector of smart pointers for callback return type.

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"
@@ skipping 43 matching lines @@
   if (!CreateRequestQueueTable(db))
     return false;
 
   // TODO(fgorski): Add indices here.
   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.
-SavePageRequest MakeSavePageRequest(const sql::Statement& statement) {
+std::unique_ptr<SavePageRequest> MakeSavePageRequest(
+    const sql::Statement& statement) {
   const int64_t id = statement.ColumnInt64(0);
   const base::Time creation_time =
       base::Time::FromInternalValue(statement.ColumnInt64(1));
   const base::Time activation_time =
       base::Time::FromInternalValue(statement.ColumnInt64(2));
   const base::Time last_attempt_time =
       base::Time::FromInternalValue(statement.ColumnInt64(3));
   const int64_t started_attempt_count = statement.ColumnInt64(4);
   const int64_t completed_attempt_count = statement.ColumnInt64(5);
   const SavePageRequest::RequestState state =
       static_cast<SavePageRequest::RequestState>(statement.ColumnInt64(6));
   const GURL url(statement.ColumnString(7));
   const ClientId client_id(statement.ColumnString(8),
                            statement.ColumnString(9));
 
   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);
-  request.set_started_attempt_count(started_attempt_count);
-  request.set_completed_attempt_count(completed_attempt_count);
-  request.set_request_state(state);
+  std::unique_ptr<SavePageRequest> request(new SavePageRequest(
+      id, url, client_id, creation_time, activation_time, kUserRequested));
+  request->set_last_attempt_time(last_attempt_time);
+  request->set_started_attempt_count(started_attempt_count);
+  request->set_completed_attempt_count(completed_attempt_count);
+  request->set_request_state(state);
   return request;
 }
 
 // Get a request for a specific id.
-SavePageRequest GetOneRequest(sql::Connection* db, const int64_t request_id) {
+std::unique_ptr<SavePageRequest> GetOneRequest(sql::Connection* db,
+                                               const int64_t request_id) {
   const char kSql[] =
       "SELECT request_id, creation_time, activation_time,"
       " last_attempt_time, started_attempt_count, completed_attempt_count,"
       " 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);
 }
 
 void BuildFailedResultList(const std::vector<int64_t>& request_ids,
                            RequestQueue::UpdateMultipleRequestResults results) {
   results.clear();
-  for (int64_t request_id : request_ids)
+  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,
                                                     int64_t request_id) {
   const char kSql[] =
       "DELETE FROM " REQUEST_QUEUE_TABLE_NAME " WHERE request_id=?";
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
   statement.BindInt64(0, request_id);
   if (!statement.Run())
     return RequestQueue::UpdateRequestResult::STORE_FAILURE;
   else if (db->GetLastChangeCount() == 0)
     return RequestQueue::UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
   else
     return RequestQueue::UpdateRequestResult::SUCCESS;
 }
 
 bool ChangeRequestState(sql::Connection* db,
                         const int64_t request_id,
                         const SavePageRequest::RequestState new_state) {
   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<int64_t>(new_state));
   statement.BindInt64(1, request_id);
   return statement.Run();
 }
 
-bool DeleteRequestsByIds(sql::Connection* db,
-                         const std::vector<int64_t>& request_ids,
-                         RequestQueue::UpdateMultipleRequestResults& results,
-                         std::vector<SavePageRequest>& requests) {
+// Helper function to delete requests corresponding to passed in requestIds,
+// and fill an outparam with the removed requests.
+bool DeleteRequestsByIds(
+    sql::Connection* db,
+    const std::vector<int64_t>& request_ids,
+    RequestQueue::UpdateMultipleRequestResults& results,
+    std::vector<std::unique_ptr<SavePageRequest>>& requests) {

[Bernhard Bauer, 2016/09/08 09:01:57]

Pointer.

[Pete Williamson, 2016/09/08 17:27:00]

Done.

   // 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()) {
     BuildFailedResultList(request_ids, results);
     return false;
   }
 
   // Read the request before we delete it, and if the delete worked, put it on
   // the queue of requests that got deleted.
   for (int64_t request_id : request_ids) {
-    SavePageRequest request = GetOneRequest(db, request_id);
+    std::unique_ptr<SavePageRequest> request = GetOneRequest(db, request_id);
     RequestQueue::UpdateRequestResult result =
         DeleteRequestById(db, request_id);
     results.push_back(std::make_pair(request_id, result));
     if (result == RequestQueue::UpdateRequestResult::SUCCESS)
-      requests.push_back(request);
+      requests.push_back(std::move(request));
   }
 
   if (!transaction.Commit()) {
     requests.clear();
     BuildFailedResultList(request_ids, results);
     return false;
   }
 
   return true;
 }
 
-bool ChangeRequestsState(sql::Connection* db,
-                         const std::vector<int64_t>& request_ids,
-                         SavePageRequest::RequestState new_state,
-                         RequestQueue::UpdateMultipleRequestResults& results,
-                         std::vector<SavePageRequest>& requests) {
+bool ChangeRequestsState(
+    sql::Connection* db,
+    const std::vector<int64_t>& request_ids,
+    SavePageRequest::RequestState new_state,
+    RequestQueue::UpdateMultipleRequestResults& results,
+    std::vector<std::unique_ptr<SavePageRequest>>& requests) {

[Bernhard Bauer, 2016/09/08 09:01:57]

Pointer.

[Pete Williamson, 2016/09/08 17:27:00]

If it's OK, I'd like to leave this for now - the ChangeRequestsState function
will be removed by https://codereview.chromium.org/2249743005, and I'd like to
reduce the merge pain.

[Bernhard Bauer, 2016/09/08 19:14:56]

OK.

   // 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()) {
     BuildFailedResultList(request_ids, results);
     return false;
   }
 
   // Update a request, then get it, and put the item we got on the output list.
   for (const auto& request_id : request_ids) {
@@ skipping 97 matching lines @@
     scoped_refptr<base::SingleThreadTaskRunner> runner,
     const GetRequestsCallback& callback) {
   const char kSql[] =
       "SELECT request_id, creation_time, activation_time,"
       " last_attempt_time, started_attempt_count, completed_attempt_count,"
       " state, url, client_namespace, client_id"
       " FROM " REQUEST_QUEUE_TABLE_NAME;
 
   sql::Statement statement(db->GetCachedStatement(SQL_FROM_HERE, kSql));
 
-  std::vector<SavePageRequest> result;
+  std::vector<std::unique_ptr<SavePageRequest>> requests;
   while (statement.Step())
-    result.push_back(MakeSavePageRequest(statement));
+    requests.push_back(MakeSavePageRequest(statement));
 
-  runner->PostTask(FROM_HERE,
-                   base::Bind(callback, statement.Succeeded(), result));
+  runner->PostTask(FROM_HERE, base::Bind(callback, statement.Succeeded(),
+                                         base::Passed(std::move(requests))));

[Bernhard Bauer, 2016/09/08 09:01:57]

FYI, you can also use base::Passed(&requests), which is a shortcut for this.

[Pete Williamson, 2016/09/08 17:27:00]

Done everywhere in this file.

 }
 
 // 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) {
   RequestQueue::UpdateMultipleRequestResults results;
-  std::vector<SavePageRequest> requests;
+  std::vector<std::unique_ptr<SavePageRequest>> requests;
   // TODO(fgorski): add UMA metrics here.
   DeleteRequestsByIds(db, request_ids, results, requests);
-  runner->PostTask(FROM_HERE, base::Bind(callback, results, requests));
+  runner->PostTask(FROM_HERE, base::Bind(callback, results,
+                                         base::Passed(std::move(requests))));
 }
 
 // static
 void RequestQueueStoreSQL::ChangeRequestsStateSync(
     sql::Connection* db,
     scoped_refptr<base::SingleThreadTaskRunner> runner,
     const std::vector<int64_t>& request_ids,
     const SavePageRequest::RequestState new_state,
     const UpdateMultipleRequestsCallback& callback) {
   RequestQueue::UpdateMultipleRequestResults results;
-  std::vector<SavePageRequest> requests;
+  std::vector<std::unique_ptr<SavePageRequest>> requests;
   // TODO(fgorski): add UMA metrics here.
   offline_pages::ChangeRequestsState(db, request_ids, new_state, results,
                                      requests);
-  runner->PostTask(FROM_HERE, base::Bind(callback, results, requests));
+  runner->PostTask(FROM_HERE, base::Bind(callback, results,
+                                         base::Passed(std::move(requests))));
 }
 
 // static
 void RequestQueueStoreSQL::ResetSync(
     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 and reinitializes it.
   bool success = db->Raze();
@@ skipping 10 matching lines @@
     // to preserve the async style behavior to prevent bugs.
     base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
                                                   base::Bind(callback));
     return false;
   }
   return true;
 }
 
 void RequestQueueStoreSQL::GetRequests(const GetRequestsCallback& callback) {
   DCHECK(db_.get());
-  if (!CheckDb(base::Bind(callback, false, std::vector<SavePageRequest>())))
+  std::vector<std::unique_ptr<SavePageRequest>> requests;
+  if (!CheckDb(base::Bind(callback, false, base::Passed(std::move(requests)))))
     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 (!CheckDb(base::Bind(callback, UpdateStatus::FAILED)))
     return;
 
   background_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&RequestQueueStoreSQL::AddOrUpdateRequestSync, db_.get(),
                  base::ThreadTaskRunnerHandle::Get(), request, callback));
 }
 
 // RemoveRequestsByRequestId to be more parallell with RemoveRequestsByClientId.
 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;
-  std::vector<SavePageRequest> requests;
-  for (int64_t request_id : request_ids)
+  for (int64_t request_id : request_ids) {
     results.push_back(std::make_pair(
         request_id, RequestQueue::UpdateRequestResult::STORE_FAILURE));
+  }
 
-  if (!CheckDb(base::Bind(callback, results, requests)))
+  if (!CheckDb(base::Bind(
+          callback, results,
+          base::Passed(std::vector<std::unique_ptr<SavePageRequest>>())))) {
     return;
+  }
 
   background_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&RequestQueueStoreSQL::RemoveRequestsSync, db_.get(),
                  base::ThreadTaskRunnerHandle::Get(), request_ids, callback));
 }
 
 void RequestQueueStoreSQL::ChangeRequestsState(
     const std::vector<int64_t>& request_ids,
     const SavePageRequest::RequestState new_state,
     const UpdateMultipleRequestsCallback& callback) {
   RequestQueue::UpdateMultipleRequestResults results;
-  std::vector<SavePageRequest> requests;
-  if (!CheckDb(base::Bind(callback, results, requests)))
+  std::vector<std::unique_ptr<SavePageRequest>> requests;
+  if (!CheckDb(
+          base::Bind(callback, results, base::Passed(std::move(requests))))) {
     return;
+  }
 
   background_task_runner_->PostTask(
       FROM_HERE, base::Bind(&RequestQueueStoreSQL::ChangeRequestsStateSync,
                             db_.get(), base::ThreadTaskRunnerHandle::Get(),
                             request_ids, new_state, callback));
 }
 
 void RequestQueueStoreSQL::Reset(const ResetCallback& callback) {
   DCHECK(db_.get());
   if (!CheckDb(base::Bind(callback, false)))
@@ skipping 27 matching lines @@
 }
 
 void RequestQueueStoreSQL::OnResetDone(const ResetCallback& callback,
                                        bool success) {
   // Complete connection initialization post reset.
   OnOpenConnectionDone(success);
   callback.Run(success);
 }
 
 }  // namespace offline_pages