Offline Page Cache uses user interaction triggers for saving pages.

chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc

Index: chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc
diff --git a/chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc b/chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc
index cf8b71ba71eb03d391dde994f41552fcc9e20cfb..c3b1f2f9de8454b3faeddeaf82d688312f13bce3 100644
--- a/chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc
+++ b/chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc
@@ -15,6 +15,7 @@
 #include "chrome/browser/android/offline_pages/test_offline_page_model_builder.h"
 #include "chrome/browser/android/offline_pages/test_request_coordinator_builder.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "components/offline_pages/core/client_namespace_constants.h"
 #include "components/offline_pages/core/offline_page_feature.h"
 #include "components/offline_pages/core/offline_page_item.h"
 #include "components/offline_pages/core/offline_page_model.h"
@@ -22,6 +23,8 @@
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/referrer.h"
+#include "content/public/test/web_contents_tester.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace offline_pages {
@@ -87,16 +90,21 @@ class RecentTabHelperTest
   // Moves forward the snapshot controller's task runner.
   void FastForwardSnapshotController();
 
+  // Navigates to the URL and commit as if it has been typed in the address bar.
+  // Note: we need this to simulate navigations to the same URL that more like a
+  // reload and not same page. NavigateAndCommit simulates a click on a link
+  // and when reusing the same URL that will be considered a same page
+  // navigation.
+  void NavigateAndCommitTyped(const GURL& url);
+
   RecentTabHelper* recent_tab_helper() const { return recent_tab_helper_; }
 
   OfflinePageModel* model() const { return model_; }
 
