Offline Page Cache uses user interaction triggers for saving pages.

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"
@@ skipping 38 matching lines @@
   bool GetTabId(content::WebContents* web_contents, int* tab_id) override {
     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),
-      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_) {
     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;
   }
 
   // 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.
   OfflinePageModel::SavePageParams save_page_params;
@@ skipping 32 matching lines @@
   if (!snapshots_enabled_)
     return;
 
   page_model_ = OfflinePageModelFactory::GetForBrowserContext(
       web_contents()->GetBrowserContext());
 }
 
 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;
   }
+  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();
 
-  EnsureInitialized();
-  if (!snapshots_enabled_)
-    return;
-
   // 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;
 
   // 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_) &&
                   OfflinePageUtils::GetOfflinePageFromWebContents(
                       web_contents()) == nullptr;
 
   UMA_HISTOGRAM_BOOLEAN("OfflinePages.CanSaveRecentPage", can_save);
 
   // 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() {
   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;
   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
 // - 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_ ||
-      !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(),
     base::Bind(&RecentTabHelper::ContinueSnapshotWithIdsToPurge,
                weak_ptr_factory_.GetWeakPtr()));
 }
 
 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);
   ids.push_back(download_info_->request_id_);
 
   page_model_->DeletePagesByOfflineId(
       ids, base::Bind(&RecentTabHelper::ContinueSnapshotAfterPurge,
                       weak_ptr_factory_.GetWeakPtr()));
 }
 
 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_;
   save_page_params.proposed_offline_id = download_info_->request_id_;
   page_model_->SavePage(save_page_params,
                         delegate_->CreatePageArchiver(web_contents()),
                         base::Bind(&RecentTabHelper::SavePageCallback,
                                    weak_ptr_factory_.GetWeakPtr()));
 }
 
 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 =
       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 (download_info_->page_snapshot_completed_)
     request_coordinator->MarkRequestCompleted(download_info_->request_id_);
   else
     request_coordinator->EnableForOffliner(download_info_->request_id_,
                                            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