[Offline pages] Adding expiration capability to OfflinePageModel

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 938 matching lines @@
               std::get<0>(save_result));
   }
 
   const OfflinePageItem* offline_page =
       model()->MaybeGetBestPageForOnlineURL(kTestUrl);
   ASSERT_TRUE(nullptr != offline_page);
 
   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());
 
   // Check if feature is correctly enabled by command-line flag.
   base::FeatureList::ClearInstanceForTesting();
   std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
   feature_list->InitializeFromCommandLine(
       offline_pages::kOfflineBookmarksFeature.name, "");
   base::FeatureList::SetInstance(std::move(feature_list));
@@ skipping 40 matching lines @@
   std::unique_ptr<base::FeatureList> feature_list2(new base::FeatureList);
   feature_list2->InitializeFromCommandLine(
       std::string(offline_pages::kOfflineBookmarksFeature.name) + "," +
           offline_pages::kOfflinePagesBackgroundLoadingFeature.name,
       "");
   base::FeatureList::SetInstance(std::move(feature_list2));
   EXPECT_TRUE(offline_pages::IsOfflinePagesBackgroundLoadingEnabled());
 }
 
 }  // namespace offline_pages