[Offline pages] Removing active_request_ from request coordinator

chrome/browser/android/offline_pages/prerendering_offliner.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 "chrome/browser/android/offline_pages/prerendering_offliner.h"
 
 #include "base/bind.h"
 #include "base/json/json_writer.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sys_info.h"
@@ skipping 237 matching lines @@
     // Create app listener for the pending request.
     app_listener_.reset(new base::android::ApplicationStatusListener(
         base::Bind(&PrerenderingOffliner::OnApplicationStateChange,
                    weak_ptr_factory_.GetWeakPtr())));
   }
 
   return accepted;
 }
 
 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();
       return true;
     }
   }
   return false;
 }
 
 void PrerenderingOffliner::SetLoaderForTesting(
@@ skipping 26 matching lines @@
   }
   return loader_.get();
 }
 
 void PrerenderingOffliner::OnApplicationStateChange(
     base::android::ApplicationState application_state) {
   if (pending_request_ && is_low_end_device_ &&
       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