Store original request URL in offline page metadata table

components/offline_pages/offline_page_model_impl_unittest.cc

Index: components/offline_pages/offline_page_model_impl_unittest.cc
diff --git a/components/offline_pages/offline_page_model_impl_unittest.cc b/components/offline_pages/offline_page_model_impl_unittest.cc
index e9cdd58ef4e60c7f8546d48794cd626192166154..076b133b22955812b5e2866681db4052fba81fbc 100644
--- a/components/offline_pages/offline_page_model_impl_unittest.cc
+++ b/components/offline_pages/offline_page_model_impl_unittest.cc
@@ -125,13 +125,24 @@ class OfflinePageModelImplTest
       const std::vector<ClientId>& client_ids);
   void DeletePagesByClientIds(const std::vector<ClientId>& client_ids);
 
-  // Returns the offline ID of the saved page.
-  std::pair<SavePageResult, int64_t> SavePage(const GURL& url,
-                                              ClientId client_id);
+  // 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,
-                                  ClientId client_id,
+                                  const ClientId& client_id,
                                   OfflinePageArchiver::ArchiverResult result);
+  // Returns the offline ID of the saved page.
+  std::pair<SavePageResult, int64_t> SavePage(const GURL& url,
+                                              const ClientId& client_id);
 
   void DeletePage(int64_t offline_id, const DeletePageCallback& callback) {
     std::vector<int64_t> offline_ids;
@@ -306,24 +317,44 @@ 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) {
-  SavePageWithArchiverResult(
-      url, client_id,
-      OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED);
-  return std::make_pair(last_save_result_, last_save_offline_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,
-    ClientId client_id,
+    const 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();
+  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,
+      OfflinePageArchiver::ArchiverResult::SUCCESSFULLY_CREATED);
+  return std::make_pair(last_save_result_, last_save_offline_id_);
 }
 
 MultipleOfflinePageItemResult OfflinePageModelImplTest::GetAllPages() {
@@ -435,7 +466,12 @@ bool OfflinePageModelImplTest::HasPages(std::string name_space) {
 
 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();
@@ -452,7 +488,7 @@ TEST_F(OfflinePageModelImplTest, SavePageSuccessful) {
 
   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);
@@ -461,6 +497,7 @@ TEST_F(OfflinePageModelImplTest, SavePageSuccessful) {
   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) {
@@ -495,27 +532,24 @@ 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());
 }
 
@@ -526,9 +560,8 @@ TEST_F(OfflinePageModelImplTest, SavePageOfflineArchiverTwoPages) {
   // 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.
@@ -842,27 +875,13 @@ 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());