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

components/offline_pages/background/request_coordinator.cc

 // 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.
 
 #include "components/offline_pages/background/request_coordinator.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 50 matching lines @@
 void RequestCoordinator::AddRequestResultCallback(
     RequestQueue::AddRequestResult result,
     const SavePageRequest& request) {
 
   // Inform the scheduler that we have an outstanding task.
   // TODO(petewil): Define proper TriggerConditions and set them.
   Scheduler::TriggerCondition conditions;
   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;
 
   // Ensure that the scheduler has actively scheduled a task.
   scheduler_->Schedule(conditions);
 
   // Send the request on to the offliner.
   SendRequestToOffliner(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(
       base::Bind(&RequestCoordinator::RequestPicked,
                  weak_ptr_factory_.GetWeakPtr()),
       base::Bind(&RequestCoordinator::RequestQueueEmpty,
                  weak_ptr_factory_.GetWeakPtr()));
-  return false;
 }
 
 void RequestCoordinator::StopProcessing() {
 }
 
 void RequestCoordinator::SendRequestToOffliner(const SavePageRequest& request) {
   // TODO(petewil): Ensure only one offliner at a time is used.
   // TODO(petewil): When we have multiple offliners, we need to pick one.
   Offliner* offliner = factory_->GetOffliner(policy_.get());
   if (!offliner) {
     DVLOG(0) << "Unable to create Offliner. "
              << "Cannot background offline page.";
     return;
   }
 
   // Start the load and save process in the offliner (Async).
   offliner->LoadAndSave(request,
                         base::Bind(&RequestCoordinator::OfflinerDoneCallback,
                                    weak_ptr_factory_.GetWeakPtr()));
 }
 
 void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
                                               Offliner::RequestStatus status) {
   DVLOG(2) << "offliner finished, saved: "
            << (status == Offliner::RequestStatus::SAVED) << ", "
            << __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): If the request succeeded, remove it from the Queue.
+  // TODO(petewil): Check time budget. Return to the scheduler if we are out.
+
+
+  // Start a request if we have time.
+  TryNextRequest();
+
 }
 
 }  // namespace offline_pages