Improve the page download:

chrome/browser/android/offline_pages/recent_tab_helper.cc

Index: chrome/browser/android/offline_pages/recent_tab_helper.cc
diff --git a/chrome/browser/android/offline_pages/recent_tab_helper.cc b/chrome/browser/android/offline_pages/recent_tab_helper.cc
index dd4badbcdc2b33bbd1eb0af96d8fd58543a5ad55..979a54bdda5ad2037addc9f6d8e0cb4c6e0a576f 100644
--- a/chrome/browser/android/offline_pages/recent_tab_helper.cc
+++ b/chrome/browser/android/offline_pages/recent_tab_helper.cc
@@ -15,9 +15,12 @@
 #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"
@@ -54,8 +57,8 @@ namespace offline_pages {
 RecentTabHelper::RecentTabHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       page_model_(nullptr),
-      snapshots_enabled_(false),
       is_page_ready_for_snapshot_(false),
+      snapshots_enabled_(false),
       delegate_(new DefaultDelegate()),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -70,6 +73,33 @@ void RecentTabHelper::SetDelegate(
   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.
@@ -108,12 +138,25 @@ void RecentTabHelper::DidFinishNavigation(
 
   // 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();
@@ -141,6 +184,14 @@ void RecentTabHelper::DocumentOnLoadCompletedInMainFrame() {
   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
@@ -152,39 +203,44 @@ void RecentTabHelper::StartSnapshot() {
 
   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()));
@@ -192,11 +248,36 @@ void RecentTabHelper::ContinueSnapshotAfterPurge(
 
 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 {
@@ -204,7 +285,7 @@ bool RecentTabHelper::IsSamePage() const {
          (web_contents()->GetLastCommittedURL() == snapshot_url_);
 }
 
-ClientId RecentTabHelper::client_id() const {
+ClientId RecentTabHelper::GetRecentPagesClientId() const {
   return ClientId(kLastNNamespace, tab_id_);
 }