[Offline pages] Removing active_request_ from request coordinator

chrome/browser/android/offline_pages/prerendering_offliner.cc

Index: chrome/browser/android/offline_pages/prerendering_offliner.cc
diff --git a/chrome/browser/android/offline_pages/prerendering_offliner.cc b/chrome/browser/android/offline_pages/prerendering_offliner.cc
index 899bc1070eb66a479878c82f7a481f5d586dbec4..f2c29a0aee39318f9fa5395cc93465865229f3ce 100644
--- a/chrome/browser/android/offline_pages/prerendering_offliner.cc
+++ b/chrome/browser/android/offline_pages/prerendering_offliner.cc
@@ -255,23 +255,23 @@ bool PrerenderingOffliner::LoadAndSave(
 }
 
 void PrerenderingOffliner::Cancel(const CancelCallback& callback) {
-  int64_t request_id = 0LL;
-  if (pending_request_) {
-    request_id = pending_request_->request_id();
-    pending_request_.reset(nullptr);
-    app_listener_.reset(nullptr);
-    GetOrCreateLoader()->StopLoading();
-    // TODO(dougarnett): Consider ability to cancel SavePage request.
-  }
-  callback.Run(request_id);
+  DCHECK(pending_request_);
+  if (!pending_request_)

[Pete Williamson, 2017/04/24 17:41:38]

Shouldn't we be running the callback even if there is no pending request?

[fgorski, 2017/04/24 21:40:58]

Handled by signature change as explained earlier.

+    return;
+  SavePageRequest canceled_request(*pending_request_.get());
+  pending_request_.reset(nullptr);
+  app_listener_.reset(nullptr);
+  GetOrCreateLoader()->StopLoading();
+  callback.Run(canceled_request);
 }
 
-bool PrerenderingOffliner::HandleTimeout(const SavePageRequest& request) {
+bool PrerenderingOffliner::HandleTimeout(int64_t request_id) {
   if (pending_request_) {
-    DCHECK(request.request_id() == pending_request_->request_id());
+    DCHECK(request_id == pending_request_->request_id());
     if (GetOrCreateLoader()->IsLowbarMet() &&
-        (request.started_attempt_count() + 1 >= policy_->GetMaxStartedTries() ||
-         request.completed_attempt_count() + 1 >=
+        (pending_request_->started_attempt_count() + 1 >=
+             policy_->GetMaxStartedTries() ||
+         pending_request_->completed_attempt_count() + 1 >=
              policy_->GetMaxCompletedTries())) {
       saved_on_last_retry_ = true;
       GetOrCreateLoader()->StartSnapshot();
@@ -318,21 +318,20 @@ void PrerenderingOffliner::OnApplicationStateChange(
       application_state ==
           base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) {
     DVLOG(1) << "App became active, canceling current offlining request";
-    SavePageRequest* request = pending_request_.get();
-    // This works because Bind will make a copy of request, and we
-    // should not have to worry about reset being called before cancel callback.
     Cancel(base::Bind(&PrerenderingOffliner::HandleApplicationStateChangeCancel,
-                      weak_ptr_factory_.GetWeakPtr(), *request));
+                      weak_ptr_factory_.GetWeakPtr()));

[Pete Williamson, 2017/04/24 17:41:38]

If we don't pass the pending request here, how is the input parameter for
HandleApplicationStateChangeCancel getting set?  I missed it somewhere.

[chili, 2017/04/24 17:44:53]

I think it's simply passing in the pending_request_

[fgorski, 2017/04/24 21:40:58]

Cathy is correct here, the value comes from Cancel and is captured prior
to resetting it.
I am doing the same fix as I did in BLO. 

   }
 }
 
 void PrerenderingOffliner::HandleApplicationStateChangeCancel(
-    const SavePageRequest& request,
-    int64_t offline_id) {
+    const SavePageRequest& canceled_request) {
   // This shouldn't be immediate, but account for case where request was reset
   // while waiting for callback.
-  if (pending_request_ && pending_request_->request_id() != offline_id)
+  if (pending_request_ &&
+      pending_request_->request_id() != canceled_request.request_id()) {
     return;
-  completion_callback_.Run(request, RequestStatus::FOREGROUND_CANCELED);
+  }
+  completion_callback_.Run(canceled_request,
+                           RequestStatus::FOREGROUND_CANCELED);
 }
 }  // namespace offline_pages