Refactor the offline page storage to include client namespace and id.

components/offline_pages/offline_page_model_unittest.cc

 // Copyright 2015 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 "components/offline_pages/offline_page_model.h"
 
 #include <stdint.h>
 #include <algorithm>
 #include <utility>
 
@@ skipping 44 matching lines @@
  public:
   OfflinePageModelTest();
   ~OfflinePageModelTest() override;
 
   void SetUp() override;
   void TearDown() override;
 
   // OfflinePageModel::Observer implementation.
   void OfflinePageModelLoaded(OfflinePageModel* model) override;
   void OfflinePageModelChanged(OfflinePageModel* model) override;
-  void OfflinePageDeleted(int64_t bookmark_id) override;
+  void OfflinePageDeleted(int64_t offline_id) override;
 
   // OfflinePageTestArchiver::Observer implementation.
   void SetLastPathCreatedByArchiver(const base::FilePath& file_path) override;
 
   // OfflinePageModel callbacks.
   void OnSavePageDone(SavePageResult result);
   void OnDeletePageDone(DeletePageResult result);
   void OnClearAllDone();
 
   // OfflinePageMetadataStore callbacks.
@@ skipping 11 matching lines @@
   // Runs until all of the tasks that are not delayed are gone from the task
   // queue.
   void PumpLoop();
   // Fast-forwards virtual time by |delta|, causing tasks with a remaining
   // delay less than or equal to |delta| to be executed.
   void FastForwardBy(base::TimeDelta delta);
   void ResetResults();
 
   OfflinePageTestStore* GetStore();
 
-  void SavePage(const GURL& url, int64_t bookmark_id);
+  void SavePage(const GURL& url, int64_t offline_id);
   void SavePageWithArchiverResult(const GURL& url,
-                                  int64_t bookmark_id,
+                                  int64_t offline_id,
                                   OfflinePageArchiver::ArchiverResult result);
 
   OfflinePageModel* model() { return model_.get(); }
 
   SavePageResult last_save_result() const {
     return last_save_result_;
   }
 
   DeletePageResult last_delete_result() const {
     return last_delete_result_;
   }
 
-  int64_t last_deleted_bookmark_id() const { return last_deleted_bookmark_id_; }
+  int64_t last_deleted_offline_id() const { return last_deleted_offline_id_; }
 
   const base::FilePath& last_archiver_path() { return last_archiver_path_; }
 
  private:
   scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
   base::ThreadTaskRunnerHandle task_runner_handle_;
   base::ScopedTempDir temp_dir_;
 
   scoped_ptr<OfflinePageModel> model_;
   SavePageResult last_save_result_;
   DeletePageResult last_delete_result_;
   base::FilePath last_archiver_path_;
-  int64_t last_deleted_bookmark_id_;
+  int64_t last_deleted_offline_id_;
 };
 
 OfflinePageModelTest::OfflinePageModelTest()
     : task_runner_(new base::TestMockTimeTaskRunner),
       task_runner_handle_(task_runner_),
       last_save_result_(SavePageResult::CANCELLED),
       last_delete_result_(DeletePageResult::CANCELLED),
-      last_deleted_bookmark_id_(-1) {
-}
+      last_deleted_offline_id_(-1) {}
 
 OfflinePageModelTest::~OfflinePageModelTest() {
 }
 
 void OfflinePageModelTest::SetUp() {
   ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
   model_ = BuildModel(BuildStore());
   model_->AddObserver(this);
   PumpLoop();
 }
 
 void OfflinePageModelTest::TearDown() {
   model_->RemoveObserver(this);
   model_.reset();
   PumpLoop();
 }
 
 void OfflinePageModelTest::OfflinePageModelLoaded(OfflinePageModel* model) {
   ASSERT_EQ(model_.get(), model);
 }
 
 void OfflinePageModelTest::OfflinePageModelChanged(OfflinePageModel* model) {
   ASSERT_EQ(model_.get(), model);
 }
 
