[Offline Pages] Two-step expiration in storage manager.

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 794bec743a53039dc339b7cfdf855ce28ce31edb..3383de3b3524b96c6ab59b2f3aa0f17041f28d7d 100644
--- a/components/offline_pages/offline_page_storage_manager.cc
+++ b/components/offline_pages/offline_page_storage_manager.cc
@@ -47,7 +47,8 @@ void OfflinePageStorageManager::OnGetStorageStatsDone(
     const ClearPagesCallback& callback,
     const ArchiveManager::StorageStats& stats) {
   DCHECK(in_progress_);
-  ClearMode mode = ShouldClearPages(stats);
+  base::Time now = clock_->Now();

[romax, 2016/05/25 20:05:24]

this time would be used throughout the whole clear process(except setting
last_clear_time), it would be helpful to keep consistency. WDYT?

+  ClearMode mode = ShouldClearPages(stats, now);
   if (mode == ClearMode::NOT_NEEDED) {
     in_progress_ = false;
     last_clear_time_ = clock_->Now();
@@ -56,39 +57,60 @@ void OfflinePageStorageManager::OnGetStorageStatsDone(
   }
   client_->GetAllPages(base::Bind(&OfflinePageStorageManager::OnGetAllPagesDone,
                                   weak_ptr_factory_.GetWeakPtr(), callback,
-                                  stats));
+                                  stats, now));
 }
 
 void OfflinePageStorageManager::OnGetAllPagesDone(
     const ClearPagesCallback& callback,
     const ArchiveManager::StorageStats& stats,
+    const base::Time& now,
     const MultipleOfflinePageItemResult& pages) {
-  std::vector<int64_t> offline_ids;
-  GetExpiredPageIds(pages, stats, offline_ids);
+  std::vector<int64_t>* page_ids_to_expire = new std::vector<int64_t>();

[romax, 2016/05/25 20:05:24]

Jian Please take a look about these 2 pointers.. I'm not sure if I'm leaking
memory here.

+  std::vector<int64_t>* page_ids_to_remove = new std::vector<int64_t>();
+  GetPageIdsToClear(pages, stats, now, page_ids_to_expire, page_ids_to_remove);
+  client_->ExpirePages(
+      *(base::Owned(page_ids_to_expire).get()), now,
+      base::Bind(&OfflinePageStorageManager::OnPagesExpired,
+                 weak_ptr_factory_.GetWeakPtr(), callback,
+                 page_ids_to_expire->size(), base::Owned(page_ids_to_remove)));
+}
+
+void OfflinePageStorageManager::OnPagesExpired(
+    const ClearPagesCallback& callback,
+    size_t pages_expired,
+    std::vector<int64_t>* page_ids_to_remove,
+    bool success) {
   client_->DeletePagesByOfflineId(
-      offline_ids, base::Bind(&OfflinePageStorageManager::OnExpiredPagesDeleted,
-                              weak_ptr_factory_.GetWeakPtr(), callback,
-                              static_cast<int>(offline_ids.size())));
+      *page_ids_to_remove,
+      base::Bind(&OfflinePageStorageManager::OnDeadPagesCleared,
+                 weak_ptr_factory_.GetWeakPtr(), callback, pages_expired,
+                 success));
 }
 
