[Offline pages] Adding expiration capability to OfflinePageModel

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 62bb1a2920fa2fb4c3cc917d101906870bbd78ee..c7dc7ab3c33acddbf773d48137a76a3b9dac054a 100644
--- a/components/offline_pages/offline_page_model_unittest.cc
+++ b/components/offline_pages/offline_page_model_unittest.cc
@@ -956,6 +956,42 @@ TEST_F(OfflinePageModelTest, GetBestPage) {
   EXPECT_EQ(std::get<1>(saved_pages[1]), offline_page->offline_id);
 }
 
+TEST_F(OfflinePageModelTest, ExpirePages) {
+  // We will save 3 pages - two for the same URL, and one for a different URL.

[jianli, 2016/05/19 20:39:54]

The comment "two for the same URL" does not match with the code.

[fgorski, 2016/05/20 17:54:23]

Done.

+  // Correct behavior will pick the most recently saved page for the correct
+  // URL.
+  std::pair<SavePageResult, int64_t> saved_pages[3];

[jianli, 2016/05/19 20:39:54]

I found out that this pattern makes the code harder to understand. Since
probably all of the time we only care about offline id, why not just having
SavePage return it? If some tests need to check the result, we could have
another helper function.

I know this behavior has nothing to do with your patch and I am not asking to
fix it now. We should probably consider the change in another patch.

[fgorski, 2016/05/20 17:54:23]

Acknowledged.

+  saved_pages[0] = SavePage(kTestUrl, kTestClientId1);
+  saved_pages[1] = SavePage(kTestUrl2, kTestClientId2);
+  saved_pages[1] = SavePage(kTestUrl3, kTestClientId3);

[jianli, 2016/05/19 20:39:54]

[1] => [2]?

[fgorski, 2016/05/20 17:54:23]

Done.

+
+  for (const auto& save_result : saved_pages) {
+    ASSERT_EQ(OfflinePageModel::SavePageResult::SUCCESS,
+              std::get<0>(save_result));
+  }
+
+  // First two pages will be expired.
+  std::vector<int64_t> pages_to_expire = {std::get<1>(saved_pages[0]),
+                                          std::get<1>(saved_pages[1])};
+  // Pages are marked as expired if they have an expiration_time set.
+  base::Time expiration_time =
+      base::Time::Now() + base::TimeDelta::FromMinutes(5);
+
+  model()->ExpirePages(pages_to_expire, expiration_time);
+  PumpLoop();
+  const std::vector<OfflinePageItem>& offline_pages = GetStore()->GetAllPages();

[jianli, 2016/05/19 20:39:54]

I think it would be better to verify against the in-memory cache since the cache
is updated after the store update. We should either verify both store and cache
or just cache.

[fgorski, 2016/05/20 17:54:23]

I can do it right now, but GetAllPages will filter out expired pages starting
the next patch...

+  for (const auto& offline_page : offline_pages) {
+    if (std::find(pages_to_expire.begin(), pages_to_expire.end(),
+                  offline_page.offline_id) != pages_to_expire.end()) {
+      EXPECT_EQ(expiration_time, offline_page.expiration_time);
+      EXPECT_TRUE(offline_page.IsExpired());
+    } else {
+      EXPECT_EQ(base::Time(), offline_page.expiration_time);
+      EXPECT_FALSE(offline_page.IsExpired());
+    }
+  }
+}
+
 TEST(CommandLineFlagsTest, OfflineBookmarks) {
   // Disabled by default.
   EXPECT_FALSE(offline_pages::IsOfflineBookmarksEnabled());