Modify RecentTabHelper to be always-on and observe the loading of the 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 a4b5be9a4bb2a2ec9712b8d54d522d5854fa75e8..d7c501aa17f655f9691c7f51e5f0bcbf2b63c0d9 100644
--- a/chrome/browser/android/offline_pages/recent_tab_helper.cc
+++ b/chrome/browser/android/offline_pages/recent_tab_helper.cc
@@ -19,6 +19,7 @@
 #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"
@@ -54,6 +55,7 @@ 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);
@@ -68,26 +70,42 @@ void RecentTabHelper::SetDelegate(
   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(
@@ -97,48 +115,33 @@ void RecentTabHelper::DidFinishNavigation(
     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();
 }
 
@@ -149,8 +152,15 @@ void RecentTabHelper::DocumentOnLoadCompletedInMainFrame() {
 // 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(),