[Offline Pages] Turn Offliner::Cancel into an async operation to resolve conflicting assumptions

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/metrics/histogram_macros.h"
 #include "base/sys_info.h"
 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
 #include "chrome/browser/android/offline_pages/offliner_helper.h"
@@ skipping 91 matching lines @@
   app_listener_.reset(new base::android::ApplicationStatusListener(
       base::Bind(&BackgroundLoaderOffliner::OnApplicationStateChange,
                  weak_ptr_factory_.GetWeakPtr())));
 
   // Load page attempt.
   loader_.get()->LoadPage(request.url());
 
   return true;
 }
 
-void BackgroundLoaderOffliner::Cancel() {
+void BackgroundLoaderOffliner::Cancel(const CancelCallback& callback) {
   // TODO(chili): We are not able to cancel a pending
   // OfflinePageModel::SavePage() operation. We just ignore the callback.

[fgorski, 2017/02/27 18:07:25]

Is this TODO valid after your change?

[chili, 2017/02/28 00:44:14]

TODO still somewhat valid, since this is simply getting around the problem.  We
don't need to cancel a pending OfflinePageModel::SavePage() operation, but if we
can, we should.

Updated comment.

-  if (!pending_request_)
+  if (!pending_request_) {
+    callback.Run(0L);
     return;
+  }
 
   if (save_state_ != NONE) {
     save_state_ = DELETE_AFTER_SAVE;
+    cancel_callback_ = callback;
     return;
   }
 
+  int64_t request_id = pending_request_->request_id();
   ResetState();
+  callback.Run(request_id);
 }
 
 void BackgroundLoaderOffliner::DidStopLoading() {
   if (!pending_request_.get()) {
     DVLOG(1) << "DidStopLoading called even though no pending request.";
     return;
   }
 
   SavePageRequest request(*pending_request_.get());
   // If there was an error navigating to page, return loading failed.
@@ skipping 95 matching lines @@
 void BackgroundLoaderOffliner::OnPageSaved(SavePageResult save_result,
                                            int64_t offline_id) {
   if (!pending_request_)
     return;
 
   SavePageRequest request(*pending_request_.get());
   ResetState();
 
   if (save_state_ == DELETE_AFTER_SAVE) {
     save_state_ = NONE;
+    cancel_callback_.Run(request.request_id());
     return;
   }
 
   save_state_ = NONE;
 
   Offliner::RequestStatus save_status;
   if (save_result == SavePageResult::SUCCESS)
     save_status = RequestStatus::SAVED;
   else
     save_status = RequestStatus::SAVE_FAILED;
@@ skipping 14 matching lines @@
   content::WebContentsObserver::Observe(loader_.get()->web_contents());
 }
 
 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();
-    Cancel();
-    completion_callback_.Run(*request, RequestStatus::FOREGROUND_CANCELED);
+    Cancel(base::Bind(

[fgorski, 2017/02/27 18:07:25]

Please document that it works, because Bind will make a copy of request and we
should not worry about reset being called before cancel callback is.

[chili, 2017/02/28 00:44:14]

Done.

+        &BackgroundLoaderOffliner::HandleApplicationStateChangeCancel,
+        weak_ptr_factory_.GetWeakPtr(), *request));
   }
 }
 
+void BackgroundLoaderOffliner::HandleApplicationStateChangeCancel(
+    const SavePageRequest& request,
+    int64_t offline_id) {

[fgorski, 2017/02/27 18:07:25]

I don't understand the signature to the cancel callback. Definitely a separate
CL if we want to fix it, but maybe a todo would make sense here.

Also, what if offline_id does not match the request id?

[chili, 2017/02/28 00:44:14]

Added a check.

The cancel callback was mainly for possible case of multiple requests at the
same time, so callback can update proper request. On second thought though, this
might be unnecessary.  I'll update in a separate CL.

+  completion_callback_.Run(request, RequestStatus::FOREGROUND_CANCELED);
+}
 }  // namespace offline_pages