[Offline pages] Create offliner that uses background loader

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
new file mode 100644
index 0000000000000000000000000000000000000000..c99d057557d1024ea833c8cfde857264c4a4cad9
--- /dev/null
+++ b/chrome/browser/android/offline_pages/background_loader_offliner.cc
@@ -0,0 +1,135 @@
+// 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/sys_info.h"
+#include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
+#include "components/offline_pages/core/background/save_page_request.h"
+#include "components/offline_pages/core/offline_page_model.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/web_contents.h"
+
+namespace offline_pages {
+
+BackgroundLoaderOffliner::BackgroundLoaderOffliner(
+    content::BrowserContext* browser_context,
+    const OfflinerPolicy* policy,
+    OfflinePageModel* offline_page_model)
+    : WebContentsObserver(),

[fgorski, 2016/12/12 17:59:16]

Is this line needed?

[chili, 2016/12/15 10:34:39]

Done.

+      browser_context_(browser_context),
+      offline_page_model_(offline_page_model),

[fgorski, 2016/12/12 17:59:16]

why do you need both browser_context_ and offline_page_model_? (latter can be
retrieved using the former)

Also, do you intend to DCHECK these?

[chili, 2016/12/15 10:34:39]

following example for the prerendering_offliner here.  Also lets us pass in a
mock/stub offlinepagemodel for testing here.

+      pending_request_(nullptr),

[fgorski, 2016/12/12 17:59:16]

not needed.

[chili, 2016/12/15 10:34:39]

Done.

+      app_listener_(nullptr),

[fgorski, 2016/12/12 17:59:16]

not needed.

[chili, 2016/12/15 10:34:39]

Done.

+      is_low_end_device_(base::SysInfo::IsLowEndDevice()),
+      weak_ptr_factory_(this) {}
+
+BackgroundLoaderOffliner::~BackgroundLoaderOffliner() {}
+
+bool BackgroundLoaderOffliner::LoadAndSave(const SavePageRequest& request,
+                                           const CompletionCallback& callback) {

[fgorski, 2016/12/12 17:59:16]

dcheck completion_callback_ please.

[chili, 2016/12/15 10:34:39]

Done.

+  DCHECK(!pending_request_.get());
+
+  if (pending_request_) {
+    DVLOG(1) << "Already have pending request";

[fgorski, 2016/12/12 17:59:16]

DCHECK above prevents this DVLOG

[chili, 2016/12/15 10:34:39]

Done.

+    return false;
+  }
+
+  if (!OfflinePageModel::CanSaveURL(request.url())) {
+    DVLOG(1) << "Not able to save page for requested url: " << request.url();
+    return false;
+  }
+
+  // Track copy of pending request.
+  pending_request_.reset(new SavePageRequest(request));
+  completion_callback_ = callback;
+
+  if (!loader_)
+    ResetState();
+
+  // Listen for app foreground/background change.
+  app_listener_.reset(new base::android::ApplicationStatusListener(
+      base::Bind(&BackgroundLoaderOffliner::OnApplicationStateChange,
+                 weak_ptr_factory_.GetWeakPtr())));
+
+  loader_.get()->LoadPage(request.url());
+
+  return true;
+}
+
+void BackgroundLoaderOffliner::Cancel() {
+  if (pending_request_) {
+    pending_request_.reset(nullptr);

[fgorski, 2016/12/12 17:59:16]

will using just: reset() work?

[dougarnett, 2016/12/12 19:24:17]

Interesting, I didn't know reset() was defined on unique_ptr. We have this in
many places (and many more in some other parts of chrome). Seems good to clean
these up.

[chili, 2016/12/15 10:34:39]

yes it does. I always thought we passed in nullptr in places to make it more
explicit... I should assume less :D

+    ResetState();

[fgorski, 2016/12/12 17:59:16]

This usage of ResetState contradicts line 49.

This will create a new loader_ is that what you intend to do here?
Wouldn't it be more appropriate to use ResetState to reset() all pointers
(pending_request_, app_listener_, loader_, ...) ?

[dougarnett, 2016/12/12 19:24:17]

Also consider comment/TODO about not currently able to cancel a pending
OfflinePageModel::SavePage() operation

[chili, 2016/12/15 10:34:39]

Line 49 is mainly there because I can't call ResetState from constructor if I
want to override it for test. Ideally, ResetState is not there at all, and
callbacks/cancel options will lead to the destruction of the object, hence I
wanted to limit the scope of ResetState to only the loader_ and resetting the
observer.

[chili, 2016/12/15 10:34:39]

Done.

+  }
+}
+
+void BackgroundLoaderOffliner::DidStopLoading() {
+  if (!pending_request_.get()) {

[fgorski, 2016/12/12 17:59:16]

Is there a chance Cancel and DidStopLoading are queued in message loop one after
another?

[chili, 2016/12/15 10:34:39]

I don't think so. The DidStopLoading() is called synchonously by the
WebContentsDelegate when it receives a message about loading. If Cancel() is
called first, it would de-register the observer from the WCD, and WCD will
therefore not end up calling this method when it cycles through its list of
observers.

+    DVLOG(1) << "DidStopLoading called even though no pending request.";
+    return;
+  }
+
+  SavePageRequest request(*pending_request_.get());
+  content::WebContents* web_contents(
+      content::WebContentsObserver::web_contents());
+
+  std::unique_ptr<OfflinePageArchiver> archiver(
+      new OfflinePageMHTMLArchiver(web_contents));
+
+  DCHECK(offline_page_model_);

[fgorski, 2016/12/12 17:59:16]

this dcheck comes a little late.

[chili, 2016/12/15 10:34:39]

Done.

+  OfflinePageModel::SavePageParams params;
+  params.url = web_contents->GetLastCommittedURL();
+  params.client_id = request.client_id();
+  params.proposed_offline_id = request.request_id();
+  offline_page_model_->SavePage(
+      params, std::move(archiver),
+      base::Bind(&BackgroundLoaderOffliner::OnPageSaved,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void BackgroundLoaderOffliner::OnPageSaved(SavePageResult save_result,
+                                           int64_t offline_id) {
+  if (!pending_request_)
+    return;
+
+  SavePageRequest request(*pending_request_.get());
+
+  pending_request_.reset(nullptr);

[fgorski, 2016/12/12 17:59:16]

one more reason to put this in ResetState()
also you can drop nullptr from here too, I think.

[chili, 2016/12/15 10:34:39]

Done.

+  ResetState();
+
+  Offliner::RequestStatus save_status;
+  if (save_result == SavePageResult::SUCCESS) {

[fgorski, 2016/12/12 17:59:16]

{} not needed. remove, please.

[chili, 2016/12/15 10:34:39]

for some reason i thought it's necessary for if/else

+    save_status = RequestStatus::SAVED;
+  } else {
+    save_status = RequestStatus::SAVE_FAILED;
+  }
+
+  completion_callback_.Run(request, save_status);
+}
+
+void BackgroundLoaderOffliner::ResetState() {
+  // TODO(chili): Remove after RequestCoordinator can handle multiple offliners.
+  // We reset the loader and observer after completion so loaders
+  // will not be re-used across different requests/tries. This is a temporary
+  // solution while there exists assumptions about the number of offliners
+  // there are.
+  loader_.reset(
+      new background_loader::BackgroundLoaderContents(browser_context_));
+  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);
+  }
+}
+
+}  // namespace offline_pages