-  const std::vector<OfflinePageItem>& all_pages() { return all_pages_; }
-
   // Returns a OfflinePageItem pointer from |all_pages| that matches the
   // provided |offline_id|. If a match is not found returns nullptr.
   const OfflinePageItem* FindPageForOfflineId(int64_t offline_id) {
-    for (const OfflinePageItem& page : all_pages_) {
+    for (const OfflinePageItem& page : GetAllPages()) {
       if (page.offline_id == offline_id)
         return &page;
     }
@@ -111,13 +119,17 @@ class RecentTabHelperTest
   size_t model_removed_count() { return model_removed_count_; }
 
   // OfflinePageModel::Observer
-  void OfflinePageModelLoaded(OfflinePageModel* model) override { }
+  void OfflinePageModelLoaded(OfflinePageModel* model) override {
+    all_pages_needs_updating_ = true;
+  }
   void OfflinePageAdded(OfflinePageModel* model,
                         const OfflinePageItem& added_page) override {
     page_added_count_++;
+    all_pages_needs_updating_ = true;
   }
   void OfflinePageDeleted(int64_t, const offline_pages::ClientId&) override {
     model_removed_count_++;
+    all_pages_needs_updating_ = true;
   }
 
   // OfflinePageTestArchiver::Observer
@@ -131,6 +143,7 @@ class RecentTabHelperTest
   size_t page_added_count_;
   size_t model_removed_count_;
   std::vector<OfflinePageItem> all_pages_;
+  bool all_pages_needs_updating_;
   scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
   base::test::ScopedFeatureList scoped_feature_list_;
 
@@ -173,6 +186,7 @@ RecentTabHelperTest::RecentTabHelperTest()
       model_(nullptr),
       page_added_count_(0),
       model_removed_count_(0),
+      all_pages_needs_updating_(true),
       task_runner_(new base::TestMockTimeTaskRunner),
       weak_ptr_factory_(this) {}
 
@@ -209,10 +223,12 @@ void RecentTabHelperTest::FailLoad(const GURL& url) {
 }
 
 const std::vector<OfflinePageItem>& RecentTabHelperTest::GetAllPages() {
-  model()->GetAllPages(
-      base::Bind(&RecentTabHelperTest::OnGetAllPagesDone,
-                 weak_ptr_factory_.GetWeakPtr()));
-  RunUntilIdle();
+  if (all_pages_needs_updating_) {
+    model()->GetAllPages(base::Bind(&RecentTabHelperTest::OnGetAllPagesDone,
+                                    weak_ptr_factory_.GetWeakPtr()));
+    RunUntilIdle();
+    all_pages_needs_updating_ = false;
+  }
   return all_pages_;
 }
 
@@ -230,26 +246,66 @@ void RecentTabHelperTest::FastForwardSnapshotController() {
   task_runner_->FastForwardBy(base::TimeDelta::FromMilliseconds(kLongDelayMs));
 }
 
+void RecentTabHelperTest::NavigateAndCommitTyped(const GURL& url) {
+  controller().LoadURL(url, content::Referrer(), ui::PAGE_TRANSITION_TYPED,
+                       std::string());
+  content::WebContentsTester* web_contents_tester =
+      content::WebContentsTester::For(web_contents());
+  web_contents_tester->CommitPendingNavigation();
+}
+
 // Checks the test setup.
-TEST_F(RecentTabHelperTest, Basic) {
+TEST_F(RecentTabHelperTest, RecentTabHelperInstanceExists) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.Init();
   EXPECT_NE(nullptr, recent_tab_helper());
 }
 
-// Loads a page and verifies that a snapshot is created.
-TEST_F(RecentTabHelperTest, SimpleCapture) {
+// Fully loads a page then simulates the tab being hidden. Verifies that a
+// snapshot is created only when the latter happens.
+TEST_F(RecentTabHelperTest, LastNCaptureAfterLoad) {
+  // Navigate and finish loading. Nothing should be saved.
   NavigateAndCommit(kTestPageUrl);
   recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
+  // Move the snapshot controller's time forward so it gets past timeouts.
   FastForwardSnapshotController();
   RunUntilIdle();
   EXPECT_TRUE(model()->is_loaded());
-  GetAllPages();
-  ASSERT_EQ(1U, all_pages().size());
-  EXPECT_EQ(kTestPageUrl, all_pages()[0].url);
+  EXPECT_EQ(0U, page_added_count());
+  ASSERT_EQ(0U, GetAllPages().size());
+
+  // Tab is hidden with a fully loaded page. A snapshot save should happen.
+  recent_tab_helper()->WasHidden();
+  RunUntilIdle();
+  EXPECT_TRUE(model()->is_loaded());
+  EXPECT_EQ(1U, page_added_count());
+  ASSERT_EQ(1U, GetAllPages().size());
+  EXPECT_EQ(kTestPageUrl, GetAllPages()[0].url);
+  EXPECT_EQ(kLastNNamespace, GetAllPages()[0].client_id.name_space);
+}
+
+// Simulates the tab being hidden too early in the page loading so that a
+// snapshot should not be created.
+TEST_F(RecentTabHelperTest, NoLastNCaptureIfTabHiddenTooEarlyInPageLoad) {
+  // Commit the navigation and hide the tab. Nothing should be saved.
+  NavigateAndCommit(kTestPageUrl);
+  recent_tab_helper()->WasHidden();
+  RunUntilIdle();
+  EXPECT_TRUE(model()->is_loaded());
+  EXPECT_EQ(0U, page_added_count());
+  ASSERT_EQ(0U, GetAllPages().size());
+
+  // Then allow the page to fully load. Nothing should be saved.
+  recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
+  // Move the snapshot controller's time forward so it gets past timeouts.
+  FastForwardSnapshotController();
+  RunUntilIdle();
+  EXPECT_EQ(0U, page_added_count());
+  ASSERT_EQ(0U, GetAllPages().size());
 }
 
-// Checks that WebContents with no tab IDs are properly ignored.
+// Checks that WebContents with no tab IDs have snapshot requests properly
+// ignored from both last_n and downloads.
 TEST_F(RecentTabHelperTest, NoTabIdNoCapture) {
   // Create delegate that returns 'false' as TabId retrieval result.
   recent_tab_helper()->SetDelegate(base::MakeUnique<TestDelegate>(
@@ -258,66 +314,72 @@ TEST_F(RecentTabHelperTest, NoTabIdNoCapture) {
   NavigateAndCommit(kTestPageUrl);
   recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
   FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(
+      ClientId("download", "id2"), 123L);
   RunUntilIdle();
   EXPECT_TRUE(model()->is_loaded());
-  GetAllPages();
   // No page should be captured.
-  ASSERT_EQ(0U, all_pages().size());
+  EXPECT_EQ(0U, page_added_count());
+  ASSERT_EQ(0U, GetAllPages().size());
 }
 
-// Triggers two snapshot captures during a single page load. Should end up with
-// one offline page (the 2nd snapshot should be kept).
+// Triggers two last_n snapshot captures during a single page load. Should end
+// up with one snapshot, the 1st being replaced by the 2nd.
 TEST_F(RecentTabHelperTest, TwoCapturesSamePageLoad) {
   NavigateAndCommit(kTestPageUrl);
-  // Triggers snapshot after a time delay.
+  // Set page loading state to the 1st snapshot-able stage. No capture so far.
   recent_tab_helper()->DocumentAvailableInMainFrame();
+  FastForwardSnapshotController();
   RunUntilIdle();
   EXPECT_TRUE(model()->is_loaded());
   EXPECT_EQ(0U, page_added_count());
-  // Move the snapshot controller's time forward so it gets past timeouts.
-  FastForwardSnapshotController();
+
+  // Tab is hidden and a snapshot should be saved.
+  recent_tab_helper()->WasHidden();
   RunUntilIdle();
   EXPECT_EQ(1U, page_added_count());
   EXPECT_EQ(0U, model_removed_count());
-  GetAllPages();
-  ASSERT_EQ(1U, all_pages().size());
-  EXPECT_EQ(kTestPageUrl, all_pages()[0].url);
-  int64_t first_offline_id = all_pages()[0].offline_id;
+  ASSERT_EQ(1U, GetAllPages().size());
+  EXPECT_EQ(kTestPageUrl, GetAllPages()[0].url);
+  int64_t first_offline_id = GetAllPages()[0].offline_id;
 
-  // Triggers snapshot after a time delay.
+  // Set page loading state to the 2nd and last snapshot-able stage. No new
+  // capture should happen.
   recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
-  // Move the snapshot controller's time forward so it gets past timeouts.
   FastForwardSnapshotController();
   RunUntilIdle();
+  EXPECT_EQ(1U, page_added_count());
+  EXPECT_EQ(0U, model_removed_count());
+  ASSERT_EQ(1U, GetAllPages().size());
+
+  // Tab is hidden again. At this point a higher quality snapshot is expected so
+  // a new one should be captured and replace the previous one.
+  recent_tab_helper()->WasHidden();
+  RunUntilIdle();
   EXPECT_EQ(2U, page_added_count());
   EXPECT_EQ(1U, model_removed_count());
-  // the same page should be simply overridden.
-  GetAllPages();
-  ASSERT_EQ(1U, all_pages().size());
-  EXPECT_EQ(kTestPageUrl, all_pages()[0].url);
-  EXPECT_NE(first_offline_id, all_pages()[0].offline_id);
+  ASSERT_EQ(1U, GetAllPages().size());
+  EXPECT_EQ(kTestPageUrl, GetAllPages()[0].url);
+  EXPECT_NE(first_offline_id, GetAllPages()[0].offline_id);
 }
 
-// Triggers two snapshot captures during a single page load, where the 2nd one
+// Triggers two last_n captures during a single page load, where the 2nd capture
 // fails. Should end up with one offline page (the 1st, successful snapshot
 // should be kept).
 // TODO(carlosk): re-enable once https://crbug.com/655697 is fixed, again.
-TEST_F(RecentTabHelperTest, DISABLED_TwoCapturesSamePageLoadSecondFails) {
+TEST_F(RecentTabHelperTest, DISABLED_TwoCapturesWhere2ndFailsSamePageLoad) {
+  // Navigate and load until the 1st stage. Tab hidden should trigger a capture.
   NavigateAndCommit(kTestPageUrl);
-  // Triggers snapshot after a time delay.
   recent_tab_helper()->DocumentAvailableInMainFrame();
-  RunUntilIdle();
-  EXPECT_TRUE(model()->is_loaded());
-  EXPECT_EQ(0U, page_added_count());
-  // Move the snapshot controller's time forward so it gets past timeouts.
   FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
   RunUntilIdle();
   EXPECT_EQ(1U, page_added_count());
   EXPECT_EQ(0U, model_removed_count());
-  GetAllPages();
-  ASSERT_EQ(1U, all_pages().size());
-  EXPECT_EQ(kTestPageUrl, all_pages()[0].url);
-  int64_t first_offline_id = all_pages()[0].offline_id;
+  ASSERT_EQ(1U, GetAllPages().size());
+  EXPECT_EQ(kTestPageUrl, GetAllPages()[0].url);
+  int64_t first_offline_id = GetAllPages()[0].offline_id;
 
   // Sets a new delegate that will make the second snapshot fail.
   TestDelegate* failing_delegate =
@@ -329,72 +391,68 @@ TEST_F(RecentTabHelperTest, DISABLED_TwoCapturesSamePageLoadSecondFails) {
   recent_tab_helper()->SetDelegate(
       std::unique_ptr<TestDelegate>(failing_delegate));
 
-  // Triggers snapshot after a time delay.
+  // Advance loading to the 2nd and final stage and then hide the tab. A new
+  // capture is requested but its creation will fail. The exact same snapshot
+  // from before should still be available.
   recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
-  // Move the snapshot controller's time forward so it gets past timeouts.
   FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
   RunUntilIdle();
   EXPECT_EQ(1U, page_added_count());
   EXPECT_EQ(0U, model_removed_count());
-  // The exact same page should still be available.
-  GetAllPages();
-  ASSERT_EQ(1U, all_pages().size());
-  EXPECT_EQ(kTestPageUrl, all_pages()[0].url);
-  EXPECT_EQ(first_offline_id, all_pages()[0].offline_id);
+  ASSERT_EQ(1U, GetAllPages().size());
+  EXPECT_EQ(kTestPageUrl, GetAllPages()[0].url);
+  EXPECT_EQ(first_offline_id, GetAllPages()[0].offline_id);
 }
 
-// Triggers two snapshot captures for two different page loads for the same URL.
-// Should end up with one offline page (snapshot from the 2nd load).
+// Triggers two last_n captures for two different loads of the same URL (aka
+// reload). Should end up with a single snapshot (from the 2nd load).
 TEST_F(RecentTabHelperTest, TwoCapturesDifferentPageLoadsSameUrl) {
+  // Fully load the page. Hide the tab and check for a snapshot.
   NavigateAndCommit(kTestPageUrl);
-  // Triggers snapshot after a time delay.
-  recent_tab_helper()->DocumentAvailableInMainFrame();
-  RunUntilIdle();
-  EXPECT_TRUE(model()->is_loaded());
-  EXPECT_EQ(0U, page_added_count());
-  // Move the snapshot controller's time forward so it gets past timeouts.
+  recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
   FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
   RunUntilIdle();
   EXPECT_EQ(1U, page_added_count());
   EXPECT_EQ(0U, model_removed_count());
-  GetAllPages();
-  ASSERT_EQ(1U, all_pages().size());
-  EXPECT_EQ(kTestPageUrl, all_pages()[0].url);
-  int64_t first_offline_id = all_pages()[0].offline_id;
+  ASSERT_EQ(1U, GetAllPages().size());
+  EXPECT_EQ(kTestPageUrl, GetAllPages()[0].url);
+  int64_t first_offline_id = GetAllPages()[0].offline_id;
 
-  NavigateAndCommit(kTestPageUrl);
-  // Triggers snapshot after a time delay.
-  recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
-  // Move the snapshot controller's time forward so it gets past timeouts.
+  // Navigate with the same URL until the page is minimally loaded then hide the
+  // tab. The previous snapshot should be removed and a new one taken.
+  NavigateAndCommitTyped(kTestPageUrl);
+  recent_tab_helper()->DocumentAvailableInMainFrame();
   FastForwardSnapshotController();
   RunUntilIdle();
+  EXPECT_EQ(1U, page_added_count());
+  EXPECT_EQ(0U, model_removed_count());
+  ASSERT_EQ(1U, GetAllPages().size());
+
+  recent_tab_helper()->WasHidden();
+  RunUntilIdle();
   EXPECT_EQ(2U, page_added_count());
   EXPECT_EQ(1U, model_removed_count());
-  // the same page should be simply overridden.
-  GetAllPages();
-  ASSERT_EQ(1U, all_pages().size());
-  EXPECT_EQ(kTestPageUrl, all_pages()[0].url);
-  EXPECT_NE(first_offline_id, all_pages()[0].offline_id);
+  ASSERT_EQ(1U, GetAllPages().size());
+  EXPECT_EQ(kTestPageUrl, GetAllPages()[0].url);
+  EXPECT_NE(first_offline_id, GetAllPages()[0].offline_id);
 }
 
-// Triggers two snapshot captures for two different page loads for the same URL,
-// where the 2nd one fails. Should end up with no offline pages (privacy driven
-// decision).
-TEST_F(RecentTabHelperTest, TwoCapturesDifferentPageLoadsSameUrlSecondFails) {
+// Triggers two last_n captures for two different page loads of the same URL
+// (aka reload), where the 2nd capture fails. Should end up with no offline
+// pages (a privacy driven decision).
+TEST_F(RecentTabHelperTest, TwoCapturesWhere2ndFailsDifferentPageLoadsSameUrl) {
+  // Fully load the page then hide the tab. A capture is expected.
   NavigateAndCommit(kTestPageUrl);
-  // Triggers snapshot after a time delay.
-  recent_tab_helper()->DocumentAvailableInMainFrame();
-  RunUntilIdle();
-  EXPECT_TRUE(model()->is_loaded());
-  EXPECT_EQ(0U, page_added_count());
-  // Move the snapshot controller's time forward so it gets past timeouts.
+  recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
   FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
   RunUntilIdle();
   EXPECT_EQ(1U, page_added_count());
   EXPECT_EQ(0U, model_removed_count());
-  GetAllPages();
-  ASSERT_EQ(1U, all_pages().size());
-  EXPECT_EQ(kTestPageUrl, all_pages()[0].url);
+  ASSERT_EQ(1U, GetAllPages().size());
+  EXPECT_EQ(kTestPageUrl, GetAllPages()[0].url);
 
   // Sets a new delegate that will make the second snapshot fail.
   TestDelegate* failing_delegate =
@@ -406,149 +464,282 @@ TEST_F(RecentTabHelperTest, TwoCapturesDifferentPageLoadsSameUrlSecondFails) {
   recent_tab_helper()->SetDelegate(
       std::unique_ptr<TestDelegate>(failing_delegate));
 
-  NavigateAndCommit(kTestPageUrl);
-  // Triggers snapshot after a time delay.
+  // Fully load the page once more then hide the tab again. A capture happens
+  // and fails but no snapshot should remain.
+  NavigateAndCommitTyped(kTestPageUrl);
   recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
-  // Move the snapshot controller's time forward so it gets past timeouts.
   FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
   RunUntilIdle();
   EXPECT_EQ(1U, page_added_count());
   EXPECT_EQ(1U, model_removed_count());
-  // the same page should be simply overridden.
-  GetAllPages();
-  ASSERT_EQ(0U, all_pages().size());
+  ASSERT_EQ(0U, GetAllPages().size());
 }
 
-// Triggers two snapshot captures for two different page loads and URLs. Should
-// end up with one offline page.
-TEST_F(RecentTabHelperTest, TwoCapturesDifferentPageLoadsAndUrls) {
+// Triggers two last_n captures for two different page loads of different URLs.
+// Should end up with a single snapshot of the last page.
+TEST_F(RecentTabHelperTest, TwoCapturesDifferentPageLoadsDifferentUrls) {
+  // Fully load the first URL then hide the tab and check for a snapshot.
   NavigateAndCommit(kTestPageUrl);
-  // Triggers snapshot after a time delay.
-  recent_tab_helper()->DocumentAvailableInMainFrame();
-  RunUntilIdle();
-  EXPECT_TRUE(model()->is_loaded());
-  EXPECT_EQ(0U, page_added_count());
-  // Move the snapshot controller's time forward so it gets past timeouts.
+  recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
   FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
   RunUntilIdle();
   EXPECT_EQ(1U, page_added_count());
   EXPECT_EQ(0U, model_removed_count());
-  GetAllPages();
-  ASSERT_EQ(1U, all_pages().size());
-  EXPECT_EQ(kTestPageUrl, all_pages()[0].url);
+  ASSERT_EQ(1U, GetAllPages().size());
+  EXPECT_EQ(kTestPageUrl, GetAllPages()[0].url);
 
-  NavigateAndCommit(kTestPageUrlOther);
-  // Triggers snapshot after a time delay.
+  // Fully load the second URL then hide the tab and check for a single snapshot
+  // of the new page.
+  NavigateAndCommitTyped(kTestPageUrlOther);
   recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
-  // Move the snapshot controller's time forward so it gets past timeouts.
   FastForwardSnapshotController();
   RunUntilIdle();
+  EXPECT_EQ(1U, page_added_count());
+  EXPECT_EQ(0U, model_removed_count());
+  ASSERT_EQ(1U, GetAllPages().size());
+
+  recent_tab_helper()->WasHidden();
+  RunUntilIdle();
   EXPECT_EQ(2U, page_added_count());
   EXPECT_EQ(1U, model_removed_count());
-  // the same page should be simply overridden.
-  GetAllPages();
-  ASSERT_EQ(1U, all_pages().size());
-  EXPECT_EQ(kTestPageUrlOther, all_pages()[0].url);
+  ASSERT_EQ(1U, GetAllPages().size());
+  EXPECT_EQ(kTestPageUrlOther, GetAllPages()[0].url);
 }
 
-// Triggers two snapshot captures via the download after a page was loaded page
-// and saved twice by last_n. Should end up with three offline pages: two from
-// downloads (which shouldn't replace each other) and one from last_n.
-TEST_F(RecentTabHelperTest, TwoDownloadCapturesInARowSamePage) {
+// Fully loads a page where last_n captures two snapshots. Then triggers two
+// snapshot requests by downloads. Should end up with three offline pages: one
+// from last_n (2nd replaces the 1st) and two from downloads (which shouldn't
+// replace each other).
+TEST_F(RecentTabHelperTest, TwoLastNAndTwoDownloadCapturesSamePage) {
+  // Fully loads the page with intermediary steps where the tab is hidden. Then
+  // check that two last_n snapshots were created but only one was kept.
   NavigateAndCommit(kTestPageUrl);
-  // Executes a regular load with snapshots taken by last_n.
   recent_tab_helper()->DocumentAvailableInMainFrame();
-  RunUntilIdle();
   FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
   RunUntilIdle();
   recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
   FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
   RunUntilIdle();
-  // Checks that two last_n snapshots were created but only one was kept.
   EXPECT_EQ(2U, page_added_count());
   EXPECT_EQ(1U, model_removed_count());
-  GetAllPages();
-  ASSERT_EQ(1U, all_pages().size());
-  EXPECT_EQ(kTestPageUrl, all_pages()[0].url);
-  int64_t first_offline_id = all_pages()[0].offline_id;
+  ASSERT_EQ(1U, GetAllPages().size());
+  EXPECT_EQ(kTestPageUrl, GetAllPages()[0].url);
+  int64_t first_offline_id = GetAllPages()[0].offline_id;
 
-  // Request the 1st offline download.
+  // First snapshot request by downloads. Two offline pages are expected.
   const int64_t second_offline_id = first_offline_id + 1;
-  recent_tab_helper()->ObserveAndDownloadCurrentPage(
-      ClientId("download", "id2"), second_offline_id);
+  const ClientId second_client_id("download", "id2");
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(second_client_id,
+                                                     second_offline_id);
   RunUntilIdle();
-  GetAllPages();
-  // Checks that both the previous last_n and download snapshots are present.
-  ASSERT_EQ(2U, all_pages().size());
+  EXPECT_EQ(3U, page_added_count());
+  EXPECT_EQ(1U, model_removed_count());
+  ASSERT_EQ(2U, GetAllPages().size());
   EXPECT_NE(nullptr, FindPageForOfflineId(first_offline_id));
   const OfflinePageItem* second_page = FindPageForOfflineId(second_offline_id);
   ASSERT_NE(nullptr, second_page);
   EXPECT_EQ(kTestPageUrl, second_page->url);
-  EXPECT_EQ("download", second_page->client_id.name_space);
-  EXPECT_EQ("id2", second_page->client_id.id);
+  EXPECT_EQ(second_client_id, second_page->client_id);
 
-  // Request the 2nd offline download.
+  // Second snapshot request by downloads. Three offline pages are expected.
   const int64_t third_offline_id = first_offline_id + 2;
-  recent_tab_helper()->ObserveAndDownloadCurrentPage(
-      ClientId("download", "id3"), third_offline_id);
+  const ClientId third_client_id("download", "id2");
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(third_client_id,
+                                                     third_offline_id);
   RunUntilIdle();
-  GetAllPages();
-  ASSERT_EQ(3U, all_pages().size());
-  // Checks that the previous last_n and download snapshots are still present
-  // and the new downloaded one was added.
+  EXPECT_EQ(4U, page_added_count());
+  EXPECT_EQ(1U, model_removed_count());
+  ASSERT_EQ(3U, GetAllPages().size());
   EXPECT_NE(nullptr, FindPageForOfflineId(first_offline_id));
   EXPECT_NE(nullptr, FindPageForOfflineId(second_offline_id));
   const OfflinePageItem* third_page = FindPageForOfflineId(third_offline_id);
   ASSERT_NE(nullptr, third_page);
   EXPECT_EQ(kTestPageUrl, third_page->url);
-  EXPECT_EQ("download", third_page->client_id.name_space);
-  EXPECT_EQ("id3", third_page->client_id.id);
+  EXPECT_EQ(third_client_id, third_page->client_id);
 }
 
 // Simulates an error (disconnection) during the load of a page. Should end up
-// with no offline pages.
+// with no offline pages for any requester.
 TEST_F(RecentTabHelperTest, NoCaptureOnErrorPage) {
   FailLoad(kTestPageUrl);
   recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
   FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(
+      ClientId("download", "id1"), 123L);
   RunUntilIdle();
   EXPECT_TRUE(model()->is_loaded());
-  GetAllPages();
-  ASSERT_EQ(0U, all_pages().size());
+  ASSERT_EQ(0U, GetAllPages().size());
 }
 
-// Checks that no snapshots are created if the Offline Page Cache feature is
-// disabled.
-TEST_F(RecentTabHelperTest, FeatureNotEnabled) {
+// Checks that last_n snapshots are not created if the feature is disabled.
+// Download requests should still work.
+TEST_F(RecentTabHelperTest, LastNFeatureNotEnabled) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.Init();
   NavigateAndCommit(kTestPageUrl);
   recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
   FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
   RunUntilIdle();
   EXPECT_TRUE(model()->is_loaded());
-  GetAllPages();
   // No page should be captured.
-  ASSERT_EQ(0U, all_pages().size());
+  ASSERT_EQ(0U, GetAllPages().size());
+
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(
+      ClientId("download", "id1"), 123L);
+  RunUntilIdle();
+  // No page should be captured.
+  ASSERT_EQ(1U, GetAllPages().size());
 }
 
-// Simulates a download request to offline the current page. Should end up with
-// one offline pages.
-TEST_F(RecentTabHelperTest, DownloadRequest) {
+// Simulates a download request to offline the current page made early during
+// loading. Should execute two captures but only the final one is kept.
+TEST_F(RecentTabHelperTest, DownloadRequestEarlyInLoad) {
+  // Commit the navigation and request the snapshot from downloads. No captures
+  // so far.
   NavigateAndCommit(kTestPageUrl);
-  recent_tab_helper()->ObserveAndDownloadCurrentPage(
-      ClientId("download", "id1"), 153L);
+  const ClientId client_id("download", "id1");
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(client_id, 153L);
+  FastForwardSnapshotController();
+  RunUntilIdle();
+  EXPECT_TRUE(model()->is_loaded());
+  ASSERT_EQ(0U, GetAllPages().size());
+
+  // Minimally load the page. First capture should occur.
+  recent_tab_helper()->DocumentAvailableInMainFrame();
+  FastForwardSnapshotController();
+  RunUntilIdle();
+  ASSERT_EQ(1U, GetAllPages().size());
+  const OfflinePageItem& early_page = GetAllPages()[0];
+  EXPECT_EQ(kTestPageUrl, early_page.url);
+  EXPECT_EQ(client_id, early_page.client_id);
+  EXPECT_EQ(153L, early_page.offline_id);
+
+  // Fully load the page. A second capture should replace the first one.
   recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
   FastForwardSnapshotController();
   RunUntilIdle();
+  EXPECT_EQ(2U, page_added_count());
+  EXPECT_EQ(1U, model_removed_count());
+  ASSERT_EQ(1U, GetAllPages().size());
+  const OfflinePageItem& later_page = GetAllPages()[0];
+  EXPECT_EQ(kTestPageUrl, later_page.url);
+  EXPECT_EQ(client_id, later_page.client_id);
+  EXPECT_EQ(153L, later_page.offline_id);
+}
+
+// Simulates a download request to offline the current page made when the page
+// is minimally loaded. Should execute two captures but only the final one is
+// kept.
+TEST_F(RecentTabHelperTest, DownloadRequestLaterInLoad) {
+  NavigateAndCommit(kTestPageUrl);
+  recent_tab_helper()->DocumentAvailableInMainFrame();
+  FastForwardSnapshotController();
+  RunUntilIdle();
   EXPECT_TRUE(model()->is_loaded());
-  GetAllPages();
-  ASSERT_EQ(1U, all_pages().size());
-  const OfflinePageItem& page = all_pages()[0];
+  ASSERT_EQ(0U, GetAllPages().size());
+
+  const ClientId client_id("download", "id1");
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(client_id, 153L);
+  RunUntilIdle();
+  ASSERT_EQ(1U, GetAllPages().size());
+  const OfflinePageItem& page = GetAllPages()[0];
   EXPECT_EQ(kTestPageUrl, page.url);
-  EXPECT_EQ("download", page.client_id.name_space);
-  EXPECT_EQ("id1", page.client_id.id);
+  EXPECT_EQ(client_id, page.client_id);
   EXPECT_EQ(153L, page.offline_id);
+
+  recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
+  FastForwardSnapshotController();
+  RunUntilIdle();
+  EXPECT_EQ(2U, page_added_count());
+  EXPECT_EQ(1U, model_removed_count());
+  ASSERT_EQ(1U, GetAllPages().size());
+}
+
+// Simulates a download request to offline the current page made after loading
+// is completed. Should end up with one offline pages.
+TEST_F(RecentTabHelperTest, DownloadRequestAfterFullyLoad) {
+  NavigateAndCommit(kTestPageUrl);
+  recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
+  FastForwardSnapshotController();
+  RunUntilIdle();
+  EXPECT_TRUE(model()->is_loaded());
+  ASSERT_EQ(0U, GetAllPages().size());
+
+  const ClientId client_id("download", "id1");
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(client_id, 153L);
+  RunUntilIdle();
+  ASSERT_EQ(1U, GetAllPages().size());
+  const OfflinePageItem& page = GetAllPages()[0];
+  EXPECT_EQ(kTestPageUrl, page.url);
+  EXPECT_EQ(client_id, page.client_id);
+  EXPECT_EQ(153L, page.offline_id);
+}
+
+// Simulates requests coming from last_n and downloads at the same time for a
+// fully loaded page.
+TEST_F(RecentTabHelperTest, SimultaneousCapturesFromLastNAndDownloads) {
+  NavigateAndCommit(kTestPageUrl);
+  recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
+  FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
+  const int64_t download_offline_id = 153L;
+  const ClientId download_client_id("download", "id1");
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(download_client_id,
+                                                     download_offline_id);
+  RunUntilIdle();
+  ASSERT_EQ(2U, GetAllPages().size());
+
+  const OfflinePageItem* downloads_page =
+      FindPageForOfflineId(download_offline_id);
+  ASSERT_TRUE(downloads_page);
+  EXPECT_EQ(kTestPageUrl, downloads_page->url);
+  EXPECT_EQ(download_client_id, downloads_page->client_id);
+
+  const OfflinePageItem& last_n_page =
+      GetAllPages()[0].offline_id != download_offline_id ? GetAllPages()[0]
+                                                         : GetAllPages()[1];
+  EXPECT_EQ(kTestPageUrl, last_n_page.url);
+  EXPECT_EQ(kLastNNamespace, last_n_page.client_id.name_space);
+}
+
+// Simulates multiple tab hidden events -- triggers for last_n snapshots --
+// happening at the same loading stages. The duplicate events should not cause
+// new snapshots to be saved.
+TEST_F(RecentTabHelperTest, DuplicateTabHiddenEventsShouldNotTriggerSnapshots) {
+  NavigateAndCommit(kTestPageUrl);
+  recent_tab_helper()->DocumentAvailableInMainFrame();
+  FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
+  RunUntilIdle();
+  EXPECT_EQ(1U, page_added_count());
+  EXPECT_EQ(0U, model_removed_count());
+  ASSERT_EQ(1U, GetAllPages().size());
+
+  recent_tab_helper()->WasHidden();
+  RunUntilIdle();
+  EXPECT_EQ(1U, page_added_count());
+  EXPECT_EQ(0U, model_removed_count());
+  ASSERT_EQ(1U, GetAllPages().size());
+
+  recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
+  FastForwardSnapshotController();
+  recent_tab_helper()->WasHidden();
+  RunUntilIdle();
+  EXPECT_EQ(2U, page_added_count());
+  EXPECT_EQ(1U, model_removed_count());
+  ASSERT_EQ(1U, GetAllPages().size());
+
+  recent_tab_helper()->WasHidden();
+  RunUntilIdle();
+  EXPECT_EQ(2U, page_added_count());
+  EXPECT_EQ(1U, model_removed_count());
+  ASSERT_EQ(1U, GetAllPages().size());
 }
 
 }  // namespace offline_pages