PrerenderingOffliner will abort background load if it sees chrome transitioning to foreground on lo…

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 cf8625f981e0317f05337ac6c03c83c4addc7a84..fda2144b0abc1c4c6eade796fece33abf0f792cd 100644
--- a/chrome/browser/android/offline_pages/prerendering_offliner.cc
+++ b/chrome/browser/android/offline_pages/prerendering_offliner.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/android/offline_pages/prerendering_offliner.h"
 
 #include "base/bind.h"
+#include "base/sys_info.h"
 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
 #include "components/offline_pages/background/save_page_request.h"
 #include "components/offline_pages/offline_page_model.h"
@@ -20,13 +21,13 @@ PrerenderingOffliner::PrerenderingOffliner(
     : browser_context_(browser_context),
       offline_page_model_(offline_page_model),
       pending_request_(nullptr),
+      app_listener_(nullptr),
       weak_ptr_factory_(this) {}
 
 PrerenderingOffliner::~PrerenderingOffliner() {}
 
 void PrerenderingOffliner::OnLoadPageDone(
     const SavePageRequest& request,
-    const CompletionCallback& completion_callback,
     Offliner::RequestStatus load_status,
     content::WebContents* web_contents) {
   // Check if request is still pending receiving a callback.
@@ -61,25 +62,24 @@ void PrerenderingOffliner::OnLoadPageDone(
     SavePage(web_contents->GetLastCommittedURL(), request.client_id(),
              std::move(archiver),
              base::Bind(&PrerenderingOffliner::OnSavePageDone,
-                        weak_ptr_factory_.GetWeakPtr(), request,
-                        completion_callback));
+                        weak_ptr_factory_.GetWeakPtr(), request));
   } else {
-    // Clear pending request and then run the completion callback.
+    // Clear pending request and then run its completion callback.
     pending_request_.reset(nullptr);
-    completion_callback.Run(request, load_status);
+    app_listener_.reset(nullptr);
+    completion_callback_.Run(request, load_status);
   }
 }
 
 void PrerenderingOffliner::OnSavePageDone(
     const SavePageRequest& request,
-    const CompletionCallback& completion_callback,
     SavePageResult save_result,
     int64_t offline_id) {
   // Check if request is still pending receiving a callback.
   if (!pending_request_)
     return;
 
-  // Also check that save callback is for same request as pending request
+  // Also check that this completed request is same as the pending one
   // (since SavePage request is not cancel-able currently and could be old).
   if (request.request_id() != pending_request_->request_id()) {
     DVLOG(1) << "Ignoring save callback for old request";
@@ -88,6 +88,7 @@ void PrerenderingOffliner::OnSavePageDone(
 
   // Clear pending request here and inform loader we are done with WebContents.
   pending_request_.reset(nullptr);
+  app_listener_.reset(nullptr);

[Pete Williamson, 2016/07/13 23:17:55]

Why do we reset for both SavePage done and LoadPageDone?  Is it because we don't
always get LoadPageDone?

[dougarnett, 2016/07/13 23:44:20]

Paired with clearing pending_request_. Only reset in LoadPageDone for the case
that the load did not succeed and there will be no SavePage attempt.

   GetOrCreateLoader()->StopLoading();
 
   // Determine status and run the completion callback.
@@ -99,7 +100,7 @@ void PrerenderingOffliner::OnSavePageDone(
     // request based on specific save error cases.
     save_status = RequestStatus::SAVE_FAILED;
   }
-  completion_callback.Run(request, save_status);
+  completion_callback_.Run(request, save_status);
 }
 
 bool PrerenderingOffliner::LoadAndSave(const SavePageRequest& request,
@@ -121,14 +122,19 @@ bool PrerenderingOffliner::LoadAndSave(const SavePageRequest& request,
 
   // Track copy of pending request for callback handling.
   pending_request_.reset(new SavePageRequest(request));
+  completion_callback_ = callback;
 
   // Kick off load page attempt.
   bool accepted = GetOrCreateLoader()->LoadPage(
-      request.url(),
-      base::Bind(&PrerenderingOffliner::OnLoadPageDone,
-                 weak_ptr_factory_.GetWeakPtr(), request, callback));
-  if (!accepted)
+      request.url(), base::Bind(&PrerenderingOffliner::OnLoadPageDone,
+                                weak_ptr_factory_.GetWeakPtr(), request));
+  if (!accepted) {
     pending_request_.reset(nullptr);
+  } else if (base::SysInfo::IsLowEndDevice()) {
+    app_listener_.reset(new base::android::ApplicationStatusListener(

[Pete Williamson, 2016/07/13 23:17:55]

Would it be better to always set up a listener, even on high end devices?  I can
see us wanting to use several signals, and even cancel background loads on high
end devices sometimes.

[dougarnett, 2016/07/13 23:44:20]

Had that originally but didn't come up with any use yet in the callback so
expected I would get the opposite feedback ;-)  I don't have strong opinion
either way.

[Pete Williamson, 2016/07/13 23:57:12]

I suppose I would be OK with waiting until we need to use the signal before
setting up the app status listener every time. (IE, the code is OK as is).

[fgorski, 2016/07/14 17:01:45]

I second Pete's initial comment.
It is the state change listener that should make checks for low end device or do
any other interpretation of the state change information.

This code does not need to be conditional on device knowledge.

[dougarnett, 2016/07/15 16:00:25]

Done.

+        base::Bind(&PrerenderingOffliner::OnApplicationStateChange,
+                   weak_ptr_factory_.GetWeakPtr())));
+  }
 
   return accepted;
 }
@@ -136,6 +142,7 @@ bool PrerenderingOffliner::LoadAndSave(const SavePageRequest& request,
 void PrerenderingOffliner::Cancel() {
   if (pending_request_) {
     pending_request_.reset(nullptr);
+    app_listener_.reset(nullptr);
     GetOrCreateLoader()->StopLoading();
     // TODO(dougarnett): Consider ability to cancel SavePage request.
   }
@@ -147,13 +154,19 @@ void PrerenderingOffliner::SetLoaderForTesting(
   loader_ = std::move(loader);
 }
 
+void PrerenderingOffliner::SetApplicationStateForTesting(
+    base::android::ApplicationState application_state) {
+  OnApplicationStateChange(application_state);
+}
+
 void PrerenderingOffliner::SavePage(
     const GURL& url,
     const ClientId& client_id,
     std::unique_ptr<OfflinePageArchiver> archiver,
-    const SavePageCallback& callback) {
+    const SavePageCallback& save_callback) {
   DCHECK(offline_page_model_);
-  offline_page_model_->SavePage(url, client_id, std::move(archiver), callback);
+  offline_page_model_->SavePage(url, client_id, std::move(archiver),
+                                save_callback);
 }
 
 PrerenderingLoader* PrerenderingOffliner::GetOrCreateLoader() {
@@ -163,4 +176,17 @@ PrerenderingLoader* PrerenderingOffliner::GetOrCreateLoader() {
   return loader_.get();
 }
 
+void PrerenderingOffliner::OnApplicationStateChange(
+    base::android::ApplicationState application_state) {
+  if (pending_request_ && base::SysInfo::IsLowEndDevice() &&
+      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,
+                             Offliner::RequestStatus::FOREGROUND_CANCELED);
+  }
+}
+
 }  // namespace offline_pages