Remove old requests from queue, start new requests if any, add test.

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 0a0ebdb23c01345f3a6a8547ce4dd4213c6f98de..091c4c0757f9a9217c295690e852d3c01a3cd2be 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -68,6 +68,9 @@ void RequestCoordinator::AddRequestResultCallback(
   scheduler_->Schedule(conditions);
 }
 
+void RequestCoordinator::UpdateRequestCallback(
+    RequestQueue::UpdateRequestResult result) {}

[dougarnett, 2016/06/14 18:12:52]

Where do we re-schedule and/or unschedule depending on any more items in queue?

[Pete Williamson, 2016/06/14 21:03:14]

OfflinerDoneCallback calls TryNextRequest() to schedule the next item in the
queue.  The proper place to unschedule is RequestQueueEmpty().

+
 void RequestCoordinator::RequestPicked(const SavePageRequest& request) {
   Scheduler::TriggerCondition conditions;
 
@@ -79,15 +82,22 @@ void RequestCoordinator::RequestPicked(const SavePageRequest& request) {
 }
 
 void RequestCoordinator::RequestQueueEmpty() {
-  // TODO(petewil): return to the BackgroundScheduler by calling
-  // ProcessingDoneCallback
+  // Return control to the scheduler when there is no more to do.
+  scheduler_callback_.Run(true);

[dougarnett, 2016/06/14 18:12:52]

unschedule() here too?

[Pete Williamson, 2016/06/14 21:03:14]

Actually, this is the main place to add unschedule.  Added (now that unschedule
has landed).

 }
 
 bool RequestCoordinator::StartProcessing(
-    const base::Callback<void(bool)>& callback) {
+    const base::Callback<void(bool)> callback) {

[dougarnett, 2016/06/14 18:12:52]

couldn't this still be a reference here if copying below anyway?

[Pete Williamson, 2016/06/14 21:03:13]

Done.

+  scheduler_callback_ = callback;
   // TODO(petewil): Check existing conditions (should be passed down from
   // BackgroundTask)
 
+  TryNextRequest();
+
+  return false;

[dougarnett, 2016/06/14 18:12:52]

should return true if caller should expect a callback, right? 

Consider TODO to check if already running, then return false. We might also
return false for above TODO if we can do a synchronous check on device
conditions.

[Pete Williamson, 2016/06/14 21:03:13]

Done.

+}
+
+void RequestCoordinator::TryNextRequest() {
   // Choose a request to process that meets the available conditions.
   // This is an async call, and returns right away.
   picker_->ChooseNextRequest(
@@ -95,7 +105,6 @@ bool RequestCoordinator::StartProcessing(
                  weak_ptr_factory_.GetWeakPtr()),
       base::Bind(&RequestCoordinator::RequestQueueEmpty,
                  weak_ptr_factory_.GetWeakPtr()));
-  return false;
 }
 
 void RequestCoordinator::StopProcessing() {
@@ -124,10 +133,19 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
            << __FUNCTION__;
   last_offlining_status_ = status;
 
-  // TODO(petewil): Check time budget.  Start a request if we have time, return
-  // to the scheduler if we are out of time.
-
   // TODO(petewil): If the request succeeded, remove it from the Queue.
+  if (status == Offliner::RequestStatus::SAVED) {
+    queue_->RemoveRequest(request.request_id(),
+                          base::Bind(&RequestCoordinator::UpdateRequestCallback,
+                                     weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  // TODO(petewil): Check time budget. Return to the scheduler if we are out.
+
+
+  // Start a request if we have time.
+  TryNextRequest();
+
 }
 
 }  // namespace offline_pages