Use a vector of smart pointers for callback return type.

components/offline_pages/background/request_picker.h

 // 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.
 
 #ifndef COMPONENTS_OFFLINE_PAGES_BACKGROUND_REQUEST_PICKER_H_
 #define COMPONENTS_OFFLINE_PAGES_BACKGROUND_REQUEST_PICKER_H_
 
 #include <memory>
 
 #include "base/memory/weak_ptr.h"
@@ skipping 19 matching lines @@
 
   // Choose which request we should process next based on the current
   // conditions, and call back to the RequestCoordinator when we have one.
   void ChooseNextRequest(
       RequestCoordinator::RequestPickedCallback picked_callback,
       RequestCoordinator::RequestNotPickedCallback not_picked_callback,
       DeviceConditions* device_conditions);
 
  private:
   // Callback for the GetRequest results to be delivered.
-  void GetRequestResultCallback(RequestQueue::GetRequestsResult result,
-                                const std::vector<SavePageRequest>& results);
+  void GetRequestResultCallback(
+      RequestQueue::GetRequestsResult result,
+      std::vector<std::unique_ptr<SavePageRequest>> results);
 
   // 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 RequestConditionsSatisfied(const SavePageRequest& request);
+  bool RequestConditionsSatisfied(const SavePageRequest* request);
 
   // Using policies, decide if the new request is preferable to the best we have
   // so far.
   bool IsNewRequestBetter(const SavePageRequest* oldRequest,
                           const SavePageRequest* newRequest,
                           RequestCompareFunction comparator);
 
   // Is the new request preferable from the retry count first standpoint?
   bool RetryCountFirstCompareFunction(const SavePageRequest* left,
                                       const SavePageRequest* right);
 
   // Is the new request better from the recency first standpoint?
   bool RecencyFirstCompareFunction(const SavePageRequest* left,
                                    const SavePageRequest* right);
 
   // Does the new request have better retry count?
   int CompareRetryCount(const SavePageRequest* left,
                         const SavePageRequest* right);
 
   // Does the new request have better creation time?
   int CompareCreationTime(const SavePageRequest* left,
                           const SavePageRequest* right);
 
-  // Split all requests into expired ones and still valid ones.
-  void SplitRequests(const std::vector<SavePageRequest>& requests,
-                     std::vector<SavePageRequest>& valid_requests,
-                     std::vector<SavePageRequest>& expired_requests);
+  // Split all requests into expired ones and still valid ones.  Takes ownership
+  // of the requests, and moves them into either valid or expired requests.
+  void SplitRequests(
+      std::vector<std::unique_ptr<SavePageRequest>>& requests,

[Bernhard Bauer, 2016/09/07 10:13:41]

There are a lot of places here (in existing code as well) that use non-const
references -- any reason why?

[Pete Williamson, 2016/09/07 20:21:01]

const references won't work with moving (since you are destroying something in
the place that you are moving it from, it can't be const).

In this particular function, we are splitting requests from a vector of requests
into two outparams.  

In just a few places, we are getting a const reference to this vector of smart
pointers, because it is occasionally used without being moved or copied, but in
most places it gets std::move-d to its final destination, so in most cases, we
use non-const vectors to work with std::move()

[Bernhard Bauer, 2016/09/07 21:43:52]

OK, but outparams should be passed as pointers, no? And regular params that are
moved should take the plain type. Right now you're using mutable references for
both, which is not only discouraged by the style guide, but also makes the
distinction between types of parameters less visible.

[Pete Williamson, 2016/09/07 22:37:07]

Done.

+      std::vector<std::unique_ptr<SavePageRequest>>& valid_requests,
+      std::vector<std::unique_ptr<SavePageRequest>>& expired_requests);
 
   // Callback used after requests get expired.
   void OnRequestExpired(
       const RequestQueue::UpdateMultipleRequestResults& results,
-      const std::vector<SavePageRequest>& requests);
+      const std::vector<std::unique_ptr<SavePageRequest>> requests);
 
   // Unowned pointer to the request queue.
   RequestQueue* queue_;
   // Unowned pointer to the policy object.
   OfflinerPolicy* policy_;
   // Unowned pointer to the request coordinator.
   RequestNotifier* notifier_;
   // Current conditions on the device
   std::unique_ptr<DeviceConditions> current_conditions_;
   // True if we prefer less-tried requests
   bool fewer_retries_better_;
   // True if we prefer requests submitted more recently
   bool earlier_requests_better_;
   // Callback for when we are done picking a request to do next.
   RequestCoordinator::RequestPickedCallback picked_callback_;
   // Callback for when there are no more reqeusts to pick.
   RequestCoordinator::RequestNotPickedCallback not_picked_callback_;
   // Allows us to pass a weak pointer to callbacks.
   base::WeakPtrFactory<RequestPicker> weak_ptr_factory_;
 };
 
 }  // namespace offline_pages
 
 #endif  // COMPONENTS_OFFLINE_PAGES_BACKGROUND_REQUEST_PICKER_H_