-void OfflinePageModelTest::OfflinePageDeleted(int64_t bookmark_id) {
-  last_deleted_bookmark_id_ = bookmark_id;
+void OfflinePageModelTest::OfflinePageDeleted(int64_t offline_id) {
+  last_deleted_offline_id_ = offline_id;
 }
 
 void OfflinePageModelTest::SetLastPathCreatedByArchiver(
     const base::FilePath& file_path) {
   last_archiver_path_ = file_path;
 }
 
 void OfflinePageModelTest::OnSavePageDone(
     OfflinePageModel::SavePageResult result) {
   last_save_result_ = result;
@@ skipping 49 matching lines @@
 void OfflinePageModelTest::ResetResults() {
   last_save_result_ = SavePageResult::CANCELLED;
   last_delete_result_ = DeletePageResult::CANCELLED;
   last_archiver_path_.clear();
 }
 
 OfflinePageTestStore* OfflinePageModelTest::GetStore() {
   return static_cast<OfflinePageTestStore*>(model()->GetStoreForTesting());
 }
 
-void OfflinePageModelTest::SavePage(const GURL& url, int64_t bookmark_id) {
+void OfflinePageModelTest::SavePage(const GURL& url, int64_t offline_id) {
   SavePageWithArchiverResult(
-      url,
-      bookmark_id,
+      url, offline_id,
       OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED);
 }
 
 void OfflinePageModelTest::SavePageWithArchiverResult(
     const GURL& url,
-    int64_t bookmark_id,
+    int64_t offline_id,
     OfflinePageArchiver::ArchiverResult result) {
   scoped_ptr<OfflinePageTestArchiver> archiver(BuildArchiver(url, result));
   model()->SavePage(
-      url, bookmark_id, std::move(archiver),
+      url, offline_id, std::move(archiver),

[Dmitry Titov, 2016/02/25 03:31:52]

I'm not sure how it compiles... SavePage now takes ClientId as well, and it is
not passed here... 
BTW, by passing existing bookmarkId as part of ClientId, you would get rid of
the last place that needs offline_id to be passed into
OfflinePageModel::SavePage, and thus simply remove the offline_id parameter for
that method. Then this CL will be more complete.

[bburns, 2016/02/26 00:14:35]

Done (I hadn't compiled tests yet until we got closure on the other stuff...)

       base::Bind(&OfflinePageModelTest::OnSavePageDone, AsWeakPtr()));
   PumpLoop();
 }
 
 TEST_F(OfflinePageModelTest, SavePageSuccessful) {
   EXPECT_FALSE(model()->HasOfflinePages());
   SavePage(kTestUrl, kTestPageBookmarkId1);
   EXPECT_TRUE(model()->HasOfflinePages());
 
   OfflinePageTestStore* store = GetStore();
   EXPECT_EQ(kTestUrl, store->last_saved_page().url);
-  EXPECT_EQ(kTestPageBookmarkId1, store->last_saved_page().bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId1, store->last_saved_page().offline_id);
   // Save last_archiver_path since it will be referred to later.
   base::FilePath archiver_path = last_archiver_path();
   EXPECT_EQ(archiver_path, store->last_saved_page().file_path);
   EXPECT_EQ(kTestFileSize, store->last_saved_page().file_size);
   EXPECT_EQ(SavePageResult::SUCCESS, last_save_result());
   ResetResults();
 
   const std::vector<OfflinePageItem>& offline_pages = model()->GetAllPages();
 
   EXPECT_EQ(1UL, offline_pages.size());
   EXPECT_EQ(kTestUrl, offline_pages[0].url);
-  EXPECT_EQ(kTestPageBookmarkId1, offline_pages[0].bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId1, offline_pages[0].offline_id);
   EXPECT_EQ(archiver_path, offline_pages[0].file_path);
   EXPECT_EQ(kTestFileSize, offline_pages[0].file_size);
   EXPECT_EQ(0, offline_pages[0].access_count);
   EXPECT_EQ(0, offline_pages[0].flags);
 }
 
 TEST_F(OfflinePageModelTest, SavePageOfflineArchiverCancelled) {
   SavePageWithArchiverResult(
       kTestUrl,
       kTestPageBookmarkId1,
@@ skipping 64 matching lines @@
       kTestUrl, kTestPageBookmarkId1, std::move(archiver),
       base::Bind(&OfflinePageModelTest::OnSavePageDone, AsWeakPtr()));
   EXPECT_TRUE(archiver_ptr->create_archive_called());
 
   // Request to save another page.
   SavePage(kTestUrl2, kTestPageBookmarkId2);
 
   OfflinePageTestStore* store = GetStore();
 
   EXPECT_EQ(kTestUrl2, store->last_saved_page().url);
-  EXPECT_EQ(kTestPageBookmarkId2, store->last_saved_page().bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId2, store->last_saved_page().offline_id);
   base::FilePath archiver_path2 = last_archiver_path();
   EXPECT_EQ(archiver_path2, store->last_saved_page().file_path);
   EXPECT_EQ(kTestFileSize, store->last_saved_page().file_size);
   EXPECT_EQ(SavePageResult::SUCCESS, last_save_result());
 
   ResetResults();
 
   archiver_ptr->CompleteCreateArchive();
   // After this pump loop archiver_ptr is invalid.
   PumpLoop();
 
   EXPECT_EQ(kTestUrl, store->last_saved_page().url);
-  EXPECT_EQ(kTestPageBookmarkId1, store->last_saved_page().bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId1, store->last_saved_page().offline_id);
   base::FilePath archiver_path = last_archiver_path();
   EXPECT_EQ(archiver_path, store->last_saved_page().file_path);
   EXPECT_EQ(kTestFileSize, store->last_saved_page().file_size);
   EXPECT_EQ(SavePageResult::SUCCESS, last_save_result());
 
   ResetResults();
 
   const std::vector<OfflinePageItem>& offline_pages = model()->GetAllPages();
 
   EXPECT_EQ(2UL, offline_pages.size());
   EXPECT_EQ(kTestUrl, offline_pages[0].url);
-  EXPECT_EQ(kTestPageBookmarkId1, offline_pages[0].bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId1, offline_pages[0].offline_id);
   EXPECT_EQ(archiver_path, offline_pages[0].file_path);
   EXPECT_EQ(kTestFileSize, offline_pages[0].file_size);
   EXPECT_EQ(0, offline_pages[0].access_count);
   EXPECT_EQ(0, offline_pages[0].flags);
   EXPECT_EQ(kTestUrl2, offline_pages[1].url);
-  EXPECT_EQ(kTestPageBookmarkId2, offline_pages[1].bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId2, offline_pages[1].offline_id);
   EXPECT_EQ(archiver_path2, offline_pages[1].file_path);
   EXPECT_EQ(kTestFileSize, offline_pages[1].file_size);
   EXPECT_EQ(0, offline_pages[1].access_count);
   EXPECT_EQ(0, offline_pages[1].flags);
 }
 
 TEST_F(OfflinePageModelTest, MarkPageAccessed) {
   SavePage(kTestUrl, kTestPageBookmarkId1);
 
   // This will increase access_count by one.
   model()->MarkPageAccessed(kTestPageBookmarkId1);
   PumpLoop();
 
   const std::vector<OfflinePageItem>& offline_pages = model()->GetAllPages();
 
   EXPECT_EQ(1UL, offline_pages.size());
   EXPECT_EQ(kTestUrl, offline_pages[0].url);
-  EXPECT_EQ(kTestPageBookmarkId1, offline_pages[0].bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId1, offline_pages[0].offline_id);
   EXPECT_EQ(kTestFileSize, offline_pages[0].file_size);
   EXPECT_EQ(1, offline_pages[0].access_count);
 }
 
 TEST_F(OfflinePageModelTest, MarkPageForDeletion) {
   SavePage(kTestUrl, kTestPageBookmarkId1);
 
   GURL offline_url = model()->GetAllPages().begin()->GetOfflineURL();
 
   // Delete the page with undo tiggerred.
@@ skipping 74 matching lines @@
 
   ResetResults();
 
   // Delete one page.
   model()->DeletePageByBookmarkId(
       kTestPageBookmarkId1, base::Bind(&OfflinePageModelTest::OnDeletePageDone,
                                        AsWeakPtr()));
 
   PumpLoop();
 
-  EXPECT_EQ(last_deleted_bookmark_id(), kTestPageBookmarkId1);
+  EXPECT_EQ(last_deleted_offline_id(), kTestPageBookmarkId1);
   EXPECT_EQ(DeletePageResult::SUCCESS, last_delete_result());
   ASSERT_EQ(1u, store->GetAllPages().size());
   EXPECT_EQ(kTestUrl2, store->GetAllPages()[0].url);
 
   // Delete another page.
   model()->DeletePageByBookmarkId(
       kTestPageBookmarkId2, base::Bind(&OfflinePageModelTest::OnDeletePageDone,
                                        AsWeakPtr()));
 
   ResetResults();
 
   PumpLoop();
 
-  EXPECT_EQ(last_deleted_bookmark_id(), kTestPageBookmarkId2);
+  EXPECT_EQ(last_deleted_offline_id(), kTestPageBookmarkId2);
   EXPECT_EQ(DeletePageResult::SUCCESS, last_delete_result());
   EXPECT_EQ(0u, store->GetAllPages().size());
 }
 
 TEST_F(OfflinePageModelTest, DeletePageNotFound) {
   model()->DeletePageByBookmarkId(
       kTestPageBookmarkId1, base::Bind(&OfflinePageModelTest::OnDeletePageDone,
                                        AsWeakPtr()));
   EXPECT_EQ(DeletePageResult::NOT_FOUND, last_delete_result());
 }
@@ skipping 20 matching lines @@
 
   ResetResults();
 
   const OfflinePageItem* page =
       model()->GetPageByBookmarkId(kTestPageBookmarkId1);
   // Delete the offline copy of the page and check the metadata.
   base::DeleteFile(page->file_path, false);
   model()->CheckForExternalFileDeletion();
   PumpLoop();
 
-  EXPECT_EQ(last_deleted_bookmark_id(), kTestPageBookmarkId1);
+  EXPECT_EQ(last_deleted_offline_id(), kTestPageBookmarkId1);
   EXPECT_EQ(0UL, model()->GetAllPages().size());
 }
 
 TEST_F(OfflinePageModelTest, DetectThatOfflineCopyIsMissingAfterLoad) {
   // Save a page.
   SavePage(kTestUrl, kTestPageBookmarkId1);
 
   ResetResults();
 
   const OfflinePageItem* page =
       model()->GetPageByBookmarkId(kTestPageBookmarkId1);
   // Delete the offline copy of the page and check the metadata.
   base::DeleteFile(page->file_path, false);
   // Reseting the model should trigger the metadata consistency check as well.
   ResetModel();
   PumpLoop();
 
-  EXPECT_EQ(last_deleted_bookmark_id(), kTestPageBookmarkId1);
+  EXPECT_EQ(last_deleted_offline_id(), kTestPageBookmarkId1);
   EXPECT_EQ(0UL, model()->GetAllPages().size());
 }
 
 TEST_F(OfflinePageModelTest, DeleteMultiplePages) {
   OfflinePageTestStore* store = GetStore();
 
   // Save 3 pages.
   scoped_ptr<OfflinePageTestArchiver> archiver(BuildArchiver(
       kTestUrl, OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED));
   model()->SavePage(
@@ skipping 33 matching lines @@
 }
 
 TEST_F(OfflinePageModelTest, GetPageByBookmarkId) {
   SavePage(kTestUrl, kTestPageBookmarkId1);
   SavePage(kTestUrl2, kTestPageBookmarkId2);
 
   const OfflinePageItem* page =
       model()->GetPageByBookmarkId(kTestPageBookmarkId1);
   ASSERT_TRUE(page);
   EXPECT_EQ(kTestUrl, page->url);
-  EXPECT_EQ(kTestPageBookmarkId1, page->bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId1, page->offline_id);
   EXPECT_EQ(kTestFileSize, page->file_size);
 
   page = model()->GetPageByBookmarkId(kTestPageBookmarkId2);
   ASSERT_TRUE(page);
   EXPECT_EQ(kTestUrl2, page->url);
-  EXPECT_EQ(kTestPageBookmarkId2, page->bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId2, page->offline_id);
   EXPECT_EQ(kTestFileSize, page->file_size);
 
   page = model()->GetPageByBookmarkId(-42);
   EXPECT_FALSE(page);
 }
 
 TEST_F(OfflinePageModelTest, GetPageByOfflineURL) {
   SavePage(kTestUrl, kTestPageBookmarkId1);
 
   OfflinePageTestStore* store = GetStore();
   GURL offline_url = store->last_saved_page().GetOfflineURL();
 
   SavePage(kTestUrl2, kTestPageBookmarkId2);
 
   GURL offline_url2 = store->last_saved_page().GetOfflineURL();
 
   const OfflinePageItem* page = model()->GetPageByOfflineURL(offline_url2);
   EXPECT_TRUE(page);
   EXPECT_EQ(kTestUrl2, page->url);
-  EXPECT_EQ(kTestPageBookmarkId2, page->bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId2, page->offline_id);
 
   page = model()->GetPageByOfflineURL(offline_url);
   EXPECT_TRUE(page);
   EXPECT_EQ(kTestUrl, page->url);
-  EXPECT_EQ(kTestPageBookmarkId1, page->bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId1, page->offline_id);
 
   page = model()->GetPageByOfflineURL(GURL("http://foo"));
   EXPECT_FALSE(page);
 }
 
 TEST_F(OfflinePageModelTest, GetPageByOnlineURL) {
   SavePage(kTestUrl, kTestPageBookmarkId1);
   SavePage(kTestUrl2, kTestPageBookmarkId2);
 
   const OfflinePageItem* page = model()->GetPageByOnlineURL(kTestUrl2);
   EXPECT_TRUE(page);
   EXPECT_EQ(kTestUrl2, page->url);
-  EXPECT_EQ(kTestPageBookmarkId2, page->bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId2, page->offline_id);
 
   page = model()->GetPageByOnlineURL(kTestUrl);
   EXPECT_TRUE(page);
   EXPECT_EQ(kTestUrl, page->url);
-  EXPECT_EQ(kTestPageBookmarkId1, page->bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId1, page->offline_id);
 
   page = model()->GetPageByOnlineURL(GURL("http://foo"));
   EXPECT_FALSE(page);
 }
 
 // Test that model returns pages that are older than 30 days as candidates for
 // clean up, hence the numbers in time delta.
 TEST_F(OfflinePageModelTest, GetPagesToCleanUp) {
   base::Time now = base::Time::Now();
 
   SavePage(kTestUrl, kTestPageBookmarkId1);
   GetStore()->UpdateLastAccessTime(kTestPageBookmarkId1,
                                    now - base::TimeDelta::FromDays(40));
 
   SavePage(kTestUrl2, kTestPageBookmarkId2);
   GetStore()->UpdateLastAccessTime(kTestPageBookmarkId2,
                                    now - base::TimeDelta::FromDays(31));
 
   SavePage(kTestUrl3, kTestPageBookmarkId3);
   GetStore()->UpdateLastAccessTime(kTestPageBookmarkId3,
                                    now - base::TimeDelta::FromDays(29));
 
   ResetModel();
 
   // Only page_1 and page_2 are expected to be picked up by the model as page_3
   // has not been in the store long enough.
   std::vector<OfflinePageItem> pages_to_clean_up = model()->GetPagesToCleanUp();
   EXPECT_EQ(2UL, pages_to_clean_up.size());
   EXPECT_EQ(kTestUrl, pages_to_clean_up[0].url);
-  EXPECT_EQ(kTestPageBookmarkId1, pages_to_clean_up[0].bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId1, pages_to_clean_up[0].offline_id);
   EXPECT_EQ(kTestUrl2, pages_to_clean_up[1].url);
-  EXPECT_EQ(kTestPageBookmarkId2, pages_to_clean_up[1].bookmark_id);
+  EXPECT_EQ(kTestPageBookmarkId2, pages_to_clean_up[1].offline_id);
 }
 
 TEST_F(OfflinePageModelTest, CanSavePage) {
   EXPECT_TRUE(OfflinePageModel::CanSavePage(GURL("http://foo")));
   EXPECT_TRUE(OfflinePageModel::CanSavePage(GURL("https://foo")));
   EXPECT_FALSE(OfflinePageModel::CanSavePage(GURL("file:///foo")));
   EXPECT_FALSE(OfflinePageModel::CanSavePage(GURL("data:image/png;base64,ab")));
   EXPECT_FALSE(OfflinePageModel::CanSavePage(GURL("chrome://version")));
   EXPECT_FALSE(OfflinePageModel::CanSavePage(GURL("chrome-native://newtab/")));
 }
@@ skipping 194 matching lines @@
   bookmark_model()->Remove(bookmark_node);
   PumpLoop();
   EXPECT_EQ(0UL, model()->GetAllPages().size());
 
   // The offline copy should be restored upon the bookmark restore.
   UndoBookmarkRemoval();
   PumpLoop();
   EXPECT_EQ(1UL, model()->GetAllPages().size());
 }
 
+// TODO(bburns): Add a unit test for returning multiple results for a
+// single client id.
+
 }  // namespace offline_pages