Failed offline snapshots won't erase a successful one from the same page load.

If a second offline snapshot starts for the same page load where a
previous one was saved, it should not immediately delete it: as it might
fail the user would end up with no offline copy of the page. This change
fixes this issue.

Three new tests are added: one to cover this precise case and a couple
more to check for related situations. Also many comments were added or
updated.

BUG=655697

Review-Url: https://codereview.chromium.org/2542833003
Cr-Commit-Position: refs/heads/master@{#436086}

chrome/browser/android/offline_pages/recent_tab_helper.cc

@@ -59,6 +59,7 @@ RecentTabHelper::RecentTabHelper(content::WebContents* web_contents)
       page_model_(nullptr),
       snapshots_enabled_(false),
       is_page_ready_for_snapshot_(false),
+      previous_request_id_for_load_(OfflinePageModel::kInvalidOfflineId),
       delegate_(new DefaultDelegate()),
       weak_ptr_factory_(this) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -160,6 +161,7 @@ void RecentTabHelper::DidFinishNavigation(
   }
 
   is_page_ready_for_snapshot_ = false;
+  previous_request_id_for_load_ = OfflinePageModel::kInvalidOfflineId;
 
   // New navigation, new snapshot session.
   snapshot_url_ = web_contents()->GetLastCommittedURL();
@@ -225,9 +227,17 @@ void RecentTabHelper::ContinueSnapshotWithIdsToPurge(
   if (!download_info_)
     return;
 
-  // Also remove the download page if this is not a first snapshot.
   std::vector<int64_t> ids(page_ids);
+  // Also remove the download page if this is not a first snapshot.
   ids.push_back(download_info_->request_id_);
+  // Do not delete a previous save from the same page load right now as it's
+  // unknown at this point in time if this snapshot will succeed.
+  for (auto it = ids.begin(); it != ids.end(); ++it) {
+    if (*it == previous_request_id_for_load_) {
+      ids.erase(it);
+      break;
+    }
+  }
 
   page_model_->DeletePagesByOfflineId(
       ids, base::Bind(&RecentTabHelper::ContinueSnapshotAfterPurge,
@@ -259,6 +269,30 @@ void RecentTabHelper::SavePageCallback(OfflinePageModel::SavePageResult result,
     return;
   download_info_->page_snapshot_completed_ =
       (result == SavePageResult::SUCCESS);
+
+  if (download_info_->page_snapshot_completed_) {
+    int64_t previous_request_id = previous_request_id_for_load_;
+    previous_request_id_for_load_ = offline_id;
+
+    // If there is a previous snapshot saved during the same page load it must
+    // be deleted before finishing the snapshot process.
+    if (previous_request_id != OfflinePageModel::kInvalidOfflineId) {
+      std::vector<int64_t> page_id = {previous_request_id};
+      page_model_->DeletePagesByOfflineId(
+          page_id, base::Bind(&RecentTabHelper::PreviousSaveWasPurged,
+                              weak_ptr_factory_.GetWeakPtr()));
+      return;
+    }
+  }
+
+  ReportSnapshotCompleted();
+}
+
+void RecentTabHelper::PreviousSaveWasPurged(
+    OfflinePageModel::DeletePageResult result) {
+  if (!download_info_)
+    return;
+
   ReportSnapshotCompleted();
 }
 

chrome/browser/android/offline_pages/recent_tab_helper.h

@@ -98,6 +98,7 @@ class RecentTabHelper
   void ContinueSnapshotAfterPurge(OfflinePageModel::DeletePageResult result);
   void SavePageCallback(OfflinePageModel::SavePageResult result,
                         int64_t offline_id);
+  void PreviousSaveWasPurged(OfflinePageModel::DeletePageResult result);
   void ReportSnapshotCompleted();
   void ReportDownloadStatusToRequestCoordinator();
   bool IsSamePage() const;
@@ -119,6 +120,10 @@ class RecentTabHelper
   // current page.
   std::unique_ptr<DownloadPageInfo> download_info_;
 
+  // Stores the request/offline ID of successful snapshots that happened during
+  // a single ongoing page load.
+  int64_t previous_request_id_for_load_;
+
   // If empty, the tab does not have AndroidId and can not capture pages.
   std::string tab_id_;
 

components/offline_pages/offline_page_test_archiver.h

@@ -50,7 +50,7 @@ class OfflinePageTestArchiver : public OfflinePageArchiver {
                      const CreateArchiveCallback& callback) override;
 
   // Completes the creation of archive. Should be used with |set_delayed| set to
-  // ture.
+  // true.
   void CompleteCreateArchive();
 
   // When set to true, |CompleteCreateArchive| should be called explicitly for