Use a vector of smart pointers for callback return type.

components/offline_pages/background/request_picker.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_picker.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "components/offline_pages/background/save_page_request.h"
 
@@ skipping 32 matching lines @@
   current_conditions_.reset(new DeviceConditions(*device_conditions));
   // Get all requests from queue (there is no filtering mechanism).
   queue_->GetRequests(base::Bind(&RequestPicker::GetRequestResultCallback,
                                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 // When we get contents from the queue, use them to pick the next
 // request to operate on (if any).
 void RequestPicker::GetRequestResultCallback(
     RequestQueue::GetRequestsResult,
-    const std::vector<SavePageRequest>& requests) {
+    std::vector<std::unique_ptr<SavePageRequest>> requests) {
   // If there is nothing to do, return right away.
   if (requests.size() == 0) {
     not_picked_callback_.Run(false);
     return;
   }
 
   // Get the expired requests to be removed from the queue, and the valid ones
   // from which to pick the next request.
-  std::vector<SavePageRequest> valid_requests;
-  std::vector<SavePageRequest> expired_requests;
-  SplitRequests(requests, valid_requests, expired_requests);
+  std::vector<std::unique_ptr<SavePageRequest>> valid_requests;
+  std::vector<std::unique_ptr<SavePageRequest>> expired_requests;
+  SplitRequests(std::move(requests), &valid_requests, &expired_requests);
   std::vector<int64_t> expired_request_ids;
-  for (auto request : expired_requests)
-    expired_request_ids.push_back(request.request_id());
+  for (const auto& request : expired_requests)
+    expired_request_ids.push_back(request->request_id());
 
   queue_->RemoveRequests(expired_request_ids,
                          base::Bind(&RequestPicker::OnRequestExpired,
                                     weak_ptr_factory_.GetWeakPtr()));
 
   // Pick the most deserving request for our conditions.
   const SavePageRequest* picked_request = nullptr;
 
   RequestCompareFunction comparator = nullptr;
 
   // Choose which comparison function to use based on policy.
   if (policy_->RetryCountIsMoreImportantThanRecency())
     comparator = &RequestPicker::RetryCountFirstCompareFunction;
   else
     comparator = &RequestPicker::RecencyFirstCompareFunction;
 
   // Iterate once through the requests, keeping track of best candidate.
   bool non_user_requested_tasks_remaining = false;
   for (unsigned i = 0; i < valid_requests.size(); ++i) {
-    if (!valid_requests[i].user_requested())
+    if (!valid_requests[i]->user_requested())
       non_user_requested_tasks_remaining = true;
-    if (!RequestConditionsSatisfied(valid_requests[i]))
+    if (!RequestConditionsSatisfied(valid_requests[i].get()))
       continue;
-    if (IsNewRequestBetter(picked_request, &(valid_requests[i]), comparator))
-      picked_request = &(valid_requests[i]);
+    if (IsNewRequestBetter(picked_request, valid_requests[i].get(), comparator))
+      picked_request = valid_requests[i].get();
   }
 
   // If we have a best request to try next, get the request coodinator to
   // start it.  Otherwise return that we have no candidates.
   if (picked_request != nullptr) {
     picked_callback_.Run(*picked_request);
   } else {
     not_picked_callback_.Run(non_user_requested_tasks_remaining);
   }
 }
 
 // Filter out requests that don't meet the current conditions.  For instance, if
 // this is a predictive request, and we are not on WiFi, it should be ignored
 // this round.
-bool RequestPicker::RequestConditionsSatisfied(const SavePageRequest& request) {
+bool RequestPicker::RequestConditionsSatisfied(const SavePageRequest* request) {
   // If the user did not request the page directly, make sure we are connected
   // to power and have WiFi and sufficient battery remaining before we take this
   // request.
 
   if (!current_conditions_->IsPowerConnected() &&
-      policy_->PowerRequired(request.user_requested())) {
+      policy_->PowerRequired(request->user_requested())) {
     return false;
   }
 
   if (current_conditions_->GetNetConnectionType() !=
           net::NetworkChangeNotifier::ConnectionType::CONNECTION_WIFI &&
-      policy_->UnmeteredNetworkRequired(request.user_requested())) {
+      policy_->UnmeteredNetworkRequired(request->user_requested())) {
     return false;
   }
 
   if (current_conditions_->GetBatteryPercentage() <
-      policy_->BatteryPercentageRequired(request.user_requested())) {
+      policy_->BatteryPercentageRequired(request->user_requested())) {
     return false;
   }
 
   // If we have already started this page the max number of times, it is not
   // eligible to try again.
-  if (request.started_attempt_count() >= policy_->GetMaxStartedTries())
+  if (request->started_attempt_count() >= policy_->GetMaxStartedTries())
     return false;
 
   // If we have already completed trying this page the max number of times, it
   // is not eligible to try again.
-  if (request.completed_attempt_count() >= policy_->GetMaxCompletedTries())
+  if (request->completed_attempt_count() >= policy_->GetMaxCompletedTries())
     return false;
 
   // If the request is paused, do not consider it.
-  if (request.request_state() == SavePageRequest::RequestState::PAUSED)
+  if (request->request_state() == SavePageRequest::RequestState::PAUSED)
     return false;
 
   // If the request is expired, do not consider it.
-  base::TimeDelta requestAge = base::Time::Now() - request.creation_time();
+  base::TimeDelta requestAge = base::Time::Now() - request->creation_time();
   if (requestAge >
       base::TimeDelta::FromSeconds(
           policy_->GetRequestExpirationTimeInSeconds()))
     return false;
 
   // If this request is not active yet, return false.
   // TODO(petewil): If the only reason we return nothing to do is that we have
   // inactive requests, we still want to try again later after their activation
   // time elapses, we shouldn't take ourselves completely off the scheduler.
-  if (request.activation_time() > base::Time::Now())
+  if (request->activation_time() > base::Time::Now())
     return false;
 
   return true;
 }
 
 // Look at policies to decide which requests to prefer.
 bool RequestPicker::IsNewRequestBetter(const SavePageRequest* oldRequest,
                                        const SavePageRequest* newRequest,
                                        RequestCompareFunction comparator) {
 
@@ skipping 65 matching lines @@
   base::TimeDelta difference = left->creation_time() - right->creation_time();
   int result = signum(difference.InMilliseconds());
 
   // Flip the direction of comparison if policy prefers fewer retries.
   if (earlier_requests_better_)
     result *= -1;
 
   return result;
 }
 
-// Split all requests into expired ones and still valid ones.
 void RequestPicker::SplitRequests(
-    const std::vector<SavePageRequest>& requests,
-    std::vector<SavePageRequest>& valid_requests,
-    std::vector<SavePageRequest>& expired_requests) {
-  for (SavePageRequest request : requests) {
-    if (base::Time::Now() - request.creation_time() >=
+    std::vector<std::unique_ptr<SavePageRequest>> requests,
+    std::vector<std::unique_ptr<SavePageRequest>>* valid_requests,
+    std::vector<std::unique_ptr<SavePageRequest>>* expired_requests) {
+  std::vector<std::unique_ptr<SavePageRequest>>::iterator request;

[brucedawson, 2016/09/12 17:20:04]

This local variable is not used and should be deleted. It currently triggers
variable shadowing warnings on the experimental VC++ /analyze (static code
analysis) build.

[Pete Williamson, 2016/09/12 20:25:57]

Removed in https://codereview.chromium.org/2332923003.

+  for (auto& request : requests) {
+    if (base::Time::Now() - request->creation_time() >=
         base::TimeDelta::FromSeconds(kRequestExpirationTimeInSeconds)) {
-      expired_requests.push_back(request);
+      expired_requests->push_back(std::move(request));
     } else {
-      valid_requests.push_back(request);
+      valid_requests->push_back(std::move(request));
     }
   }
 }
 
 // Callback used after expired requests are deleted from the queue and notifies
 // the coordinator.
 void RequestPicker::OnRequestExpired(
     const RequestQueue::UpdateMultipleRequestResults& results,
-    const std::vector<SavePageRequest>& requests) {
-  for (auto request : requests)
+    const std::vector<std::unique_ptr<SavePageRequest>> requests) {
+  std::vector<std::unique_ptr<SavePageRequest>>::const_iterator request;
+  for (request = requests.begin(); request != requests.end(); ++request)

[brucedawson, 2016/09/12 17:20:06]

I'm confused as to why this was changed to an explicit for loop - the
inconsistency with the previous function seems odd, and I'm failing to see the
reason. What am I missing?

[Pete Williamson, 2016/09/12 20:25:57]

Due to CR feedback, I changed some places to use the for_each style, but forgot
to check the file for all other uses.  Fixed in
https://codereview.chromium.org/2332923003.

[brucedawson, 2016/09/12 20:48:40]

Although, this one *started out* using for-each style, so that's curious. But,
no matter.

     notifier_->NotifyCompleted(
-        request, RequestCoordinator::BackgroundSavePageResult::EXPIRED);
+        *(request->get()),
+        RequestCoordinator::BackgroundSavePageResult::EXPIRED);
 }
 
 }  // namespace offline_pages