Store original request URL in offline page metadata table

components/offline_pages/offline_page_model_impl_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_impl.h"
 
 #include <stdint.h>
 #include <algorithm>
 #include <utility>
 
@@ skipping 107 matching lines @@
   void FastForwardBy(base::TimeDelta delta);
   void ResetResults();
 
   OfflinePageTestStore* GetStore();
 
   MultipleOfflinePageItemResult GetAllPages();
   MultipleOfflinePageItemResult GetPagesByClientIds(
       const std::vector<ClientId>& client_ids);
   void DeletePagesByClientIds(const std::vector<ClientId>& client_ids);
 
+  // Saves the page without waiting for it to finish.
+  void AsyncSavePageWithArchiver(

[fgorski, 2016/11/10 23:56:13]

nit: I like to put Async/Sync at the end. (Sorts better in code search) Up to
you thou.

[jianli, 2016/11/16 01:25:40]

Done.

+      const GURL& url,
+      const ClientId& client_id,
+      const GURL& original_url,
+      std::unique_ptr<OfflinePageArchiver> archiver);
+
+  // All 3 methods below will wait for the save to finish.
+  void SavePageWithArchiver(
+      const GURL& url,
+      const ClientId& client_id,
+      std::unique_ptr<OfflinePageArchiver> archiver);
+  void SavePageWithArchiverResult(const GURL& url,
+                                  const ClientId& client_id,
+                                  OfflinePageArchiver::ArchiverResult result);
   // Returns the offline ID of the saved page.
   std::pair<SavePageResult, int64_t> SavePage(const GURL& url,
-                                              ClientId client_id);
-
-  void SavePageWithArchiverResult(const GURL& url,
-                                  ClientId client_id,
-                                  OfflinePageArchiver::ArchiverResult result);
+                                              const ClientId& client_id);
 
   void DeletePage(int64_t offline_id, const DeletePageCallback& callback) {
     std::vector<int64_t> offline_ids;
     offline_ids.push_back(offline_id);
     model()->DeletePagesByOfflineId(offline_ids, callback);
   }
 
   bool HasPages(std::string name_space);
 
   CheckPagesExistOfflineResult CheckPagesExistOffline(std::set<GURL>);
@@ skipping 154 matching lines @@
   last_delete_result_ = DeletePageResult::CANCELLED;
   last_archiver_path_.clear();
   last_pages_exist_result_.clear();
   last_cleared_pages_count_ = 0;
 }
 
 OfflinePageTestStore* OfflinePageModelImplTest::GetStore() {
   return static_cast<OfflinePageTestStore*>(model()->GetStoreForTesting());
 }
 
