Offline Page Cache uses user interaction triggers for saving pages.

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 cdc9d8445dbe024e9c4cb61bc473ea53ec4da73b..b75e7a0e469b67b2671f668b1e5e4b987a169988 100644
--- a/chrome/browser/android/offline_pages/recent_tab_helper.cc
+++ b/chrome/browser/android/offline_pages/recent_tab_helper.cc
@@ -56,9 +56,6 @@ 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),
       delegate_(new DefaultDelegate()),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -82,6 +79,7 @@ void RecentTabHelper::ObserveAndDownloadCurrentPage(
   // RequestCoordinator.
   if (!snapshots_enabled_ || !page_model_) {
     ReportDownloadStatusToRequestCoordinator();
+    weak_ptr_factory_.InvalidateWeakPtrs();

[carlosk, 2017/01/04 20:52:48]

This change is what justifies the change of if-checks to DCHECKs for the
callbacks further below. I couldn't find any use case where resetting
download_info_ (just below) wouldn't justify invalidating all weak pointers
(what IIUC would avoid any future scheduled callback calls).

[Dmitry Titov, 2017/01/05 21:50:03]

Acknowledged.

     download_info_.reset();
     return;
   }
@@ -134,29 +132,20 @@ void RecentTabHelper::EnsureInitialized() {
 void RecentTabHelper::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->IsInMainFrame() ||
-      !navigation_handle->HasCommitted()) {
+      navigation_handle->IsExternalProtocol() ||
+      navigation_handle->IsSamePage() || !navigation_handle->HasCommitted()) {

[carlosk, 2017/01/04 20:52:48]

IsExternalProtocol: if a navigation is handled externally the current page
contents should not change so we should not snapshot IMO. It might be the case
that this and HasCommitted are muttually exclusive (I'm not sure) so this is
just-in-case.

IsSamePage: same page navigations shouldn't change the page contents either.
There's even some pages (I think YouTube video pages are one example) that
execute a same page navigation during a regular load through javascript.

[dewittj, 2017/01/07 00:50:09]

This change should probably be extracted into a small CL

[carlosk, 2017/01/17 23:16:27]

Moved this and many others to a new CL: https://crrev.com/2635023005. Will
rebase into that change in the next patch set with the new approach.

     return;
   }
-
-  // Cancel tasks in flight that relate to the previous page.
-  weak_ptr_factory_.InvalidateWeakPtrs();
-
   EnsureInitialized();
   if (!snapshots_enabled_)
     return;

[carlosk, 2017/01/04 20:52:48]

It might be better to move this up even further. If snapshots_enabled_ is true
we won't be doing anything anyway.

[Dmitry Titov, 2017/01/05 21:50:03]

Acknowledged.

 
+  // Cancel tasks in flight that relate to the previous page.
+  weak_ptr_factory_.InvalidateWeakPtrs();
+
   // We navigated to a different page, lets report progress to Background
   // Offliner.
-  if (download_info_ && !navigation_handle->IsSamePage()) {
-    ReportDownloadStatusToRequestCoordinator();
-  }

[carlosk, 2017/01/04 20:52:48]

The same page check now happens earlier and the download_info_ check happens
inside ReportDownloadStatusToRequestCoordinator anyway.

[Dmitry Titov, 2017/01/05 21:50:03]

Acknowledged.

-
-  if (offline_pages::IsOffliningRecentPagesEnabled()) {
-    download_info_ = base::MakeUnique<DownloadPageInfo>(
-        GetRecentPagesClientId(), OfflinePageModel::kInvalidOfflineId);
-  } else {
-    download_info_.reset();
-  }
+  ReportDownloadStatusToRequestCoordinator();
 
   is_page_ready_for_snapshot_ = false;
 
@@ -174,8 +163,22 @@ void RecentTabHelper::DidFinishNavigation(
   // Always reset so that posted tasks get canceled.
   snapshot_controller_->Reset();
 
-  if (!can_save)
+  if (can_save) {
+    // When last_n is using navigation triggers |download_info_| will always be
+    // set for a page being that can be saved. Otherwise, when using user
+    // interaction triggers, it is only set while a page is being saved.
+    if (IsOffliningRecentPagesUsingNavigationTriggers()) {
+      download_info_ = base::MakeUnique<DownloadPageInfo>(
+          GetRecentPagesClientId(), OfflinePageModel::kInvalidOfflineId);
+    } else if (IsOffliningRecentPagesUsingInteractionTriggers()) {
+      last_n_listen_to_tab_hidden_ = true;
+      last_n_saved_quality_ = PageQuality::POOR;
+    }
+  } else {
+    last_n_listen_to_tab_hidden_ = false;
+    download_info_.reset();

[Dmitry Titov, 2017/01/05 21:50:03]

I think this reset() should happen unconditionally before this big if(can_save).
Otherwise there is a branch in these ifs that keeps the old download_info_,
which should not happen if we actually navigated.

[carlosk, 2017/01/17 23:16:27]

Done. Indeed, every time we invalidate pointers we should also reset
download_info_.

     snapshot_controller_->Stop();
+  }
 }
 
 void RecentTabHelper::DocumentAvailableInMainFrame() {
@@ -195,6 +198,28 @@ void RecentTabHelper::WebContentsDestroyed() {
   ReportDownloadStatusToRequestCoordinator();
 }
 
+void RecentTabHelper::WasHidden() {
+  if (!IsOffliningRecentPagesUsingInteractionTriggers())
+    return;
+
+  // Return immediately if any of these are true last_n is not listening to tab
+  // hidden events or if a snapshot is currently being saved.
+  if (!last_n_listen_to_tab_hidden_ || download_info_)
+    return;
+
+  // Do not save if page quality is too low or if we already have a save with
+  // the current quality level.
+  // Note: we assume page quality for a page can only increase.
+  PageQuality current_quality = snapshot_controller_->current_page_quality();
+  if (current_quality == PageQuality::POOR ||
+      current_quality == last_n_saved_quality_) {
+    return;
+  }
+
+  download_info_ = base::MakeUnique<DownloadPageInfo>(
+      GetRecentPagesClientId(), OfflinePageModel::kInvalidOfflineId);
+  StartSnapshotInternal();
+}
 
 // This starts a sequence of async operations chained through callbacks:
 // - compute the set of old 'last_n' pages that have to be purged
@@ -205,13 +230,28 @@ void RecentTabHelper::WebContentsDestroyed() {
 void RecentTabHelper::StartSnapshot() {
   is_page_ready_for_snapshot_ = true;
 
-  if (!snapshots_enabled_ ||
-      !page_model_ ||
-      !download_info_) {
-    ReportSnapshotCompleted();
-    return;
+  // This is a navigation based snapshot trigger: before proceeding we must
+  // verify we should proceed.
+  bool should_save = snapshots_enabled_ && page_model_ && download_info_;
+
+  // If navigation triggers are disabled for last_n, don't proceed if this is
+  // not save requested by downloads.
+  if (IsOffliningRecentPagesUsingInteractionTriggers())
+    should_save &= !isSnapshottingForLastN();
+
+  if (should_save) {
+    StartSnapshotInternal();
+  } else {
+    // We're not calling ReportSnapshotCompleted here because we don't want to
+    // mix up signals of this with a last_n save being completed.
+    snapshot_controller_->PendingSnapshotCompleted();
+    ReportDownloadStatusToRequestCoordinator();
   }
+}
 
+void RecentTabHelper::StartSnapshotInternal() {
+  DCHECK(snapshots_enabled_);
+  DCHECK(download_info_);
   // Remove previously captured pages for this tab.
   page_model_->GetOfflineIdsForClientId(
     GetRecentPagesClientId(),
@@ -221,8 +261,7 @@ void RecentTabHelper::StartSnapshot() {
 
 void RecentTabHelper::ContinueSnapshotWithIdsToPurge(
     const std::vector<int64_t>& page_ids) {
-  if (!download_info_)
-    return;
+  DCHECK(download_info_);
 
   // Also remove the download page if this is not a first snapshot.
   std::vector<int64_t> ids(page_ids);
@@ -235,13 +274,14 @@ void RecentTabHelper::ContinueSnapshotWithIdsToPurge(
 
 void RecentTabHelper::ContinueSnapshotAfterPurge(
     OfflinePageModel::DeletePageResult result) {
-  if (!download_info_ ||
-      result != OfflinePageModel::DeletePageResult::SUCCESS ||
-      !IsSamePage()) {
+  DCHECK(download_info_);
+  if (result != OfflinePageModel::DeletePageResult::SUCCESS) {
     ReportSnapshotCompleted();
     return;
   }
 
+  page_quality_on_save_start_ = snapshot_controller_->current_page_quality();
+
   OfflinePageModel::SavePageParams save_page_params;
   save_page_params.url = snapshot_url_;
   save_page_params.client_id = download_info_->client_id_;
@@ -254,24 +294,29 @@ void RecentTabHelper::ContinueSnapshotAfterPurge(
 
 void RecentTabHelper::SavePageCallback(OfflinePageModel::SavePageResult result,
                                        int64_t offline_id) {
-  if (!download_info_)
-    return;
+  DCHECK(download_info_);
   download_info_->page_snapshot_completed_ =
       (result == SavePageResult::SUCCESS);
   ReportSnapshotCompleted();
 }
 
 void RecentTabHelper::ReportSnapshotCompleted() {
+  if (IsOffliningRecentPagesUsingInteractionTriggers() &&
+      isSnapshottingForLastN()) {
+    // This was a last_n save triggered by the tab being hidden.
+    if (download_info_->page_snapshot_completed_)
+      last_n_saved_quality_ = page_quality_on_save_start_;
+    download_info_.reset();
+    return;
+  }
+
   snapshot_controller_->PendingSnapshotCompleted();
   // Tell RequestCoordinator how the request should be processed further.
   ReportDownloadStatusToRequestCoordinator();
 }
 
 void RecentTabHelper::ReportDownloadStatusToRequestCoordinator() {
-  if (!download_info_)
-    return;
-
-  if (download_info_->request_id_ == OfflinePageModel::kInvalidOfflineId)
+  if (!download_info_ || isSnapshottingForLastN())
     return;
 
   RequestCoordinator* request_coordinator =
@@ -290,13 +335,17 @@ void RecentTabHelper::ReportDownloadStatusToRequestCoordinator() {
                                            download_info_->client_id_);
 }
 
-bool RecentTabHelper::IsSamePage() const {
-  return web_contents() &&
-         (web_contents()->GetLastCommittedURL() == snapshot_url_);
-}
-
 ClientId RecentTabHelper::GetRecentPagesClientId() const {
   return ClientId(kLastNNamespace, tab_id_);
 }
 
+bool RecentTabHelper::isSnapshottingForLastN() const {
+  // For a snapshot to be executing for a last_n save download_info_ must be set
+  // and it must have an invalid request id.
+  // Note that at any point in time the download_info_ can be replaced with one
+  // created for a download snapshot request.
+  return download_info_ &&
+         download_info_->request_id_ == OfflinePageModel::kInvalidOfflineId;
+}
+
 }  // namespace offline_pages