Use a vector of smart pointers for callback return type.

components/offline_pages/background/request_picker.h

Index: components/offline_pages/background/request_picker.h
diff --git a/components/offline_pages/background/request_picker.h b/components/offline_pages/background/request_picker.h
index 04469f78576b3c011decee98e8b40188deb74152..678d3c3b0c1c564ed354f43048c7f3513c7777fa 100644
--- a/components/offline_pages/background/request_picker.h
+++ b/components/offline_pages/background/request_picker.h
@@ -37,13 +37,14 @@ class RequestPicker {
 
  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.
@@ -67,15 +68,17 @@ class RequestPicker {
   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_;