RecentTabHelper won't accept overlapping requests from Downloads anymore.

chrome/browser/android/offline_pages/recent_tab_helper_unittest.cc

 // Copyright 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 "base/memory/ptr_util.h"
 #include "base/run_loop.h"
 #include "base/strings/string16.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/android/offline_pages/offline_page_model_factory.h"
 #include "chrome/browser/android/offline_pages/request_coordinator_factory.h"
 #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"
@@ skipping 70 matching lines @@
   // 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);
 
+  ClientId NewDownloadClientId();
+
   RecentTabHelper* recent_tab_helper() const { return recent_tab_helper_; }
 
   OfflinePageModel* model() const { return model_; }
 
   // 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 : GetAllPages()) {
       if (page.offline_id == offline_id)
         return &page;
@@ skipping 137 matching lines @@
 }
 
 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();
 }
 
+ClientId RecentTabHelperTest::NewDownloadClientId() {
+  static int counter = 0;
+  return ClientId(kDownloadNamespace,
+                  std::string("id") + base::IntToString(++counter));
+}
+
 // Checks the test setup.
 TEST_F(RecentTabHelperTest, RecentTabHelperInstanceExists) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.Init();
   EXPECT_NE(nullptr, recent_tab_helper());
 }
 
 // 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) {
@@ skipping 41 matching lines @@
 // 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>(
       this, task_runner(), kTabId, false));
 
   NavigateAndCommit(kTestPageUrl);
   recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
   FastForwardSnapshotController();
   recent_tab_helper()->WasHidden();
-  recent_tab_helper()->ObserveAndDownloadCurrentPage(
-      ClientId("download", "id2"), 123L);
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(NewDownloadClientId(),
+                                                     123L);
   RunUntilIdle();
   EXPECT_TRUE(model()->is_loaded());
   // No page should be captured.
   EXPECT_EQ(0U, page_added_count());
   ASSERT_EQ(0U, GetAllPages().size());
 }
 
 // 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) {
@@ skipping 195 matching lines @@
   recent_tab_helper()->WasHidden();
   RunUntilIdle();
   EXPECT_EQ(2U, page_added_count());
   EXPECT_EQ(1U, model_removed_count());
   ASSERT_EQ(1U, GetAllPages().size());
   EXPECT_EQ(kTestPageUrl, GetAllPages()[0].url);
   int64_t first_offline_id = GetAllPages()[0].offline_id;
 
   // First snapshot request by downloads. Two offline pages are expected.
   const int64_t second_offline_id = first_offline_id + 1;
-  const ClientId second_client_id("download", "id2");
+  const ClientId second_client_id = NewDownloadClientId();
   recent_tab_helper()->ObserveAndDownloadCurrentPage(second_client_id,
                                                      second_offline_id);
   RunUntilIdle();
   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(second_client_id, second_page->client_id);
 
   // Second snapshot request by downloads. Three offline pages are expected.
   const int64_t third_offline_id = first_offline_id + 2;
-  const ClientId third_client_id("download", "id2");
+  const ClientId third_client_id = NewDownloadClientId();
   recent_tab_helper()->ObserveAndDownloadCurrentPage(third_client_id,
                                                      third_offline_id);
   RunUntilIdle();
   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(third_client_id, third_page->client_id);
 }
 
 // Simulates an error (disconnection) during the load of a page. Should end up
 // 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);
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(NewDownloadClientId(),
+                                                     123L);
   RunUntilIdle();
   EXPECT_TRUE(model()->is_loaded());
   ASSERT_EQ(0U, GetAllPages().size());
 }
 
 // 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());
   // No page should be captured.
   ASSERT_EQ(0U, GetAllPages().size());
 
-  recent_tab_helper()->ObserveAndDownloadCurrentPage(
-      ClientId("download", "id1"), 123L);
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(NewDownloadClientId(),
+                                                     123L);
   RunUntilIdle();
   // No page should be captured.
   ASSERT_EQ(1U, GetAllPages().size());
 }
 
 // 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);
-  const ClientId client_id("download", "id1");
+  const ClientId client_id = NewDownloadClientId();
   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();
@@ skipping 20 matching lines @@
 // 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());
   ASSERT_EQ(0U, GetAllPages().size());
 
-  const ClientId client_id("download", "id1");
+  const ClientId client_id = NewDownloadClientId();
   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);
 
   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");
+  const ClientId client_id = NewDownloadClientId();
   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");
+  const ClientId download_client_id = NewDownloadClientId();
   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);
@@ skipping 32 matching lines @@
   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());
 }
 
+// Simulates multiple download requests and verifies that overlapping requests
+// are ignored.
+TEST_F(RecentTabHelperTest, OverlappingDownloadRequestsAreIgnored) {
+  // Navigates and commits then make two download snapshot requests.
+  NavigateAndCommit(kTestPageUrl);
+  const ClientId client_id_1 = NewDownloadClientId();
+  const int64_t offline_id_1 = 153L;
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(client_id_1, offline_id_1);
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(NewDownloadClientId(),
+                                                     351L);
+
+  // Finish loading the page. Only the first request should be executed.
+  recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
+  FastForwardSnapshotController();
+  RunUntilIdle();
+  EXPECT_EQ(1U, page_added_count());
+  EXPECT_EQ(0U, model_removed_count());
+  ASSERT_EQ(1U, GetAllPages().size());
+  const OfflinePageItem& fist_page = GetAllPages()[0];
+  EXPECT_EQ(client_id_1, fist_page.client_id);
+  EXPECT_EQ(offline_id_1, fist_page.offline_id);
+
+  // Make two additional download snapshot requests. Again only the first should
+  // generate a snapshot.
+  const ClientId client_id_3 = NewDownloadClientId();
+  const int64_t offline_id_3 = 789L;
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(client_id_3, offline_id_3);
+  recent_tab_helper()->ObserveAndDownloadCurrentPage(NewDownloadClientId(),
+                                                     987L);
+  RunUntilIdle();
+  EXPECT_EQ(2U, page_added_count());
+  EXPECT_EQ(0U, model_removed_count());
+  ASSERT_EQ(2U, GetAllPages().size());
+  const OfflinePageItem* second_page = FindPageForOfflineId(offline_id_3);
+  ASSERT_TRUE(second_page);
+  EXPECT_EQ(client_id_3, second_page->client_id);
+  EXPECT_EQ(offline_id_3, second_page->offline_id);
+}
+
 }  // namespace offline_pages