Fix snapshots from Downloads being deleted by last_n.

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/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/test/base/chrome_render_view_host_test_harness.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"
 #include "components/offline_pages/core/offline_page_test_archiver.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace offline_pages {
 
 namespace {
 const GURL kTestPageUrl("http://mystery.site/foo.html");
 const GURL kTestPageUrlOther("http://crazy.site/foo_other.html");
 const int kTabId = 153;
-}
+}  // namespace
 
 class TestDelegate: public RecentTabHelper::Delegate {
  public:
   const size_t kArchiveSizeToReport = 1234;
 
   explicit TestDelegate(
       OfflinePageTestArchiver::Observer* observer,
       scoped_refptr<base::SingleThreadTaskRunner> task_runner,
       int tab_id,
       bool tab_id_result);
@@ skipping 27 matching lines @@
 
 class RecentTabHelperTest
     : public ChromeRenderViewHostTestHarness,
       public OfflinePageModel::Observer,
       public OfflinePageTestArchiver::Observer {
  public:
   RecentTabHelperTest();
   ~RecentTabHelperTest() override {}
 
   void SetUp() override;
+  void TearDown() override;
   const std::vector<OfflinePageItem>& GetAllPages();
 
   void FailLoad(const GURL& url);
 
   // Runs default thread.
   void RunUntilIdle();
   // Moves forward the snapshot controller's task runner.
   void FastForwardSnapshotController();
 
   RecentTabHelper* recent_tab_helper() const { return recent_tab_helper_; }
 
   OfflinePageModel* model() const { return model_; }
 
   const std::vector<OfflinePageItem>& all_pages() { return all_pages_; }
 
+  // Returns the index in |all_pages| of the OfflinePageItem that matches the
+  // provided |offline_id|. If a match is not found returns -1.
+  int FindPageIndexForOfflineId(int64_t offline_id) {
+    for (size_t i = 0; i < all_pages_.size(); ++i) {
+      if (all_pages_[i].offline_id == offline_id)
+        return i;
+    }
+    return -1;
+  }
+
   scoped_refptr<base::TestMockTimeTaskRunner>& task_runner() {
     return task_runner_;
   }
 
   size_t page_added_count() { return page_added_count_; }
   size_t model_removed_count() { return model_removed_count_; }
 
   // OfflinePageModel::Observer
   void OfflinePageModelLoaded(OfflinePageModel* model) override { }
   void OfflinePageAdded(OfflinePageModel* model,
@@ skipping 54 matching lines @@
 
 RecentTabHelperTest::RecentTabHelperTest()
     : recent_tab_helper_(nullptr),
       model_(nullptr),
       page_added_count_(0),
       model_removed_count_(0),
       task_runner_(new base::TestMockTimeTaskRunner),
       weak_ptr_factory_(this) {}
 
 void RecentTabHelperTest::SetUp() {
-  content::RenderViewHostTestHarness::SetUp();
+  ChromeRenderViewHostTestHarness::SetUp();
 
   scoped_feature_list_.InitAndEnableFeature(kOffliningRecentPagesFeature);
   // Sets up the factory for testing.
   OfflinePageModelFactory::GetInstance()->SetTestingFactoryAndUse(
       browser_context(), BuildTestOfflinePageModel);
   RunUntilIdle();
 
   RecentTabHelper::CreateForWebContents(web_contents());
   recent_tab_helper_ =
       RecentTabHelper::FromWebContents(web_contents());
 
   recent_tab_helper_->SetDelegate(base::MakeUnique<TestDelegate>(
       this, task_runner(), kTabId, true));
 
   model_ = OfflinePageModelFactory::GetForBrowserContext(browser_context());
   model_->AddObserver(this);
+  RequestCoordinatorFactory::GetInstance()->ReturnNullptrForTesting(true);
+}
+
+void RecentTabHelperTest::TearDown() {
+  ChromeRenderViewHostTestHarness::TearDown();

[fgorski, 2016/12/13 17:17:38]

per my other comment you don't need this code, but if you did, I think this
isn't the right order for a tear down operation.

[carlosk, 2016/12/13 20:04:19]

Removed.

+  RequestCoordinatorFactory::GetInstance()->ReturnNullptrForTesting(false);
 }
 
 void RecentTabHelperTest::FailLoad(const GURL& url) {
   controller().LoadURL(url, content::Referrer(), ui::PAGE_TRANSITION_TYPED,
                        std::string());
   content::RenderFrameHostTester::For(main_rfh())->SimulateNavigationStart(url);
   content::RenderFrameHostTester::For(main_rfh())->
       SimulateNavigationError(url, net::ERR_INTERNET_DISCONNECTED);
   content::RenderFrameHostTester::For(main_rfh())->
       SimulateNavigationErrorPageCommit();
@@ skipping 85 matching lines @@
   // the same page should be simply overridden.
   GetAllPages();
   EXPECT_EQ(1U, all_pages().size());
   EXPECT_EQ(kTestPageUrl, all_pages()[0].url);
   EXPECT_NE(first_offline_id, all_pages()[0].offline_id);
 }
 
 // Triggers two snapshot captures during a single page load, where the 2nd one
 // fails. Should end up with one offline page (the 1st, successful snapshot
 // should be kept).
-TEST_F(RecentTabHelperTest, TwoCapturesSamePageLoadSecondFails) {
+// TODO(carlosk): re-enable once https://crbug.com/655697 is fixed, again.
+TEST_F(RecentTabHelperTest, DISABLED_TwoCapturesSamePageLoadSecondFails) {
   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();
   RunUntilIdle();
   EXPECT_EQ(1U, page_added_count());
@@ skipping 128 matching lines @@
   FastForwardSnapshotController();
   RunUntilIdle();
   EXPECT_EQ(2U, page_added_count());
   EXPECT_EQ(1U, model_removed_count());
   // the same page should be simply overridden.
   GetAllPages();
   EXPECT_EQ(1U, all_pages().size());
   EXPECT_EQ(kTestPageUrlOther, all_pages()[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) {
+  NavigateAndCommit(kTestPageUrl);
+  // Executes a regular load with snapshots taken by last_n.
+  recent_tab_helper()->DocumentAvailableInMainFrame();
+  RunUntilIdle();
+  FastForwardSnapshotController();
+  RunUntilIdle();
+  recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
+  FastForwardSnapshotController();
+  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();
+  EXPECT_EQ(1U, all_pages().size());
+  EXPECT_EQ(kTestPageUrl, all_pages()[0].url);
+  int64_t first_offline_id = all_pages()[0].offline_id;
+
+  // Request the 1st offline download.
+  const int64_t second_offline_id = 123l;

[fgorski, 2016/12/13 17:17:38]

nit: 123L

[carlosk, 2016/12/13 20:04:19]

Done here and in the last test in this file where I based this from.

+  ASSERT_NE(second_offline_id, first_offline_id);

[fgorski, 2016/12/13 17:17:38]

how is the first offline id assigned?

[carlosk, 2016/12/13 20:04:19]

The first entry is a last_n one and its offline ID is automatically set by the
offline model. Downloads do "suggest" their own IDs, hence the difference.

+  recent_tab_helper()->ObserveAndDownloadCurrentPage(
+      ClientId("download", "id2"), second_offline_id);
+  RunUntilIdle();
+  GetAllPages();
+  // Checks that both the previous last_n and download snapshots are present.
+  ASSERT_EQ(2U, all_pages().size());
+  EXPECT_NE(-1, FindPageIndexForOfflineId(first_offline_id));
+  {

[fgorski, 2016/12/13 17:17:38]

nit:  I am not sure why you are using {} here.
That don't seem to match out code anywhere.

[carlosk, 2016/12/13 20:04:20]

I use them to isolate the internal scope and avoid wrong copy/pasted code that
incorrectly reuses variables. In this case I didn't want |second_index| to be
reused by mistake in the similar section below.

But I agree it kinds looks weird here so I'm removing it.

+    int second_index = FindPageIndexForOfflineId(second_offline_id);
+    EXPECT_NE(-1, second_index);
+    EXPECT_EQ(kTestPageUrl, all_pages()[second_index].url);
+    EXPECT_EQ("download", all_pages()[second_index].client_id.name_space);
+    EXPECT_EQ("id2", all_pages()[second_index].client_id.id);
+  }
+
+  // Request the 2nd offline download.
+  const int64_t third_offline_id = 456l;

[fgorski, 2016/12/13 17:17:38]

ditto

[carlosk, 2016/12/13 20:04:20]

Done.

+  ASSERT_NE(third_offline_id, first_offline_id);
+  ASSERT_NE(third_offline_id, second_offline_id);

[fgorski, 2016/12/13 17:17:38]

you control both IDs. I am not sure this line makes sense.

[carlosk, 2016/12/13 20:04:19]

Done.

+  recent_tab_helper()->ObserveAndDownloadCurrentPage(
+      ClientId("download", "id3"), 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_NE(-1, FindPageIndexForOfflineId(first_offline_id));
+  EXPECT_NE(-1, FindPageIndexForOfflineId(second_offline_id));
+  {
+    int third_index = FindPageIndexForOfflineId(third_offline_id);

[fgorski, 2016/12/13 17:17:38]

given how you use the index below, wouldn't it make more sense to return
iterator from the FindPage... method?

[carlosk, 2016/12/13 20:04:19]

I agree with what you're saying given the usage below but I find iterator
comparisons more complicated so I switched the method to returning a pointer.

+    EXPECT_NE(-1, third_index);
+    EXPECT_EQ(kTestPageUrl, all_pages()[third_index].url);
+    EXPECT_EQ("download", all_pages()[third_index].client_id.name_space);
+    EXPECT_EQ("id3", all_pages()[third_index].client_id.id);
+  }
+}
+
 // Simulates an error (disconnection) during the load of a page. Should end up
 // with no offline pages.
 TEST_F(RecentTabHelperTest, NoCaptureOnErrorPage) {
   FailLoad(kTestPageUrl);
   recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
   FastForwardSnapshotController();
   RunUntilIdle();
   EXPECT_TRUE(model()->is_loaded());
   GetAllPages();
   EXPECT_EQ(0U, all_pages().size());
 }
 
 // Checks that no snapshots are created if the Offline Page Cache feature is
 // disabled.
 TEST_F(RecentTabHelperTest, FeatureNotEnabled) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.Init();
   NavigateAndCommit(kTestPageUrl);
   recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
   FastForwardSnapshotController();
   RunUntilIdle();
   EXPECT_TRUE(model()->is_loaded());
   GetAllPages();
   // No page should be captured.
   EXPECT_EQ(0U, all_pages().size());
 }
 
 // Simulates a download request to offline the current page. Should end up with
-// no offline pages.
+// one offline pages.
 TEST_F(RecentTabHelperTest, DISABLED_DownloadRequest) {
   NavigateAndCommit(kTestPageUrl);
   recent_tab_helper()->ObserveAndDownloadCurrentPage(
       ClientId("download", "id1"), 153l);
   recent_tab_helper()->DocumentOnLoadCompletedInMainFrame();
   RunUntilIdle();
   EXPECT_TRUE(model()->is_loaded());
   GetAllPages();
   EXPECT_EQ(1U, all_pages().size());
   const OfflinePageItem& page = all_pages()[0];
   EXPECT_EQ(kTestPageUrl, page.url);
   EXPECT_EQ("download", page.client_id.name_space);
   EXPECT_EQ("id1", page.client_id.id);
   EXPECT_EQ(153l, page.offline_id);
 }
 
 }  // namespace offline_pages