Minor improvements to RecentTabHelper.

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/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;
   }
 
   // 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/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>(
         GetRecentPagesClientId(), OfflinePageModel::kInvalidOfflineId);
   } else {
     download_info_.reset();
   }
 
   is_page_ready_for_snapshot_ = false;
 
@@ skipping 51 matching lines @@
 
   // 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()) {

[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;
   }
 
   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() {
   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())

[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 =
       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: 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