Improve the page download:

chrome/browser/android/offline_pages/recent_tab_helper.cc

 // Copyright (c) 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/recent_tab_helper.h"
 
 #include <queue>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
+#include "chrome/browser/android/offline_pages/downloads/offline_page_notification_bridge.h"
 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
 #include "chrome/browser/android/offline_pages/offline_page_model_factory.h"
 #include "chrome/browser/android/offline_pages/offline_page_utils.h"
+#include "chrome/browser/android/offline_pages/request_coordinator_factory.h"
+#include "components/offline_pages/background/request_coordinator.h"
 #include "components/offline_pages/client_namespace_constants.h"
 #include "components/offline_pages/offline_page_feature.h"
 #include "components/offline_pages/offline_page_item.h"
 #include "components/offline_pages/offline_page_model.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(offline_pages::RecentTabHelper);
@@ skipping 16 matching lines @@
     return offline_pages::OfflinePageUtils::GetTabId(web_contents, tab_id);
   }
 };
 }  // namespace
 
 namespace offline_pages {
 
 RecentTabHelper::RecentTabHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       page_model_(nullptr),
+      is_page_ready_for_snapshot_(false),
       snapshots_enabled_(false),
-      is_page_ready_for_snapshot_(false),
       delegate_(new DefaultDelegate()),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 }
 
 RecentTabHelper::~RecentTabHelper() {
 }
 
 void RecentTabHelper::SetDelegate(
     std::unique_ptr<RecentTabHelper::Delegate> delegate) {
   DCHECK(delegate);
   delegate_ = std::move(delegate);
 }
 
