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

components/offline_pages/offline_page_model_unittest.cc

Index: components/offline_pages/offline_page_model_unittest.cc
diff --git a/components/offline_pages/offline_page_model_unittest.cc b/components/offline_pages/offline_page_model_unittest.cc
index 253dc67da9e427671da31095bbe886f1f17cad82..0215c714ed29c7ef9899ee9c078ea0105f6b4f79 100644
--- a/components/offline_pages/offline_page_model_unittest.cc
+++ b/components/offline_pages/offline_page_model_unittest.cc
@@ -62,7 +62,7 @@ class OfflinePageModelTest
   // 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;
@@ -94,9 +94,9 @@ class OfflinePageModelTest
 
   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(); }
@@ -109,7 +109,7 @@ class OfflinePageModelTest
     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_; }
 
@@ -122,7 +122,7 @@ class OfflinePageModelTest
   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()
@@ -130,8 +130,7 @@ OfflinePageModelTest::OfflinePageModelTest()
       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() {
 }
@@ -157,8 +156,8 @@ 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(
@@ -228,20 +227,19 @@ 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();
 }
@@ -253,7 +251,7 @@ TEST_F(OfflinePageModelTest, SavePageSuccessful) {
 
   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);
@@ -265,7 +263,7 @@ TEST_F(OfflinePageModelTest, SavePageSuccessful) {
 
   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);
@@ -350,7 +348,7 @@ TEST_F(OfflinePageModelTest, SavePageOfflineArchiverTwoPages) {
   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);
@@ -363,7 +361,7 @@ TEST_F(OfflinePageModelTest, SavePageOfflineArchiverTwoPages) {
   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);
@@ -375,13 +373,13 @@ TEST_F(OfflinePageModelTest, SavePageOfflineArchiverTwoPages) {
 
   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);
@@ -399,7 +397,7 @@ TEST_F(OfflinePageModelTest, MarkPageAccessed) {
 
   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);
 }
@@ -494,7 +492,7 @@ TEST_F(OfflinePageModelTest, DeletePageSuccessful) {
 
   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);
@@ -508,7 +506,7 @@ TEST_F(OfflinePageModelTest, DeletePageSuccessful) {
 
   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());
 }
@@ -549,7 +547,7 @@ TEST_F(OfflinePageModelTest, DetectThatOfflineCopyIsMissing) {
   model()->CheckForExternalFileDeletion();
   PumpLoop();
 
-  EXPECT_EQ(last_deleted_bookmark_id(), kTestPageBookmarkId1);
+  EXPECT_EQ(last_deleted_offline_id(), kTestPageBookmarkId1);
   EXPECT_EQ(0UL, model()->GetAllPages().size());
 }
 
@@ -567,7 +565,7 @@ TEST_F(OfflinePageModelTest, DetectThatOfflineCopyIsMissingAfterLoad) {
   ResetModel();
   PumpLoop();
 
-  EXPECT_EQ(last_deleted_bookmark_id(), kTestPageBookmarkId1);
+  EXPECT_EQ(last_deleted_offline_id(), kTestPageBookmarkId1);
   EXPECT_EQ(0UL, model()->GetAllPages().size());
 }
 
@@ -621,13 +619,13 @@ TEST_F(OfflinePageModelTest, GetPageByBookmarkId) {
       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);
@@ -647,12 +645,12 @@ TEST_F(OfflinePageModelTest, GetPageByOfflineURL) {
   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);
@@ -665,12 +663,12 @@ TEST_F(OfflinePageModelTest, GetPageByOnlineURL) {
   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);
@@ -700,9 +698,9 @@ TEST_F(OfflinePageModelTest, GetPagesToCleanUp) {
   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) {
@@ -917,4 +915,7 @@ TEST_F(OfflinePageModelBookmarkChangeTest, UndoBookmarkRemoval) {
   EXPECT_EQ(1UL, model()->GetAllPages().size());
 }
 
+// TODO(bburns): Add a unit test for returning multiple results for a
+// single client id.
+
 }  // namespace offline_pages