Changes to fit better with the needs of the download manager.

components/offline_pages/background/request_coordinator.h

Index: components/offline_pages/background/request_coordinator.h
diff --git a/components/offline_pages/background/request_coordinator.h b/components/offline_pages/background/request_coordinator.h
index 5c6c03ba07193c4a779da7d6ea1d0ead94cf177e..3a683f887deaafaea8df46caf846bf6ee7939cac 100644
--- a/components/offline_pages/background/request_coordinator.h
+++ b/components/offline_pages/background/request_coordinator.h
@@ -50,21 +50,19 @@ class RequestCoordinator : public KeyedService {
   bool SavePageLater(
       const GURL& url, const ClientId& client_id, bool user_reqeusted);
 
-  // Remove a list of requests by |client_id|.  This removes requests from the
+  // Remove a list of requests by |request_id|.  This removes requests from the
   // request queue, but does not cancel an in-progress pre-render.
   // TODO(petewil): Add code to cancel an in-progress pre-render.
-  void RemoveRequests(const std::vector<ClientId>& client_ids);
+  void RemoveRequests(const std::vector<int64_t>& request_ids);
 
   // Callback that receives the response for GetQueuedRequests.  Client must
   // copy the result right away, it goes out of scope at the end of the
   // callback.
-  typedef base::Callback<void(const std::vector<ClientId>&)>
+  typedef base::Callback<void(const std::vector<SavePageRequest>&)>

[Dmitry Titov, 2016/08/10 02:52:14]

this is a strange type. A reference to a vector of actual instances. I think it
will take a copy on each SavePageRequest to construct this, and then the
receiver has to copy the whole thing again.

does std::move() work on SavePageRequests? In other words, can receiver do this:
void myCallback(const std::vector<SavePageRequest>& requests) {
  std::vector<SavePageRequest> mystorage;
  mystorage.push_back(std::move(requests[0]));
  ...
so that there is no additional copying?

[Pete Williamson, 2016/08/10 21:15:25]

We copy the SavePageRequest items into the vector, then pass a reference to that
up through several levels of callbacks, so typically the reference is getting
copied, not the vector.  My plan was for the consumer at the top level to then
make another copy.  I don't think that std::move is safe here because there
isn't a smart pointer underlying it, the vector is just on the stack.

If you like, I could change this to return a smart pointer to a const vector of
SavePageResults, that would allow a caller to use std::move

[Pete Williamson, 2016/08/12 00:00:40]

As we dicussed, we'll leave this behavior in for now.  crbug.com/637077 tracks
fixing the copying for both this and OfflinePageItem.

       QueuedRequestCallback;
 
-  // For a client namespace, get the ClientId of all requests for that
-  // namespace.
-  void GetQueuedRequests(const std::string& client_namespace,
-                         const QueuedRequestCallback& callback);
+  // Get all save page request items in the callback.
+  void GetQueuedRequests(const QueuedRequestCallback& callback);

[Dmitry Titov, 2016/08/10 02:52:14]

Can we rename it to GetAllRequests? It does return all, and there are no other
requests other than queued...

[Pete Williamson, 2016/08/10 21:15:25]

Done.

 
   // Starts processing of one or more queued save page later requests.
   // Returns whether processing was started and that caller should expect
@@ -113,8 +111,7 @@ class RequestCoordinator : public KeyedService {
  private:
   // Receives the results of a get from the request queue, and turns that into
   // ClientId objects for the caller of GetQueuedRequests.
-  void GetQueuedRequestsCallback(const std::string& client_namespace,
-                                 const QueuedRequestCallback& callback,
+  void GetQueuedRequestsCallback(const QueuedRequestCallback& callback,
                                  RequestQueue::GetRequestsResult result,
                                  const std::vector<SavePageRequest>& requests);