Cancel prerendering when a request is paused or removed.

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 94 matching lines @@
                                  weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
 void RequestCoordinator::GetQueuedRequestsCallback(
     const GetRequestsCallback& callback,
     RequestQueue::GetRequestsResult result,
     const std::vector<SavePageRequest>& requests) {
   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()));
 }
 
 void RequestCoordinator::PauseRequests(
     const std::vector<int64_t>& request_ids) {
+  CancelActiveRequestIfItMatches(request_ids);
   queue_->ChangeRequestsState(
       request_ids, SavePageRequest::RequestState::PAUSED,
       base::Bind(&RequestCoordinator::UpdateMultipleRequestsCallback,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void RequestCoordinator::ResumeRequests(
     const std::vector<int64_t>& request_ids) {
   queue_->ChangeRequestsState(
       request_ids, SavePageRequest::RequestState::AVAILABLE,
@@ skipping 123 matching lines @@
 
   GetOffliner();
   if (!offliner_) {
     DVLOG(0) << "Unable to create Offliner. "
              << "Cannot background offline page.";
     return;
   }
 
   DCHECK(!is_busy_);
   is_busy_ = true;
   active_request_.reset(new SavePageRequest(request));

[dougarnett, 2016/08/23 17:00:39]

We may want to move this down and set with updated_request which has been
MarkAttemptStarted

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

Done.

[dougarnett, 2016/08/23 17:51:30]

not the current problem though, as the MarkAbortAttempted is not currently
performed on the active_request_ (done the request passed into the callback). 

 
   // Prepare an updated request to attempt.
   SavePageRequest updated_request(request);
   updated_request.MarkAttemptStarted(base::Time::Now());
 
   // Start the load and save process in the offliner (Async).
   if (offliner_->LoadAndSave(
           updated_request, base::Bind(&RequestCoordinator::OfflinerDoneCallback,
                                       weak_ptr_factory_.GetWeakPtr()))) {
     // Offliner accepted request so update it in the queue.
@@ skipping 33 matching lines @@
     // TODO(dougarnett): See if we can conclusively identify other attempt
     // aborted cases to treat this way (eg, for Render Process Killed).
     SavePageRequest updated_request(request);
     updated_request.MarkAttemptAborted();
     queue_->UpdateRequest(updated_request,
                           base::Bind(&RequestCoordinator::UpdateRequestCallback,
                                      weak_ptr_factory_.GetWeakPtr(),
                                      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;
     remove_requests.push_back(request.request_id());
     queue_->RemoveRequests(
         remove_requests, base::Bind(&RequestCoordinator::RemoveRequestsCallback,
                                     weak_ptr_factory_.GetWeakPtr()));
     NotifyCompleted(request, SavePageStatus::SUCCESS);
   } else if (request.completed_attempt_count() + 1 >=
              policy_->GetMaxCompletedTries()) {
@@ skipping 74 matching lines @@
   FOR_EACH_OBSERVER(Observer, observers_, OnChanged(request));
 }
 
 void RequestCoordinator::GetOffliner() {
   if (!offliner_) {
     offliner_ = factory_->GetOffliner(policy_.get());
   }
 }
 
 }  // namespace offline_pages