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

When offline page requests are completed, remove them from the queue of outstanding requests, and start the next request (if any)

This changelist also adds a test for the new code.

BUG=610521

Review-Url: https://codereview.chromium.org/2064793003
Cr-Commit-Position: refs/heads/master@{#399914}

components/offline_pages/background/request_coordinator.cc

@@ -68,6 +68,11 @@ void RequestCoordinator::AddRequestResultCallback(
   scheduler_->Schedule(conditions);
 }
 
+// Called in response to updating a request in the request queue.
+void RequestCoordinator::UpdateRequestCallback(
+    RequestQueue::UpdateRequestResult result) {}
+
+// Called by the request picker when a request has been picked.
 void RequestCoordinator::RequestPicked(const SavePageRequest& request) {
   Scheduler::TriggerCondition conditions;
 
@@ -79,23 +84,35 @@ void RequestCoordinator::RequestPicked(const SavePageRequest& request) {
 }
 
 void RequestCoordinator::RequestQueueEmpty() {
-  // TODO(petewil): return to the BackgroundScheduler by calling
-  // ProcessingDoneCallback
+  // Clear the outstanding "safety" task in the scheduler.
+  scheduler_->Unschedule();
+  // Return control to the scheduler when there is no more to do.
+  scheduler_callback_.Run(true);
 }
 
 bool RequestCoordinator::StartProcessing(
     const base::Callback<void(bool)>& callback) {
+  scheduler_callback_ = callback;
   // TODO(petewil): Check existing conditions (should be passed down from
   // BackgroundTask)
 
+  TryNextRequest();
+
+  // TODO(petewil): Should return true if the caller should expect a
+  // callback.  Return false if there is already a request running.
+  // Probably best to do this when I prevent multiple instances from
+  // running at the same time.
+  return true;
+}
+
+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() {
@@ -124,10 +141,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

components/offline_pages/background/request_coordinator.h

@@ -55,6 +55,11 @@ class RequestCoordinator : public KeyedService {
   // is stopped or complete.
   void StopProcessing();
 
+  // A way for tests to set the callback in use when an operation is over.
+  void SetProcessingCallbackForTest(const base::Callback<void(bool)> callback) {
+    scheduler_callback_ = callback;
+  }
+
   // Returns the request queue used for requests.  Coordinator keeps ownership.
   RequestQueue* queue() { return queue_.get(); }
 
@@ -70,6 +75,8 @@ class RequestCoordinator : public KeyedService {
   void AddRequestResultCallback(RequestQueue::AddRequestResult result,
                                 const SavePageRequest& request);
 
+  void UpdateRequestCallback(RequestQueue::UpdateRequestResult result);
+
   // Callback from the request picker when it has chosen our next request.
   void RequestPicked(const SavePageRequest& request);
 
@@ -78,9 +85,15 @@ class RequestCoordinator : public KeyedService {
 
   void SendRequestToOffliner(const SavePageRequest& request);
 
+  // Called by the offliner when an offlining request is completed. (and by
+  // tests).
   void OfflinerDoneCallback(const SavePageRequest& request,
                             Offliner::RequestStatus status);
 
+  void TryNextRequest();
+
+  friend class RequestCoordinatorTest;
+
   // RequestCoordinator takes over ownership of the policy
   std::unique_ptr<OfflinerPolicy> policy_;
   // OfflinerFactory.  Used to create offline pages. Owned.
@@ -93,6 +106,8 @@ class RequestCoordinator : public KeyedService {
   Offliner::RequestStatus last_offlining_status_;
   // Class to choose which request to schedule next
   std::unique_ptr<RequestPicker> picker_;
+  // Calling this returns to the scheduler across the JNI bridge.
+  base::Callback<void(bool)> scheduler_callback_;
   // Allows us to pass a weak pointer to callbacks.
   base::WeakPtrFactory<RequestCoordinator> weak_ptr_factory_;
 

components/offline_pages/background/request_coordinator_unittest.cc

@@ -27,6 +27,7 @@ namespace {
 // put test constants here
 const GURL kUrl("http://universe.com/everything");
 const ClientId kClientId("bookmark", "42");
+const int kRequestId(1);
 }  // namespace
 
 class SchedulerStub : public Scheduler {
@@ -94,10 +95,17 @@ class RequestCoordinatorTest
   void EmptyCallbackFunction(bool result) {
   }
 
+  // Callback for Add requests
+  void AddRequestDone(RequestQueue::AddRequestResult result,
+                      const SavePageRequest& request);
+
   // Callback for getting requests.
   void GetRequestsDone(RequestQueue::GetRequestsResult result,
                        const std::vector<SavePageRequest>& requests);
 
+  void SendOfflinerDoneCallback(const SavePageRequest& request,
+                                Offliner::RequestStatus status);
+
   RequestQueue::GetRequestsResult last_get_requests_result() const {
     return last_get_requests_result_;
   }
@@ -144,12 +152,24 @@ void RequestCoordinatorTest::GetRequestsDone(
   last_requests_ = requests;
 }
 
+
+void RequestCoordinatorTest::AddRequestDone(
+    RequestQueue::AddRequestResult result,
+    const SavePageRequest& request) {}
+
+void RequestCoordinatorTest::SendOfflinerDoneCallback(
+    const SavePageRequest& request, Offliner::RequestStatus status) {
+  // Using the fact that the test class is a friend, call to the callback
+  coordinator_->OfflinerDoneCallback(request, status);
+}
+
+
 TEST_F(RequestCoordinatorTest, StartProcessingWithNoRequests) {
   base::Callback<void(bool)> callback =
       base::Bind(
           &RequestCoordinatorTest::EmptyCallbackFunction,
           base::Unretained(this));
-  EXPECT_FALSE(coordinator()->StartProcessing(callback));
+  EXPECT_TRUE(coordinator()->StartProcessing(callback));
 }
 
 TEST_F(RequestCoordinatorTest, SavePageLater) {
@@ -174,4 +194,39 @@ TEST_F(RequestCoordinatorTest, SavePageLater) {
   EXPECT_TRUE(scheduler_stub->schedule_called());
 }
 
+TEST_F(RequestCoordinatorTest, OfflinerDone) {
+
+  // Add a request to the queue, wait for callbacks to finish.
+  offline_pages::SavePageRequest request(
+      kRequestId, kUrl, kClientId, base::Time::Now());
+  coordinator()->queue()->AddRequest(
+      request,
+      base::Bind(&RequestCoordinatorTest::AddRequestDone,
+                 base::Unretained(this)));
+  PumpLoop();
+
+  // We need to give a callback to the request.
+  base::Callback<void(bool)> callback =
+      base::Bind(
+          &RequestCoordinatorTest::EmptyCallbackFunction,
+          base::Unretained(this));
+  coordinator()->SetProcessingCallbackForTest(callback);
+
+  // Call the OfflinerDoneCallback to simulate the page being completed, wait
+  // for callbacks.
+  SendOfflinerDoneCallback(request, Offliner::RequestStatus::SAVED);
+  PumpLoop();
+
+  // Verify the request gets removed from the queue, and wait for callbacks.
+  coordinator()->queue()->GetRequests(
+      base::Bind(&RequestCoordinatorTest::GetRequestsDone,
+                 base::Unretained(this)));
+  PumpLoop();
+
+  // We should not find any requests in the queue anymore.
+  // RequestPicker should *not* have tried to start an additional job,
+  // because the request queue is empty now.
+  EXPECT_EQ(0UL, last_requests().size());
+}
+
 }  // namespace offline_pages