-std::pair<SavePageResult, int64_t> OfflinePageModelImplTest::SavePage(
+void OfflinePageModelImplTest::AsyncSavePageWithArchiver(
     const GURL& url,
-    ClientId client_id) {
+    const ClientId& client_id,
+    const GURL& original_url,
+    std::unique_ptr<OfflinePageArchiver> archiver) {
+  OfflinePageModel::SavePageInfo save_page_info;
+  save_page_info.url = url;
+  save_page_info.client_id = client_id;
+  save_page_info.original_url = original_url;
+  model()->SavePage(
+      save_page_info,
+      std::move(archiver),
+      base::Bind(&OfflinePageModelImplTest::OnSavePageDone, AsWeakPtr()));
+}
+
+void OfflinePageModelImplTest::SavePageWithArchiver(
+    const GURL& url,
+    const ClientId& client_id,
+    std::unique_ptr<OfflinePageArchiver> archiver) {
+  AsyncSavePageWithArchiver(url, client_id, GURL(), std::move(archiver));
+  PumpLoop();
+}
+
+void OfflinePageModelImplTest::SavePageWithArchiverResult(
+    const GURL& url,
+    const ClientId& client_id,
+    OfflinePageArchiver::ArchiverResult result) {
+  std::unique_ptr<OfflinePageTestArchiver> archiver(BuildArchiver(url, result));
+  SavePageWithArchiver(url, client_id, std::move(archiver));

[fgorski, 2016/11/10 23:56:13]

please don't chain methods like that.
It is better to call final implementation (Async/Pump).

[jianli, 2016/11/16 01:25:40]

Done.

+}
+
+std::pair<SavePageResult, int64_t>
+OfflinePageModelImplTest::SavePage(const GURL& url, const ClientId& client_id) {
   SavePageWithArchiverResult(

[fgorski, 2016/11/10 23:56:14]

Same here. Please update.

[jianli, 2016/11/16 01:25:40]

Done.

-      url, client_id,
+      url,
+      client_id,
       OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED);
   return std::make_pair(last_save_result_, last_save_offline_id_);
 }
 
-void OfflinePageModelImplTest::SavePageWithArchiverResult(
-    const GURL& url,
-    ClientId client_id,
-    OfflinePageArchiver::ArchiverResult result) {
-  std::unique_ptr<OfflinePageTestArchiver> archiver(BuildArchiver(url, result));
-  model()->SavePage(
-      url, client_id, 0l, std::move(archiver),
-      base::Bind(&OfflinePageModelImplTest::OnSavePageDone, AsWeakPtr()));
-  PumpLoop();
-}
-
 MultipleOfflinePageItemResult OfflinePageModelImplTest::GetAllPages() {
   MultipleOfflinePageItemResult result;
   model()->GetAllPages(
       base::Bind(&OfflinePageModelImplTest::OnGetMultipleOfflinePageItemsResult,
                  AsWeakPtr(), base::Unretained(&result)));
   PumpLoop();
   return result;
 }
 
 MultipleOfflinePageItemResult OfflinePageModelImplTest::GetPagesByClientIds(
@@ skipping 89 matching lines @@
   for (const auto& page : all_pages) {
     if (page.client_id.name_space == name_space)
       return true;
   }
 
   return false;
 }
 
 TEST_F(OfflinePageModelImplTest, SavePageSuccessful) {
   EXPECT_FALSE(HasPages(kTestClientNamespace));
-  SavePage(kTestUrl, kTestClientId1);
+
+   std::unique_ptr<OfflinePageTestArchiver> archiver(BuildArchiver(
+      kTestUrl, OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED));
+  AsyncSavePageWithArchiver(
+      kTestUrl, kTestClientId1, kTestUrl2, std::move(archiver));
+  PumpLoop();
   EXPECT_TRUE(HasPages(kTestClientNamespace));
 
   OfflinePageTestStore* store = GetStore();
   EXPECT_EQ(kTestUrl, store->last_saved_page().url);
   EXPECT_EQ(kTestClientId1.id, store->last_saved_page().client_id.id);
   EXPECT_EQ(kTestClientId1.name_space,
             store->last_saved_page().client_id.name_space);
   // 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 = GetAllPages();
 
-  EXPECT_EQ(1UL, offline_pages.size());
+  ASSERT_EQ(1UL, offline_pages.size());

[fgorski, 2016/11/10 23:56:13]

Good!

[jianli, 2016/11/16 01:25:40]

Acknowledged.

   EXPECT_EQ(kTestUrl, offline_pages[0].url);
   EXPECT_EQ(kTestClientId1.id, offline_pages[0].client_id.id);
   EXPECT_EQ(kTestClientId1.name_space, offline_pages[0].client_id.name_space);
   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(kTestTitle, offline_pages[0].title);
+  EXPECT_EQ(kTestUrl2, offline_pages[0].original_url);
 }
 
 TEST_F(OfflinePageModelImplTest, SavePageOfflineArchiverCancelled) {
   SavePageWithArchiverResult(
       kTestUrl, kTestClientId1,
       OfflinePageArchiver::ArchiverResult::ERROR_CANCELED);
   EXPECT_EQ(SavePageResult::CANCELLED, last_save_result());
 }
 
 TEST_F(OfflinePageModelImplTest, SavePageOfflineArchiverDeviceFull) {
@@ skipping 14 matching lines @@
   SavePageWithArchiverResult(
       kTestUrl, kTestClientId1,
       OfflinePageArchiver::ArchiverResult::ERROR_ARCHIVE_CREATION_FAILED);
   EXPECT_EQ(SavePageResult::ARCHIVE_CREATION_FAILED, last_save_result());
 }
 
 TEST_F(OfflinePageModelImplTest, SavePageOfflineArchiverReturnedWrongUrl) {
   std::unique_ptr<OfflinePageTestArchiver> archiver(
       BuildArchiver(GURL("http://other.random.url.com"),
                     OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED));
-  model()->SavePage(
-      kTestUrl, kTestClientId1, 0l, std::move(archiver),
-      base::Bind(&OfflinePageModelImplTest::OnSavePageDone, AsWeakPtr()));
-  PumpLoop();
+  SavePageWithArchiver(kTestUrl, kTestClientId1, std::move(archiver));
   EXPECT_EQ(SavePageResult::ARCHIVE_CREATION_FAILED, last_save_result());
 }
 
 TEST_F(OfflinePageModelImplTest, SavePageOfflineCreationStoreWriteFailure) {
   GetStore()->set_test_scenario(
       OfflinePageTestStore::TestScenario::WRITE_FAILED);
-  SavePage(kTestUrl, kTestClientId1);
+  SavePageWithArchiverResult(
+      kTestUrl, kTestClientId1,
+      OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED);
   EXPECT_EQ(SavePageResult::STORE_FAILURE, last_save_result());
 }
 
 TEST_F(OfflinePageModelImplTest, SavePageLocalFileFailed) {
   // Don't create archiver since it will not be needed for pages that are not
   // going to be saved.
-  model()->SavePage(
-      kFileUrl, kTestClientId1, 0l, std::unique_ptr<OfflinePageTestArchiver>(),
-      base::Bind(&OfflinePageModelImplTest::OnSavePageDone, AsWeakPtr()));
-  PumpLoop();
+  SavePageWithArchiver(
+      kFileUrl, kTestClientId1, std::unique_ptr<OfflinePageTestArchiver>());
   EXPECT_EQ(SavePageResult::SKIPPED, last_save_result());
 }
 
 TEST_F(OfflinePageModelImplTest, SavePageOfflineArchiverTwoPages) {
   std::unique_ptr<OfflinePageTestArchiver> archiver(BuildArchiver(
       kTestUrl, OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED));
   // archiver_ptr will be valid until after first PumpLoop() call after
   // CompleteCreateArchive() is called.
   OfflinePageTestArchiver* archiver_ptr = archiver.get();
   archiver_ptr->set_delayed(true);
-  model()->SavePage(
-      kTestUrl, kTestClientId1, 0l, std::move(archiver),
-      base::Bind(&OfflinePageModelImplTest::OnSavePageDone, AsWeakPtr()));
+  AsyncSavePageWithArchiver(
+      kTestUrl, kTestClientId1, GURL(), std::move(archiver));
   EXPECT_TRUE(archiver_ptr->create_archive_called());
 
   // Request to save another page.
   SavePage(kTestUrl2, kTestClientId2);
 
   OfflinePageTestStore* store = GetStore();
 
   EXPECT_EQ(kTestUrl2, store->last_saved_page().url);
   EXPECT_EQ(kTestClientId2, store->last_saved_page().client_id);
   base::FilePath archiver_path2 = last_archiver_path();
@@ skipping 293 matching lines @@
   PumpLoop();
 
   EXPECT_EQ(0UL, GetAllPages().size());
   EXPECT_FALSE(base::PathExists(path));
 }
 
 TEST_F(OfflinePageModelImplTest, DeleteMultiplePages) {
   OfflinePageTestStore* store = GetStore();
 
   // Save 3 pages.
-  std::unique_ptr<OfflinePageTestArchiver> archiver(BuildArchiver(
-      kTestUrl, OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED));
-  model()->SavePage(
-      kTestUrl, kTestClientId1, 0l, std::move(archiver),
-      base::Bind(&OfflinePageModelImplTest::OnSavePageDone, AsWeakPtr()));
-  PumpLoop();
+  SavePage(kTestUrl, kTestClientId1);
   int64_t offline1 = last_save_offline_id();
 
-  std::unique_ptr<OfflinePageTestArchiver> archiver2(BuildArchiver(
-      kTestUrl2, OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED));
-  model()->SavePage(
-      kTestUrl2, kTestClientId2, 0l, std::move(archiver2),
-      base::Bind(&OfflinePageModelImplTest::OnSavePageDone, AsWeakPtr()));
-  PumpLoop();
+  SavePage(kTestUrl2, kTestClientId2);
   int64_t offline2 = last_save_offline_id();
 
-  std::unique_ptr<OfflinePageTestArchiver> archiver3(BuildArchiver(
-      kTestUrl3, OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED));
-  model()->SavePage(
-      kTestUrl3, kTestClientId3, 0l, std::move(archiver3),
-      base::Bind(&OfflinePageModelImplTest::OnSavePageDone, AsWeakPtr()));
+  SavePage(kTestUrl3, kTestClientId3);
   PumpLoop();
 
   EXPECT_EQ(3u, store->GetAllPages().size());
 
   // Delete multiple pages.
   std::vector<int64_t> ids_to_delete;
   ids_to_delete.push_back(offline2);
   ids_to_delete.push_back(offline1);
   ids_to_delete.push_back(23434LL);  // Non-existent ID.
   model()->DeletePagesByOfflineId(
@@ skipping 350 matching lines @@
   // Check if feature is correctly enabled by command-line flag.
   scoped_feature_list.reset(new base::test::ScopedFeatureList);
   scoped_feature_list->InitFromCommandLine(
       std::string(kOfflineBookmarksFeature.name) + "," +
           kOfflinePagesSharingFeature.name,
       "");
   EXPECT_TRUE(offline_pages::IsOfflinePagesSharingEnabled());
 }
 
 }  // namespace offline_pages