Last_n: Delete previously saved snapshot when navigating.

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 f248220fa487c9f93c2255b461f8081c191ff694..4bccbf2bbcb9a41c5c6faf3f4880eba796bdfacb 100644
--- a/chrome/browser/android/offline_pages/recent_tab_helper.cc
+++ b/chrome/browser/android/offline_pages/recent_tab_helper.cc
@@ -212,9 +212,13 @@ void RecentTabHelper::DidFinishNavigation(
         downloads_ongoing_snapshot_info_.get(), false);
   }
 
+  // If the previous page was saved, delete it now.
+  if (last_n_latest_saved_snapshot_info_) {

[dewittj, 2017/04/17 21:17:42]

This is an interesting strategy, I wonder if there are any race conditions (with
Chrome exiting for example) that might prevent us from deleting the page.

Also, why have a whole new snapshot info rather than just annotating the
existing snapshot info as complete?

[carlosk, 2017/04/18 17:56:19]

There is a very small window of time between the page committing and this
observer method being called when if Chrome should be terminated within it then
it would cause the snapshot to be left alive and potentially accessible.

But upon future restarts the snapshot will eventually be deleted either when a
new snapshot should be saved for that tab or when the tab should be closed.
 
This mimics the way we already keep the last successful snapshot info for
downloads so it makes it consistent with how things already work inside this
file.

I am not creating a new instance but transferring from one smart pointer to
another. I can't keep the "ongoing" pointer after snapshot completion because
its existence is used as a flag for ongoing process of snapshot creation.

+    std::vector<int64_t> id{last_n_latest_saved_snapshot_info_->request_id};
+    page_model_->DeletePagesByOfflineId(id, DeletePageCallback());
+  }
+
   // Cancel any and all in flight snapshot tasks from the previous page.
-  DVLOG_IF(1, last_n_ongoing_snapshot_info_)
-      << " - Canceling ongoing last_n snapshot";
   CancelInFlightSnapshots();
   downloads_snapshot_on_hold_ = false;
 
@@ -419,8 +423,12 @@ void RecentTabHelper::ContinueSnapshotAfterPurge(
 void RecentTabHelper::SavePageCallback(SnapshotProgressInfo* snapshot_info,
                                        OfflinePageModel::SavePageResult result,
                                        int64_t offline_id) {
-  DCHECK(snapshot_info->IsForLastN() ||
+  DCHECK((snapshot_info->IsForLastN() &&
+          snapshot_info->request_id == OfflinePageModel::kInvalidOfflineId) ||
          snapshot_info->request_id == offline_id);
+  // Store the assigned offline_id (for downloads case it will already contain
+  // the same value).
+  snapshot_info->request_id = offline_id;
   ReportSnapshotCompleted(snapshot_info, result == SavePageResult::SUCCESS);
 }
 
@@ -434,7 +442,12 @@ void RecentTabHelper::ReportSnapshotCompleted(
            << " for: " << web_contents()->GetLastCommittedURL().spec();
   if (snapshot_info->IsForLastN()) {
     DCHECK_EQ(snapshot_info, last_n_ongoing_snapshot_info_.get());
-    last_n_ongoing_snapshot_info_.reset();
+    if (success) {
+      last_n_latest_saved_snapshot_info_ =
+          std::move(last_n_ongoing_snapshot_info_);
+    } else {
+      last_n_ongoing_snapshot_info_.reset();
+    }
     return;
   }
 
@@ -481,10 +494,15 @@ ClientId RecentTabHelper::GetRecentPagesClientId() const {
 }
 
 void RecentTabHelper::CancelInFlightSnapshots() {
+  DVLOG_IF(1, last_n_ongoing_snapshot_info_)
+      << " - Canceling ongoing last_n snapshot";
+  DVLOG_IF(1, downloads_ongoing_snapshot_info_)
+      << " - Canceling ongoing downloads snapshot";
   weak_ptr_factory_.InvalidateWeakPtrs();
   downloads_ongoing_snapshot_info_.reset();
   downloads_latest_saved_snapshot_info_.reset();
   last_n_ongoing_snapshot_info_.reset();
+  last_n_latest_saved_snapshot_info_.reset();
 }
 
 }  // namespace offline_pages