Cancel prerendering when a request is paused or removed.

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 88ebb6092d655506a1fd9a701bb813f1639ce229..542fab31d9bbfe5b3593bc4fbfc40e8649646764 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -112,8 +112,24 @@ void RequestCoordinator::GetQueuedRequestsCallback(
   callback.Run(requests);
 }
 
+void RequestCoordinator::CancelActiveRequestIfItMatches(

[dougarnett, 2016/08/23 16:32:13]

CancelRequestIfActive()?  a bit shorter

[Pete Williamson, 2016/08/23 17:48:43]

That's shorter, but not really accurate, since we have a list of requests.  How
about CancelActiveRequestIfNeeded?

[dougarnett, 2016/08/23 20:59:44]

ugh, seems no more accurate than mine. I guess stick with what you have - I am
reading it differently now and makes more sense to me. ;-)

+    const std::vector<int64_t>& request_ids) {
+  // If we have a request in progress and need to cancel it, call the
+  // pre-renderer to cancel.  TODO Make sure we remove any page created by the
+  // prerenderer if it doesn't get the cancel in time.
+  if (active_request_ != nullptr) {
+    if (request_ids.end() != std::find(request_ids.begin(), request_ids.end(),
+                                       active_request_->request_id())) {
+      offliner_->Cancel();

[dougarnett, 2016/08/23 16:32:13]

consider calling StopProcessing() instead or check its logic to see what else we
should be doing here if we call offliner directly. Actually, we may want this
and StopProcessing to both call a helper method to take care of bookkeeping
(last_offlining_status, is_canceled, active_request) but might support both
stopping and just canceling current and trying next one.

[Pete Williamson, 2016/08/23 17:48:43]

Added StopPrerendering() to handle common tasks.

+      // When we get a callback from the offliner, we will call
+      // TryNextRequest(); even though return value is PRERENDER_CANCELLED.

[dougarnett, 2016/08/23 16:32:13]

hmm, the Offliner interface says it will not call the completion callback so we
should discuss that. But if it does call it, then we may want it to return
REQUEST_COORDINATOR_CANCELED

[Pete Williamson, 2016/08/23 17:48:43]

Ah, I had forgotten that.  I don't need to modify the offliner, I'll just keep
track of the fact that I need to call TryNextRequest(), and call it after
removing/pausing.

+    }
+  }
+}
+
 void RequestCoordinator::RemoveRequests(
     const std::vector<int64_t>& request_ids) {
+  CancelActiveRequestIfItMatches(request_ids);
   queue_->RemoveRequests(request_ids,
                          base::Bind(&RequestCoordinator::RemoveRequestsCallback,
                                     weak_ptr_factory_.GetWeakPtr()));
@@ -121,6 +137,7 @@ void RequestCoordinator::RemoveRequests(
 
 void RequestCoordinator::PauseRequests(
     const std::vector<int64_t>& request_ids) {
+  CancelActiveRequestIfItMatches(request_ids);
   queue_->ChangeRequestsState(
       request_ids, SavePageRequest::RequestState::PAUSED,
       base::Bind(&RequestCoordinator::UpdateMultipleRequestsCallback,
@@ -318,6 +335,19 @@ void RequestCoordinator::OfflinerDoneCallback(const SavePageRequest& request,
                                      updated_request.client_id()));
     NotifyCompleted(updated_request, SavePageStatus::FOREGROUND_CANCELED);
 
+    // TODO: Will this be PRERENDERING_CANCELED or REQUEST_COORDINATOR_CANCELED

[dougarnett, 2016/08/23 16:32:13]

as noted above, I wasn't expecting there to be a callback currently (so we need
to discuss if there does need to be one)

[Pete Williamson, 2016/08/23 17:48:43]

We don't need one, this change reverted.

+    // for a call to
+    // offliner_->Cancel()?
+  } else if (status == Offliner::RequestStatus::PRERENDERING_CANCELED) {
+    // If the user called cancel, the request never had a chance to complete, so
+    // mark the attempt as aborted, and it may be eligible for retrying later
+    // (for instance, if it is later unpaused).
+    SavePageRequest updated_request(request);
+    updated_request.MarkAttemptAborted();
+    queue_->UpdateRequest(updated_request,
+                          base::Bind(&RequestCoordinator::UpdateRequestCallback,
+                                     weak_ptr_factory_.GetWeakPtr(),
+                                     updated_request.client_id()));
   } else if (status == Offliner::RequestStatus::SAVED) {
     // Remove the request from the queue if it succeeded.
     std::vector<int64_t> remove_requests;