Add a save page request to the request queue (and test it).

components/offline_pages/background/request_coordinator.cc

Index: components/offline_pages/background/request_coordinator.cc
diff --git a/components/offline_pages/background/request_coordinator.cc b/components/offline_pages/background/request_coordinator.cc
index e2270fbfd99f8df5e2b9e6a5eb2a7fd7a232fce3..94699852450de3498f167f1c67e8060937cc9330 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "base/bind.h"
 #include "components/offline_pages/background/offliner_factory.h"
 #include "components/offline_pages/background/offliner_policy.h"
 #include "components/offline_pages/background/save_page_request.h"
@@ -13,27 +14,47 @@
 
 namespace offline_pages {
 
-RequestCoordinator::RequestCoordinator(
-    std::unique_ptr<OfflinerPolicy> policy,
-    std::unique_ptr<OfflinerFactory> factory) {
+RequestCoordinator::RequestCoordinator(std::unique_ptr<OfflinerPolicy> policy,
+                                       std::unique_ptr<OfflinerFactory> factory,
+                                       std::unique_ptr<RequestQueue> queue) {
   // Do setup as needed.
   // TODO(petewil): Assert policy not null.
   policy_ = std::move(policy);
   factory_ = std::move(factory);
+  queue_ = std::move(queue);
 }
 
 RequestCoordinator::~RequestCoordinator() {}
 
 bool RequestCoordinator::SavePageLater(
     const GURL& url, const ClientId& client_id) {
+  DVLOG(0) << "URL is " << url << " " << __FUNCTION__;

[dougarnett, 2016/05/10 20:23:34]

maybe not submit? or the @@@@@ one below

[Pete Williamson, 2016/05/10 21:07:45]

Changed both to DVLOG(2).  Both of these will help as we get our wireframe up
and working.

 
   // TODO(petewil): We need a robust scheme for allocating new IDs.
-  // TODO(petewil): Build a SavePageRequest.
-  // TODO(petewil): Put the request on the request queue, nudge the scheduler.
+  static int64_t id = 0;
+
+  // Build a SavePageRequest.
+  offline_pages::SavePageRequest request(
+  id++, url, client_id, base::Time::Now());

[fgorski, 2016/05/10 21:18:12]

nit align

Also, please add a TODO to use something like base::Clock.
This will help us when testing this class.

[Pete Williamson, 2016/05/10 22:51:57]

Done.

+
+  // Put the request on the request queue.
+  queue_->AddRequest(request,
+                     base::Bind(&RequestCoordinator::AddRequestResultCallback,
+                                base::Unretained(this)));

[fgorski, 2016/05/10 21:18:12]

base::Unretained should not be used in production.

You should add a weak_ptr_factory_ to the coordinator and use weak pointer here.

[Pete Williamson, 2016/05/10 22:51:57]

Done.

+  // TODO: Do I need to persist the request in case the add fails?

[fgorski, 2016/05/10 21:18:12]

Adding a request to the queue is meant to persist it.
Perhaps the right thing to do is to update the interface, to use a callback for
SavePageLater, which will be able to send a result like that.

This is something to consider as an alternative and definitely doesn't have to
be solved for this patch.

[Pete Williamson, 2016/05/10 22:51:57]

Noted.  We'll solve this with a later patch (and I'll leave the TODO in to
remind us).

[fgorski, 2016/05/11 19:45:15]

Acknowledged.

 
   return true;
 }
 
+void RequestCoordinator::AddRequestResultCallback(
+    RequestQueue::AddRequestResult result,
+    const SavePageRequest& request) {
+  DVLOG(0) << "@@@@@@ " << __FUNCTION__;
+
+  // Inform the scheduler that we have an outstanding task.
+  // TODO(petewil): implement.
+}
+
 bool RequestCoordinator::StartProcessing(
     const ProcessingDoneCallback& callback) {
   return false;