RecentTabHelper won't accept overlapping requests from Downloads anymore.

chrome/browser/android/offline_pages/recent_tab_helper.h

Index: chrome/browser/android/offline_pages/recent_tab_helper.h
diff --git a/chrome/browser/android/offline_pages/recent_tab_helper.h b/chrome/browser/android/offline_pages/recent_tab_helper.h
index 6aa442ec8151aef2fcab62a284ac2dbe0f60736c..d640c439443889ec1a090b2e4369b3130dd66885 100644
--- a/chrome/browser/android/offline_pages/recent_tab_helper.h
+++ b/chrome/browser/android/offline_pages/recent_tab_helper.h
@@ -57,50 +57,33 @@ class RecentTabHelper
   };
   void SetDelegate(std::unique_ptr<RecentTabHelper::Delegate> delegate);
 
-  // Support for Download Page feature. The Download Page button does this:
-  // 1. Creates suspended request for Background Offliner.
-  // 2. Calls this method with properly filled ClientId.
-  // 3. This tab helper observes the page load and captures the page
-  //    if the load progresses far enough, as indicated by SnapshotController.
-  // 4. If capture is successful, this tab helper removes the suspended request.
-  //    Otherwise (navigation to other page, close tab) tab helper un-suspends
-  //    the request so the Background Offliner starts working on it.
-  // 5. If Chrome is killed at any point, next time Background Offliner loads
-  //    it converts all suspended requests from last session into active.
+  // Creates a request to download the current page with a properly filled
+  // |client_id| and valid |request_id| issued by RequestCoordinator from a
+  // suspended request. This method might be called multiple times for the same
+  // page at any point after its navigation commits. There are some important
+  // points about how requests are handled:
+  // a) While there is an ongoing request, new requests are ignored (no
+  //    overlapping snapshots).
+  // b) The page has to be sufficiently loaded to be considerer of minimum
+  //    quality for the request to be started immediately.
+  // c) Any calls made before the page is considered to have minimal quality
+  //    will be scheduled to be executed once that happens. The scheduled
+  //    request is considered "ongoing" for a) purposes.
+  // d) If the save operation is successful the dormant request with
+  //    RequestCoordinator is canceled; otherwise it is resumed. This logic is
+  //    robust to crashes.
+  // e) At the moment the page reaches high quality, if there was a successful
+  //    snapshot saved at a lower quality then a new snapshot is automatically
+  //    requested to replace it.
+  // Note #1: Page quality is determined by SnapshotController and is based on
+  // its assessment of "how much loaded" it is.
+  // Note #2: Currently this method only accepts download requests from the
+  // downloads namespace. Multiple clients are not supported.
   void ObserveAndDownloadCurrentPage(const ClientId& client_id,
                                      int64_t request_id);
 
  private:
-  // Keeps client_id/request_id that will be used for the offline snapshot.
-  struct SnapshotProgressInfo {
-   public:
-    // For a downloads snapshot request, where the |request_id| is defined.
-    SnapshotProgressInfo(const ClientId& client_id, int64_t request_id)
-        : client_id(client_id), request_id(request_id) {}
-
-    // For a last_n snapshot request.
-    explicit SnapshotProgressInfo(const ClientId& client_id)
-        : client_id(client_id) {}
-
-    bool IsForLastN();
-
-    // The ClientID to go with the offline page.
-    ClientId client_id;
-
-    // Id of the suspended request in Background Offliner. Used to un-suspend
-    // the request if the capture of the current page was not possible (e.g.
-    // the user navigated to another page before current one was loaded).
-    // 0 if this is a "last_n" info.
-    int64_t request_id = OfflinePageModel::kInvalidOfflineId;
-
-    // True if there was at least one snapshot successfully completed.
-    bool page_snapshot_completed = false;
-
-    // Expected snapshot quality should the saving succeed. This value is only
-    // valid if |page_snapshot_completed| is true.
-    SnapshotController::PageQuality expected_page_quality =
-        SnapshotController::PageQuality::POOR;
-  };
+  struct SnapshotProgressInfo;
 
   explicit RecentTabHelper(content::WebContents* web_contents);
   friend class content::WebContentsUserData<RecentTabHelper>;
@@ -113,11 +96,13 @@ class RecentTabHelper
   void SavePageCallback(SnapshotProgressInfo* snapshot_info,
                         OfflinePageModel::SavePageResult result,
                         int64_t offline_id);
-  void ReportSnapshotCompleted(SnapshotProgressInfo* snapshot_info);
+  void ReportSnapshotCompleted(SnapshotProgressInfo* snapshot_info,
+                               bool success);
   void ReportDownloadStatusToRequestCoordinator(
-      SnapshotProgressInfo* snapshot_info);
+      SnapshotProgressInfo* snapshot_info,
+      bool cancel_background_request);
   ClientId GetRecentPagesClientId() const;
-  void SaveSnapshotForDownloads();
+  void SaveSnapshotForDownloads(bool replace_latest);
 
   // Page model is a service, no ownership. Can be null - for example, in
   // case when tab is in incognito profile.
@@ -127,9 +112,17 @@ class RecentTabHelper
   // Not page-specific.
   bool snapshots_enabled_ = false;
 
-  // Snapshot progress information for a downloads triggered request. Null if
-  // downloads is not capturing or hasn't captured the current page.
-  std::unique_ptr<SnapshotProgressInfo> downloads_latest_snapshot_info_;
+  // Snapshot progress information for an ongoing snapshot requested by
+  // downloads. Null if there's no ongoing request.
+  std::unique_ptr<SnapshotProgressInfo> downloads_ongoing_snapshot_info_;
+
+  // This is set to true if the ongoing snapshot for downloads is waiting on the
+  // page to reach a minimal quality level to start.
+  bool downloads_snapshot_on_hold_ = false;
+
+  // Snapshot information for the last successful snapshot requested by
+  // downloads. Null no successful one has ever completed.

[Dmitry Titov, 2017/01/27 04:18:22]

nit: "Null no" -> "Null if no"

[carlosk, 2017/01/27 18:47:14]

Done.

+  std::unique_ptr<SnapshotProgressInfo> downloads_latest_saved_snapshot_info_;
 
   // Snapshot progress information for a last_n triggered request. Null if
   // last_n is not currently capturing the current page.