Modify RecentTabHelper to be always-on and observe the loading of the 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"
 #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/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 "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);
 
 namespace {
@@ skipping 15 matching lines @@
   }
 };
 }  // 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::LazyInitialize() {
-  snapshot_controller_.reset(new SnapshotController(delegate_->GetTaskRunner(),
-                                                    this));
+// 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.

[dewittj, 2016/08/24 23:01:39]

perhaps the snapshot controller should initially be stopped.

[Dmitry Titov, 2016/08/25 00:36:09]

Perhaps, but this is not the only user of it by now (Background Loader is using
it as well, assuming it is in READY state after creation). Looks ok to
explicitly set the state though...

[dewittj, 2016/08/25 01:35:22]

Acknowledged.

+
   int tab_id_number = 0;
   tab_id_.clear();
 
   if (delegate_->GetTabId(web_contents(), &tab_id_number))
     tab_id_ = base::IntToString(tab_id_number);
 
-  page_model_ = OfflinePageModelFactory::GetForBrowserContext(
-      web_contents()->GetBrowserContext());
-
   // TODO(dimich): When we have BackgroundOffliner, avoid capturing prerenderer
   // WebContents with its origin as well.
-  snapshots_enabled_ = page_model_ &&
-                       !tab_id_.empty() &&
+  snapshots_enabled_ = !tab_id_.empty() &&
                        !web_contents()->GetBrowserContext()->IsOffTheRecord();
 
   if (!snapshots_enabled_)
-    snapshot_controller_->Stop();
+    return;
+
+  page_model_ = OfflinePageModelFactory::GetForBrowserContext(
+      web_contents()->GetBrowserContext());
+}
+
+void RecentTabHelper::DidStartNavigation(

[dewittj, 2016/08/24 23:01:39]

This seems like it will change behavior for slower connections.  Isn't it
possible for navigation to start, but not be committed?  In that case the "right
moment" might still happen on the previous committed page.

Is this intentional/expected?  I'm worried that this might be more common that
we'd want in EM/slow networks type of places.

[Dmitry Titov, 2016/08/25 00:36:09]

Done. I guess I'm being too paranoid about DidFinishNavigation not being called
in some cases...

+    content::NavigationHandle* navigation_handle) {
+  if (!navigation_handle->IsInMainFrame())
+    return;
+  // Cancel tasks in flight that relate to the previous page.
+  weak_ptr_factory_.InvalidateWeakPtrs();
+  is_page_ready_for_snapshot_ = false;
 }
 
 void RecentTabHelper::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   if (!navigation_handle->IsInMainFrame() ||
       !navigation_handle->HasCommitted()) {
     return;
   }
 
-  // Initialize lazily. It needs TabAndroid for initization, which is also a
-  // TabHelper - so can't initialize in constructor because of uncertain order
-  // of creation of TabHelpers.
-  if (!snapshot_controller_)
-    LazyInitialize();
-
+  EnsureInitialized();

[dewittj, 2016/08/24 23:01:39]

optional nit: conceptually it might make sense to have this method return a bool
representing enabled/ok to proceed with snapshot, rather than tracking
snapshots_enabled_... wdyt?

[Dmitry Titov, 2016/08/25 00:36:09]

I think I'd need to cache the snapshots_enabled to be able to return it every
time from EnsureInitialized... I see your point but it doesn't seem to make the
code easier to read.

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

[dewittj, 2016/08/24 23:01:39]

Why is this gone?  It seems like there are still places that we'd want to cancel
async steps.

[Dmitry Titov, 2016/08/25 00:36:08]

Moved back, and before initialization.

-
   // New navigation, new snapshot session.
-  snapshot_url_ = GURL();
-  GURL last_committed_url = web_contents()->GetLastCommittedURL();
+  snapshot_url_ = web_contents()->GetLastCommittedURL();
 
   // Check for conditions that would cause us not to snapshot.
   bool can_save = !navigation_handle->IsErrorPage() &&
-                  OfflinePageModel::CanSaveURL(last_committed_url);
+                  OfflinePageModel::CanSaveURL(snapshot_url_);
 
   UMA_HISTOGRAM_BOOLEAN("OfflinePages.CanSaveRecentPage", can_save);
 
-  // Always reset so that posted tasks get cancelled.
+  // Always reset so that posted tasks get canceled.
   snapshot_controller_->Reset();
 
   if (!can_save)
     snapshot_controller_->Stop();
-  else
-    snapshot_url_ = last_committed_url;
 }
 
 void RecentTabHelper::DocumentAvailableInMainFrame() {
-  if (!snapshots_enabled_)
-    return;
+  EnsureInitialized();
   snapshot_controller_->DocumentAvailableInMainFrame();
 }
 
 void RecentTabHelper::DocumentOnLoadCompletedInMainFrame() {
-  // TODO(dimich): Figure out when this can fire before DidFinishNavigate().
-  // See bug 628716 for more info.
-  if (!snapshots_enabled_)
-    return;
+  EnsureInitialized();
   snapshot_controller_->DocumentOnLoadCompletedInMainFrame();
 }
 
 // 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() {
-  if (!snapshots_enabled_)
+  is_page_ready_for_snapshot_ = true;
+
+  if (!snapshots_enabled_ ||
+      !page_model_ ||
+      !offline_pages::IsOffliningRecentPagesEnabled()) {
+    ReportSnapshotCompleted();
     return;
+  }
+
   // Remove previously captured pages for this tab.
   page_model_->GetOfflineIdsForClientId(
     client_id(),
     base::Bind(&RecentTabHelper::ContinueSnapshotWithIdsToPurge,
                weak_ptr_factory_.GetWeakPtr()));
 }
 
 void RecentTabHelper::ContinueSnapshotWithIdsToPurge(
     const std::vector<int64_t>& page_ids) {
   page_model_->DeletePagesByOfflineId(
@@ skipping 32 matching lines @@
 bool RecentTabHelper::IsSamePage() const {
   return web_contents() &&
          (web_contents()->GetLastCommittedURL() == snapshot_url_);
 }
 
 ClientId RecentTabHelper::client_id() const {
   return ClientId(kLastNNamespace, tab_id_);
 }
 
 }  // namespace offline_pages