+void RecentTabHelper::ObserveAndDownloadCurrentPage(
+    const ClientId& client_id, int64_t request_id) {
+  EnsureInitialized();
+  download_info_ = base::MakeUnique<DownloadPageInfo>(client_id, request_id);
+
+  // If this tab helper is not enabled, immediately give the job back to
+  // RequestCoordinator.
+  if (!snapshots_enabled_ || !page_model_) {
+    ReportDownloadRequestStatus(false);
+    download_info_.reset();
+    return;
+  }
+
+  // No snapshots yet happened on the current page - return and wait for some.
+  if (!is_page_ready_for_snapshot_)
+    return;
+
+  // If snapshot already happened and we missed it, go ahead and snapshot now.
+  page_model_->SavePage(
+      web_contents()->GetLastCommittedURL(),
+      client_id,
+      request_id,
+      delegate_->CreatePageArchiver(web_contents()),
+      base::Bind(&RecentTabHelper::SavePageCallback,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
 // Initialize lazily. It needs TabAndroid for initialization, which is also a
 // TabHelper - so can't initialize in constructor because of uncertain order
 // of creation of TabHelpers.
 void RecentTabHelper::EnsureInitialized() {
   if (snapshot_controller_)  // Initialized already.
     return;
 
   snapshot_controller_.reset(
       new SnapshotController(delegate_->GetTaskRunner(), this));
   snapshot_controller_->Stop();  // It is reset when navigation commits.
@@ skipping 18 matching lines @@
 
 void RecentTabHelper::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->IsInMainFrame() ||
       !navigation_handle->HasCommitted()) {
     return;
   }
 
   // Cancel tasks in flight that relate to the previous page.
   weak_ptr_factory_.InvalidateWeakPtrs();
-  is_page_ready_for_snapshot_ = false;
 
   EnsureInitialized();
   if (!snapshots_enabled_)
     return;
 
+  // We navigated to a different page, lets report progress to Background
+  // Offliner.
+  if (download_info_ && !navigation_handle->IsSamePage()) {
+    ReportDownloadRequestStatus(download_info_->page_snapshot_completed_);
+  }
+
+  if (offline_pages::IsOffliningRecentPagesEnabled()) {
+    download_info_ = base::MakeUnique<DownloadPageInfo>(
+        GetRecentPagesClientId(), 0l);
+  } else {
+    download_info_.reset();
+  }
+
+  is_page_ready_for_snapshot_ = false;
 
   // New navigation, new snapshot session.
   snapshot_url_ = web_contents()->GetLastCommittedURL();
 
   // Check for conditions that would cause us not to snapshot.
   bool can_save = !navigation_handle->IsErrorPage() &&
                   OfflinePageModel::CanSaveURL(snapshot_url_);
 
   UMA_HISTOGRAM_BOOLEAN("OfflinePages.CanSaveRecentPage", can_save);
 
   // Always reset so that posted tasks get canceled.
   snapshot_controller_->Reset();
 
   if (!can_save)
     snapshot_controller_->Stop();
 }
 
 void RecentTabHelper::DocumentAvailableInMainFrame() {
   EnsureInitialized();
   snapshot_controller_->DocumentAvailableInMainFrame();
 }
 
 void RecentTabHelper::DocumentOnLoadCompletedInMainFrame() {
   EnsureInitialized();
   snapshot_controller_->DocumentOnLoadCompletedInMainFrame();
 }
 
+void RecentTabHelper::WebContentsDestroyed() {
+  // WebContents (and maybe Tab) is destroyed, report status to Offliner.
+  if (!download_info_)
+    return;
+  ReportDownloadRequestStatus(download_info_->page_snapshot_completed_);
+}
+
+
 // This starts a sequence of async operations chained through callbacks:
 // - compute the set of old 'last_n' pages that have to be purged
 // - delete the pages found in the previous step
 // - snapshot the current web contents
 // Along the chain, the original URL is passed and compared, to detect
 // possible navigation and cancel snapshot in that case.
 void RecentTabHelper::StartSnapshot() {
   is_page_ready_for_snapshot_ = true;
 
   if (!snapshots_enabled_ ||
       !page_model_ ||
-      !offline_pages::IsOffliningRecentPagesEnabled()) {
-    ReportSnapshotCompleted();
+      !download_info_) {
+    ReportSnapshotCompleted(false);

[dewittj, 2016/10/12 03:27:27]

nit: either replace bool with an enum class or add comments /* success= */

Also rest of file.

[Dmitry Titov, 2016/10/12 23:11:14]

Done. No parameters.

     return;
   }
 
   // Remove previously captured pages for this tab.
   page_model_->GetOfflineIdsForClientId(
-    client_id(),
+    GetRecentPagesClientId(),
     base::Bind(&RecentTabHelper::ContinueSnapshotWithIdsToPurge,
                weak_ptr_factory_.GetWeakPtr()));
 }
 
 void RecentTabHelper::ContinueSnapshotWithIdsToPurge(
     const std::vector<int64_t>& page_ids) {
+  if (!download_info_)
+    return;
+
+  // Also remove the download page if this is not a first snapshot.
+  std::vector<int64_t> ids(page_ids);
+  ids.push_back(download_info_->request_id_);
+
   page_model_->DeletePagesByOfflineId(
-      page_ids, base::Bind(&RecentTabHelper::ContinueSnapshotAfterPurge,
-                           weak_ptr_factory_.GetWeakPtr()));
+      ids, base::Bind(&RecentTabHelper::ContinueSnapshotAfterPurge,
+                      weak_ptr_factory_.GetWeakPtr()));
 }
 
 void RecentTabHelper::ContinueSnapshotAfterPurge(
     OfflinePageModel::DeletePageResult result) {
-  if (result != OfflinePageModel::DeletePageResult::SUCCESS) {
-    // If previous pages can't be deleted, don't add new ones.
-    ReportSnapshotCompleted();
+  if (!download_info_ ||
+      result != OfflinePageModel::DeletePageResult::SUCCESS ||
+      !IsSamePage()) {
+    ReportSnapshotCompleted(false);
     return;
   }
 
-  if (!IsSamePage()) {
-    ReportSnapshotCompleted();
-    return;
-  }
-
-  page_model_->SavePage(snapshot_url_, client_id(), 0l,
+  page_model_->SavePage(snapshot_url_,
+                        download_info_->client_id_,
+                        download_info_->request_id_,
                         delegate_->CreatePageArchiver(web_contents()),
                         base::Bind(&RecentTabHelper::SavePageCallback,
                                    weak_ptr_factory_.GetWeakPtr()));
 }
 
 void RecentTabHelper::SavePageCallback(OfflinePageModel::SavePageResult result,
                                        int64_t offline_id) {
-  ReportSnapshotCompleted();
+  if (!download_info_)
+    return;
+  bool success = (result == SavePageResult::SUCCESS);
+  download_info_->page_snapshot_completed_ = success;

[dewittj, 2016/10/12 03:27:27]

could |page_snapshot_completed_| be used instead of the parameter, just checking
that down in ReportDownloadRequestStatus?

[Dmitry Titov, 2016/10/12 23:11:14]

Done. This is actually more correct, thanks!

+  ReportSnapshotCompleted(success);
 }
 
-void RecentTabHelper::ReportSnapshotCompleted() {
+void RecentTabHelper::ReportSnapshotCompleted(bool success) {
   snapshot_controller_->PendingSnapshotCompleted();
+  // Tell RequestCoordinator how the request should be processed further.
+  ReportDownloadRequestStatus(success);
+}
+
+void RecentTabHelper::ReportDownloadRequestStatus(bool completed) {

[Pete Williamson, 2016/10/12 17:33:36]

The function name doesn't represent what it does well.  Maybe
ReportDownloadStatusToRequestCoordinator() ?
ReportStatusToRequestCoordinator() ?

The function seems to have two duties, to make sure that the status makes it to
the notifier, and to take the request off the disabled list.  It would be great
if the name could represent both of these.

[Dmitry Titov, 2016/10/12 23:11:14]

Done.

+  if (!download_info_ || download_info_->request_id_ <= 0l)
+    return;
+
+  RequestCoordinator* request_coordinator =
+      RequestCoordinatorFactory::GetForBrowserContext(
+          web_contents()->GetBrowserContext());
+  if (!request_coordinator)
+    return;
+
+  // It is OK to call these methods more then once, depending on
+  // number of snapshots attempted in this tab helper. If the request_id is not
+  // in the list of RequestCoordinator, these calls have no effect.
+  if (completed)
+    request_coordinator->MarkRequestCompleted(download_info_->request_id_);
+  else
+    request_coordinator->EnableForOffliner(download_info_->request_id_);
 }
 
 bool RecentTabHelper::IsSamePage() const {
   return web_contents() &&
          (web_contents()->GetLastCommittedURL() == snapshot_url_);
 }
 
-ClientId RecentTabHelper::client_id() const {
+ClientId RecentTabHelper::GetRecentPagesClientId() const {
   return ClientId(kLastNNamespace, tab_id_);
 }
 
 }  // namespace offline_pages