[Offline Pages] Adding more expiration logic.

components/offline_pages/offline_page_storage_manager.cc

Index: components/offline_pages/offline_page_storage_manager.cc
diff --git a/components/offline_pages/offline_page_storage_manager.cc b/components/offline_pages/offline_page_storage_manager.cc
index a4465e833d2fda1c386d2b640f21c462407d2b46..b0114192011fd99322fd4e52cad70ef2631485f9 100644
--- a/components/offline_pages/offline_page_storage_manager.cc
+++ b/components/offline_pages/offline_page_storage_manager.cc
@@ -4,6 +4,8 @@
 
 #include "components/offline_pages/offline_page_storage_manager.h"
 
+#include <algorithm>
+
 #include "base/bind.h"
 #include "components/offline_pages/client_policy_controller.h"
 #include "components/offline_pages/offline_page_client_policy.h"
@@ -46,9 +48,40 @@ void OfflinePageStorageManager::ClearExpiredPages(
 void OfflinePageStorageManager::GetExpiredPageIds(
     const MultipleOfflinePageItemResult& pages,
     std::vector<int64_t>& offline_ids) {
+  base::Time now = base::Time::Now();

[fgorski, 2016/05/12 14:49:32]

use base::Clock for tracking time. ability to inject the clock allows you to
control the time in tests.

[romax, 2016/05/12 22:20:03]

Done.

+
+  // Creating a map from namespace to a vector of page items.
+  // Sort each vector based on last accessed time and all pages after index
+  // min{size(), page_limit} should be expired. And then start iterating

[fgorski, 2016/05/12 14:49:32]

did you consider an alternative, where you sort by namespace and
last_access_time?

[romax, 2016/05/12 22:20:03]

I thought it's more convenient and intuitive to use a map... 

+  // backwards to expire pages.
+  std::map<std::string, std::vector<OfflinePageItem>> pages_map;
+  pages_map.clear();

[fgorski, 2016/05/12 14:49:32]

is this necessary?

[romax, 2016/05/12 22:20:03]

it's not

+
   for (const auto& page : pages) {

[fgorski, 2016/05/12 14:49:32]

nit: single condition with single line if/for/while/.. don't require {}, but you
can leave it in. (This is based Chromium style guide). 

Applies to the while below (where braces are required)
and inner for where they are optional. 

[romax, 2016/05/12 22:20:03]

Done.

-    if (IsPageExpired(page))
-      offline_ids.push_back(page.offline_id);
+    pages_map[page.client_id.name_space].push_back(page);
+  }
+  for (auto& iter : pages_map) {
+    std::string name_space = iter.first;
+    std::vector<OfflinePageItem>& page_list = iter.second;
+
+    LifetimePolicy policy =
+        policy_controller_->GetPolicy(name_space).lifetime_policy;
+
+    std::sort(page_list.begin(), page_list.end(),
+              [](const OfflinePageItem& a, const OfflinePageItem& b) -> bool {
+                return a.last_access_time > b.last_access_time;
+              });

[fgorski, 2016/05/12 14:49:32]

nit: put an empty line here, and remove the one before while, these variables
are about that while loop mostly.

[romax, 2016/05/12 22:20:03]

Done.

+    int page_list_size = page_list.size();
+    int pos = 0;
+
+    while (pos < page_list_size &&
+           (policy.page_limit == kUnlimitedPages || pos < policy.page_limit) &&
+           !IsPageExpired(now, page_list.at(pos)))
+      pos++;
+
+    for (int i = pos; i < page_list_size; i++) {
+      offline_ids.push_back(page_list.at(i).offline_id);
+    }
   }
 }
 
@@ -64,11 +97,10 @@ bool OfflinePageStorageManager::ShouldClearPages() {
   return !in_progress_;
 }
 
-bool OfflinePageStorageManager::IsPageExpired(const OfflinePageItem& page) {
-  base::Time now = base::Time::Now();
+bool OfflinePageStorageManager::IsPageExpired(const base::Time& now,

[fgorski, 2016/05/12 14:49:32]

Can you change the name to ShouldBeExpired? This will give us a leg up on two
step expiration.

[romax, 2016/05/12 22:20:03]

Done.

+                                              const OfflinePageItem& page) {
   const LifetimePolicy& policy =
       policy_controller_->GetPolicy(page.client_id.name_space).lifetime_policy;
   return now - page.last_access_time > policy.expiration_period;
 }
-
 }  // namespace offline_pages