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

chrome/browser/android/offline_pages/background_loader_offliner.cc

Index: chrome/browser/android/offline_pages/background_loader_offliner.cc
diff --git a/chrome/browser/android/offline_pages/background_loader_offliner.cc b/chrome/browser/android/offline_pages/background_loader_offliner.cc
index 732a4eca3a09ec77bba2bbc4366e30faed6f2413..1bdd2410da38929de7c40758514cbf6b7e5cfe34 100644
--- a/chrome/browser/android/offline_pages/background_loader_offliner.cc
+++ b/chrome/browser/android/offline_pages/background_loader_offliner.cc
@@ -109,18 +109,23 @@ bool BackgroundLoaderOffliner::LoadAndSave(const SavePageRequest& request,
   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() {
@@ -236,6 +241,7 @@ void BackgroundLoaderOffliner::OnPageSaved(SavePageResult save_result,
 
   if (save_state_ == DELETE_AFTER_SAVE) {
     save_state_ = NONE;
+    cancel_callback_.Run(request.request_id());
     return;
   }
 
@@ -270,9 +276,15 @@ void BackgroundLoaderOffliner::OnApplicationStateChange(
           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