-void OfflinePageStorageManager::OnExpiredPagesDeleted(
+void OfflinePageStorageManager::OnDeadPagesCleared(
     const ClearPagesCallback& callback,
-    int pages_cleared,
+    size_t pages_cleared,
+    bool success,
     DeletePageResult result) {
   last_clear_time_ = clock_->Now();
-  ClearStorageResult clear_result = result == DeletePageResult::SUCCESS
-                                        ? ClearStorageResult::SUCCESS
-                                        : ClearStorageResult::DELETE_FAILURE;
+  ClearStorageResult clear_result;
+  if (!success) {
+    clear_result = ClearStorageResult::EXPIRE_FAILURE;
+  } else {
+    clear_result = result == DeletePageResult::SUCCESS
+                       ? ClearStorageResult::SUCCESS
+                       : ClearStorageResult::DELETE_FAILURE;
+  }
   in_progress_ = false;
   callback.Run(pages_cleared, clear_result);
 }
 
-void OfflinePageStorageManager::GetExpiredPageIds(
+void OfflinePageStorageManager::GetPageIdsToClear(
     const MultipleOfflinePageItemResult& pages,
     const ArchiveManager::StorageStats& stats,
-    std::vector<int64_t>& offline_ids) {
-  base::Time now = clock_->Now();
-
+    const base::Time& now,
+    std::vector<int64_t>* page_ids_to_expire,
+    std::vector<int64_t>* page_ids_to_remove) {
   // 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
@@ -97,8 +119,13 @@ void OfflinePageStorageManager::GetExpiredPageIds(
   std::vector<OfflinePageItem> kept_pages;
   int64_t kept_pages_size = 0;
 
-  for (const auto& page : pages)
-    pages_map[page.client_id.name_space].push_back(page);
+  for (const auto& page : pages) {
+    if (!page.IsExpired()) {
+      pages_map[page.client_id.name_space].push_back(page);
+    } else if (now - page.expiration_time >= kRemovePageItemInterval) {
+      page_ids_to_remove->push_back(page.offline_id);
+    }
+  }
 
   for (auto& iter : pages_map) {
     std::string name_space = iter.first;
@@ -112,8 +139,8 @@ void OfflinePageStorageManager::GetExpiredPageIds(
                 return a.last_access_time > b.last_access_time;
               });
 
-    int page_list_size = static_cast<int>(page_list.size());
-    int pos = 0;
+    size_t page_list_size = page_list.size();
+    size_t pos = 0;
     while (pos < page_list_size &&
            (policy.page_limit == kUnlimitedPages || pos < policy.page_limit) &&
            !ShouldBeExpired(now, page_list.at(pos))) {
@@ -123,7 +150,7 @@ void OfflinePageStorageManager::GetExpiredPageIds(
     }
 
     for (; pos < page_list_size; pos++)
-      offline_ids.push_back(page_list.at(pos).offline_id);
+      page_ids_to_expire->push_back(page_list.at(pos).offline_id);
   }
 
   // If we're still over the clear threshold, we're going to clear remaining
@@ -139,11 +166,11 @@ void OfflinePageStorageManager::GetExpiredPageIds(
               [](const OfflinePageItem& a, const OfflinePageItem& b) -> bool {
                 return a.last_access_time < b.last_access_time;
               });
-    int kept_pages_size = static_cast<int>(kept_pages.size());
-    int pos = 0;
+    size_t kept_pages_size = kept_pages.size();
+    size_t pos = 0;
     while (pos < kept_pages_size && space_to_release > 0) {
       space_to_release -= kept_pages.at(pos).file_size;
-      offline_ids.push_back(kept_pages.at(pos).offline_id);
+      page_ids_to_expire->push_back(kept_pages.at(pos).offline_id);
       pos++;
     }
   }
@@ -151,7 +178,8 @@ void OfflinePageStorageManager::GetExpiredPageIds(
 
 OfflinePageStorageManager::ClearMode
 OfflinePageStorageManager::ShouldClearPages(
-    const ArchiveManager::StorageStats& storage_stats) {
+    const ArchiveManager::StorageStats& storage_stats,
+    const base::Time& now) {
   int64_t total_size = storage_stats.total_archives_size;
   int64_t free_space = storage_stats.free_disk_space;
   if (total_size == 0)
@@ -165,7 +193,7 @@ OfflinePageStorageManager::ShouldClearPages(
   // If it's been more than the pre-defined interval since the last time we
   // clear the storage, we should clear pages.
   if (last_clear_time_ == base::Time() ||
-      clock_->Now() - last_clear_time_ >= kClearStorageInterval) {
+      now - last_clear_time_ >= kClearStorageInterval) {
     return ClearMode::DEFAULT;
   }