[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..5ece044bccb6f7f6cb32769482543427cf396224 100644
--- a/components/offline_pages/offline_page_storage_manager.cc
+++ b/components/offline_pages/offline_page_storage_manager.cc
@@ -51,7 +51,7 @@ void OfflinePageStorageManager::OnGetStorageStatsDone(
   if (mode == ClearMode::NOT_NEEDED) {
     in_progress_ = false;
     last_clear_time_ = clock_->Now();
-    callback.Run(0, ClearStorageResult::UNNECESSARY);
+    callback.Run(0, ClearStorageResult::UNNECESSARY, true);
     return;
   }
   client_->GetAllPages(base::Bind(&OfflinePageStorageManager::OnGetAllPagesDone,
@@ -63,30 +63,47 @@ void OfflinePageStorageManager::OnGetAllPagesDone(
     const ClearPagesCallback& callback,
     const ArchiveManager::StorageStats& stats,
     const MultipleOfflinePageItemResult& pages) {
-  std::vector<int64_t> offline_ids;
-  GetExpiredPageIds(pages, stats, offline_ids);
-  client_->DeletePagesByOfflineId(
-      offline_ids, base::Bind(&OfflinePageStorageManager::OnExpiredPagesDeleted,
-                              weak_ptr_factory_.GetWeakPtr(), callback,
-                              static_cast<int>(offline_ids.size())));
+  std::vector<int64_t> page_ids_to_expire;
+  std::vector<int64_t> page_ids_to_remove;
+  GetExpiredPageIds(pages, stats, page_ids_to_expire, page_ids_to_remove);
+  client_->ExpirePages(
+      page_ids_to_expire, clock_->Now(),

[jianli, 2016/05/24 23:43:32]

"clock_->Now()" passed here will be different from the one used in
GetExpiredPageIds. This might cause subtle issues in the future.

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

Done.

+      base::Bind(&OfflinePageStorageManager::OnExpiredPagesDeleted,

[jianli, 2016/05/24 23:43:32]

I think OnExpiredPagesDeleted should be OnPagesExpired since expired pages are
still kept in the store.

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

Done.

+                 weak_ptr_factory_.GetWeakPtr(), callback,
+                 static_cast<int>(page_ids_to_expire.size()),

[jianli, 2016/05/24 23:43:32]

Why doing all these casts, instead of simply using size_t?

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

Done.

+                 page_ids_to_remove));
 }
 
 void OfflinePageStorageManager::OnExpiredPagesDeleted(
     const ClearPagesCallback& callback,
-    int pages_cleared,
+    int pages_expired,
+    const std::vector<int64_t>& page_ids_to_remove,
     DeletePageResult result) {
+  client_->RemovePageItems(
+      page_ids_to_remove,
+      base::Bind(&OfflinePageStorageManager::OnExpiredPagesCleared,

[jianli, 2016/05/24 23:43:32]

OnExpiredPagesCleared => OnOlderExpiredPagesDeleted

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

I'd prefer to assign names to pages in different lifetime.
Like offline pages becomes 'expired offline pages' when it was expired(archive
deleted and metadata marked as expired), and an 'expired offline page' would
become a 'dead offline page' after 21 days passes since last access. WDYT?

+                 weak_ptr_factory_.GetWeakPtr(), callback, pages_expired,
+                 result));
+}
+
+void OfflinePageStorageManager::OnExpiredPagesCleared(
+    const ClearPagesCallback& callback,
+    int pages_cleared,
+    DeletePageResult result,
+    bool success) {
   last_clear_time_ = clock_->Now();
   ClearStorageResult clear_result = result == DeletePageResult::SUCCESS
                                         ? ClearStorageResult::SUCCESS
                                         : ClearStorageResult::DELETE_FAILURE;
   in_progress_ = false;
-  callback.Run(pages_cleared, clear_result);
+  callback.Run(pages_cleared, clear_result, success);
 }
 
 void OfflinePageStorageManager::GetExpiredPageIds(

[jianli, 2016/05/24 23:43:32]

Since now we return not just expired page ids, it would be better to rename this
method to something like GetPageIdsToCleanUp.

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

Done.

     const MultipleOfflinePageItemResult& pages,
     const ArchiveManager::StorageStats& stats,
-    std::vector<int64_t>& offline_ids) {
+    std::vector<int64_t>& page_ids_to_expire,

[jianli, 2016/05/24 23:43:32]

I don't think passing as reference is allowed in our C++ style guidelines.

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

Done.

+    std::vector<int64_t>& page_ids_to_remove) {

[jianli, 2016/05/24 23:43:32]

ditto

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

Done.

   base::Time now = clock_->Now();
 
   // Creating a map from namespace to a vector of page items.
@@ -97,8 +114,14 @@ 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 (clock_->Now() - page.expiration_time >=

[jianli, 2016/05/24 23:43:32]

nit: cache clock_->Now() before for loop, to be more deterministic

Indeed you already cache it at line 107

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

Done.

+               kRemovePageItemInterval) {
+      page_ids_to_remove.push_back(page.offline_id);
+    }
+  }
 
   for (auto& iter : pages_map) {
     std::string name_space = iter.first;
@@ -123,7 +146,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
@@ -143,7 +166,7 @@ void OfflinePageStorageManager::GetExpiredPageIds(
     int 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++;
     }
   }