[Offline pages] Removing active_request_ from request coordinator

chrome/browser/android/offline_pages/background_loader_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/background_loader_offliner.h"
 
+#include "base/bind.h"
 #include "base/json/json_writer.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sys_info.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
 #include "chrome/browser/android/offline_pages/offliner_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/offline_pages/core/background/offliner_policy.h"
 #include "components/offline_pages/core/background/save_page_request.h"
 #include "components/offline_pages/core/client_namespace_constants.h"
 #include "components/offline_pages/core/offline_page_feature.h"
 #include "components/offline_pages/core/offline_page_model.h"
 #include "content/public/browser/browser_context.h"
@@ skipping 45 matching lines @@
   adjusted_histogram_name += "." + client_id.name_space;
   return adjusted_histogram_name;
 }
 
 void RecordErrorCauseUMA(const ClientId& client_id, net::Error error_code) {
   UMA_HISTOGRAM_SPARSE_SLOWLY(
       AddHistogramSuffix(client_id,
                          "OfflinePages.Background.BackgroundLoadingFailedCode"),
       std::abs(error_code));
 }
+
+void HandleApplicationStateChangeCancel(
+    const Offliner::CompletionCallback& completion_callback,
+    const SavePageRequest& canceled_request) {
+  completion_callback.Run(canceled_request,
+                          Offliner::RequestStatus::FOREGROUND_CANCELED);
+}
 }  // namespace
 
 BackgroundLoaderOffliner::BackgroundLoaderOffliner(
     content::BrowserContext* browser_context,
     const OfflinerPolicy* policy,
     OfflinePageModel* offline_page_model)
     : browser_context_(browser_context),
       offline_page_model_(offline_page_model),
       policy_(policy),
       is_low_end_device_(base::SysInfo::IsLowEndDevice()),
@@ skipping 97 matching lines @@
   // Load page attempt.
   loader_.get()->LoadPage(request.url());
 
   snapshot_controller_.reset(
       new SnapshotController(base::ThreadTaskRunnerHandle::Get(), this,
                              kOfflineDomContentLoadedMs, page_delay_ms_));
 
   return true;
 }
 
-void BackgroundLoaderOffliner::Cancel(const CancelCallback& callback) {
+bool BackgroundLoaderOffliner::Cancel(const CancelCallback& callback) {
+  DCHECK(pending_request_);
+  // We ignore the case where pending_request_ is not set, but given the checks
+  // in RequestCoordinator this should not happen.
+  if (!pending_request_)
+    return false;
+
   // TODO(chili): We are not able to cancel a pending
   // OfflinePageModel::SaveSnapshot() operation. We will notify caller that
   // cancel completed once the SavePage operation returns.
-  if (!pending_request_) {
-    callback.Run(0LL);
-    return;
-  }
-
   if (save_state_ != NONE) {
     save_state_ = DELETE_AFTER_SAVE;
     cancel_callback_ = callback;
-    return;
+    return true;
   }
 
-  int64_t request_id = pending_request_->request_id();
+  // Post the cancel callback right after this call concludes.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(callback, *pending_request_.get()));
   ResetState();
-  callback.Run(request_id);
+  return true;
 }
 
-bool BackgroundLoaderOffliner::HandleTimeout(const SavePageRequest& request) {
+bool BackgroundLoaderOffliner::HandleTimeout(int64_t request_id) {
   if (pending_request_) {
-    DCHECK(request.request_id() == pending_request_->request_id());
-    if (is_low_bar_met_ &&
-        (request.started_attempt_count() + 1 >= policy_->GetMaxStartedTries() ||
-         request.completed_attempt_count() + 1 >=
-             policy_->GetMaxCompletedTries())) {
+    DCHECK(request_id == pending_request_->request_id());
+    if (is_low_bar_met_ && (pending_request_->started_attempt_count() + 1 >=
+                                policy_->GetMaxStartedTries() ||
+                            pending_request_->completed_attempt_count() + 1 >=
+                                policy_->GetMaxCompletedTries())) {
       // If we are already in the middle of a save operation, let it finish
       // but do not return SAVED_ON_LAST_RETRY
       if (save_state_ == NONE) {
         did_snapshot_on_last_retry_ = true;
         StartSnapshot();
       }
       return true;
     }
   }
   return false;
@@ skipping 166 matching lines @@
                                            int64_t offline_id) {
   if (!pending_request_)
     return;
 
   SavePageRequest request(*pending_request_.get());
   bool did_snapshot_on_last_retry = did_snapshot_on_last_retry_;
   ResetState();
 
   if (save_state_ == DELETE_AFTER_SAVE) {
     save_state_ = NONE;
-    cancel_callback_.Run(request.request_id());
+    cancel_callback_.Run(request);
     return;
   }
 
   save_state_ = NONE;
 
   Offliner::RequestStatus save_status;
   if (save_result == SavePageResult::SUCCESS) {
     if (did_snapshot_on_last_retry)
       save_status = RequestStatus::SAVED_ON_LAST_RETRY;
     else
@@ skipping 23 matching lines @@
   content::WebContentsObserver::Observe(contents);
   OfflinerData::AddToWebContents(contents, this);
 }
 
 void BackgroundLoaderOffliner::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(
-        &BackgroundLoaderOffliner::HandleApplicationStateChangeCancel,
-        weak_ptr_factory_.GetWeakPtr(), *request));
+    // No need to check the return value or complete early, as false would
+    // indicate that there was no request, in which case the state change is
+    // ignored.
+    Cancel(
+        base::Bind(HandleApplicationStateChangeCancel, completion_callback_));
   }
 }
 
-void BackgroundLoaderOffliner::HandleApplicationStateChangeCancel(
-    const SavePageRequest& request,
-    int64_t offline_id) {
-  // If for some reason the request was reset during while waiting for callback
-  // ignore the completion callback.
-  if (pending_request_ && pending_request_->request_id() != offline_id)
-    return;
-  completion_callback_.Run(request, RequestStatus::FOREGROUND_CANCELED);
-}
-
 void BackgroundLoaderOffliner::AddLoadingSignal(const char* signal_name) {
   base::TimeTicks current_time = base::TimeTicks::Now();
   base::TimeDelta delay_so_far = current_time - load_start_time_;
   // We would prefer to use int64_t here, but JSON does not support that type.
   // Given the choice between int and double, we choose to implicitly convert to
   // a double since it maintains more precision (we can get a longer time in
   // milliseconds than we can with a 2 bit int, 53 bits vs 32).
   double delay = delay_so_far.InMilliseconds();
   signal_data_.SetDouble(signal_name, delay);
 }
 
 }  // namespace offline_pages
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(offline_pages::OfflinerData);