Minor improvements to RecentTabHelper.

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..8283a5670264e08ee98293c83581d2277f7b1619 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/17 23:09:56]

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, so
to stop any interrupt any ongoing snapshot save by interrupting its callback
chain.

     download_info_.reset();
     return;
   }
@@ -134,22 +132,21 @@ 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/17 23:09:56]

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 mutually 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.

[carlosk, 2017/01/18 03:06:10]

I had to remove the IsExternalProtocol check because it was causing 3 tests to
crash. I'm asking around to see if that's WAI or a bug.

     return;
   }
 
-  // Cancel tasks in flight that relate to the previous page.
-  weak_ptr_factory_.InvalidateWeakPtrs();
-
   EnsureInitialized();
   if (!snapshots_enabled_)
     return;
 
+  // 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()) {

[carlosk, 2017/01/17 23:09:57]

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

[dewittj, 2017/01/17 23:21:52]

Perhaps a minor rename to reflect the download_info_ check would be helpful?
OTOH, the name is already pretty long :)

[carlosk, 2017/01/18 03:06:10]

Keeping as is as discussed offline.

-    ReportDownloadStatusToRequestCoordinator();
-  }
+  ReportDownloadStatusToRequestCoordinator();
 
   if (offline_pages::IsOffliningRecentPagesEnabled()) {
     download_info_ = base::MakeUnique<DownloadPageInfo>(
@@ -221,8 +218,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,9 +231,9 @@ void RecentTabHelper::ContinueSnapshotWithIdsToPurge(
 
 void RecentTabHelper::ContinueSnapshotAfterPurge(
     OfflinePageModel::DeletePageResult result) {
-  if (!download_info_ ||
-      result != OfflinePageModel::DeletePageResult::SUCCESS ||
-      !IsSamePage()) {

[carlosk, 2017/01/17 23:09:57]

This IsSamePage check is not needed: if anything happens to the committed URL we
will receive a DidFinishNavigation with the commit where all callback pointers
will be invalidated. Replaced with DCHECK just to play it safe.

+  DCHECK(download_info_);
+  DCHECK_EQ(snapshot_url_, web_contents()->GetLastCommittedURL());
+  if (result != OfflinePageModel::DeletePageResult::SUCCESS) {
     ReportSnapshotCompleted();
     return;
   }
@@ -254,8 +250,7 @@ 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();
@@ -268,10 +263,7 @@ void RecentTabHelper::ReportSnapshotCompleted() {
 }
 
 void RecentTabHelper::ReportDownloadStatusToRequestCoordinator() {
-  if (!download_info_)
-    return;
-
-  if (download_info_->request_id_ == OfflinePageModel::kInvalidOfflineId)
+  if (!download_info_ || isSnapshottingForLastN())

[dewittj, 2017/01/17 23:21:52]

!download_info_ is redundant here unless you take my advice to move
|isSnapshottingForLastN| into DownloadPageInfo.

[carlosk, 2017/01/18 03:06:10]

It is not redundant: here the value is "or-ed" (||) whereas in
isSnapshottingForLastN it is "and-ed" (&&). But I'm eliminating that other
methods because right now this is the only use of it.

     return;
 
   RequestCoordinator* request_coordinator =
@@ -290,13 +282,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: 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