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

components/offline_pages/offline_page_storage_manager.cc

 // Copyright 2016 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_storage_manager.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/time/clock.h"
@@ skipping 33 matching lines @@
 }
 
 void OfflinePageStorageManager::OnGetStorageStatsDone(
     const ClearPagesCallback& callback,
     const ArchiveManager::StorageStats& stats) {
   DCHECK(in_progress_);
   ClearMode mode = ShouldClearPages(stats);
   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,
                                   weak_ptr_factory_.GetWeakPtr(), callback,
                                   stats));
 }
 
 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_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) {
+    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.
   // Sort each vector based on last accessed time and all pages after index
   // min{size(), page_limit} should be expired. And then start iterating
   // backwards to expire pages.
   std::map<std::string, std::vector<OfflinePageItem>> pages_map;
   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;
     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;
               });
 
     int page_list_size = static_cast<int>(page_list.size());
     int pos = 0;
     while (pos < page_list_size &&
            (policy.page_limit == kUnlimitedPages || pos < policy.page_limit) &&
            !ShouldBeExpired(now, page_list.at(pos))) {
       kept_pages_size += page_list.at(pos).file_size;
       kept_pages.push_back(page_list.at(pos));
       pos++;
     }
 
     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
   // pages from oldest last access time.
   int64_t free_space = stats.free_disk_space;
   int64_t total_size = stats.total_archives_size;
   int64_t space_to_release =
       kept_pages_size -
       (total_size + free_space) * kOfflinePageStorageClearThreshold;
   if (space_to_release > 0) {
     // Here we're sorting the |kept_pages| with oldest first.
     std::sort(kept_pages.begin(), kept_pages.end(),
               [](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;
     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++;
     }
   }
 }
 
 OfflinePageStorageManager::ClearMode
 OfflinePageStorageManager::ShouldClearPages(
     const ArchiveManager::StorageStats& storage_stats) {
   int64_t total_size = storage_stats.total_archives_size;
   int64_t free_space = storage_stats.free_disk_space;
@@ skipping 17 matching lines @@
 }
 
 bool OfflinePageStorageManager::ShouldBeExpired(const base::Time& now,
                                                 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