[Offline Pages] Updated clearing logic 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 249427d6069ea4b772fd444a940dfc20df53c7b2..8f5c7daa73a6cb2e08ab7918813848bcd03050ff 100644
--- a/components/offline_pages/offline_page_storage_manager.cc
+++ b/components/offline_pages/offline_page_storage_manager.cc
@@ -7,9 +7,11 @@
 #include <algorithm>
 
 #include "base/bind.h"

[fgorski, 2016/05/19 04:14:22]

needed?

[romax, 2016/05/19 18:34:42]

forgot to remove...

+#include "base/sys_info.h"
 #include "base/time/clock.h"
 #include "base/time/default_clock.h"
 #include "base/time/time.h"

[fgorski, 2016/05/19 04:14:22]

time.h already included in header.

[romax, 2016/05/19 18:34:42]

Done.

+#include "components/offline_pages/archive_manager.h"

[fgorski, 2016/05/19 04:14:22]

same

[romax, 2016/05/19 18:34:42]

Done.

 #include "components/offline_pages/client_policy_controller.h"
 #include "components/offline_pages/offline_page_client_policy.h"
 #include "components/offline_pages/offline_page_item.h"
@@ -19,9 +21,11 @@ namespace offline_pages {
 
 OfflinePageStorageManager::OfflinePageStorageManager(
     Client* client,
-    ClientPolicyController* policy_controller)
+    ClientPolicyController* policy_controller,
+    ArchiveManager* archive_manager)
     : client_(client),
       policy_controller_(policy_controller),
+      archive_manager_(archive_manager),
       in_progress_(false),
       clock_(new base::DefaultClock()),
       weak_ptr_factory_(this) {}
@@ -30,33 +34,63 @@ OfflinePageStorageManager::~OfflinePageStorageManager() {}
 
 void OfflinePageStorageManager::ClearPagesIfNeeded(
     const ClearPageCallback& callback) {
-  if (!ShouldClearPages())
+  if (in_progress_)
     return;
   in_progress_ = true;
-  client_->GetAllPages(base::Bind(&OfflinePageStorageManager::ClearExpiredPages,
-                                  weak_ptr_factory_.GetWeakPtr(), callback));
+  archive_manager_->GetStorageStats(
+      base::Bind(&OfflinePageStorageManager::OnGetStorageStatsDone,
+                 weak_ptr_factory_.GetWeakPtr(), callback));
 }
 
-void OfflinePageStorageManager::ClearExpiredPages(
+void OfflinePageStorageManager::SetClockForTesting(
+    std::unique_ptr<base::Clock> clock) {
+  clock_ = std::move(clock);
+}
+
+void OfflinePageStorageManager::OnGetStorageStatsDone(
     const ClearPageCallback& callback,
-    const MultipleOfflinePageItemResult& pages) {
+    const ArchiveManager::StorageStats& stats) {
   DCHECK(in_progress_);
+  ClearMode mode = ShouldClearPages(stats);
+  if (mode == ClearMode::NO_NEED) {
+    in_progress_ = false;
+    last_clear_time_ = clock_->Now();
+    callback.Run(0, StorageClearResult::UNNECESSARY);
+    return;
+  }
+  client_->GetAllPages(base::Bind(&OfflinePageStorageManager::OnGetAllPagesDone,
+                                  weak_ptr_factory_.GetWeakPtr(), callback,
+                                  stats));
+}
+
+void OfflinePageStorageManager::OnGetAllPagesDone(
+    const ClearPageCallback& callback,
+    const ArchiveManager::StorageStats& stats,
+    const MultipleOfflinePageItemResult& pages) {
   std::vector<int64_t> offline_ids;
-  GetExpiredPageIds(pages, offline_ids);
+  GetExpiredPageIds(pages, offline_ids, stats);
   client_->DeletePagesByOfflineId(
       offline_ids,
       base::Bind(&OfflinePageStorageManager::OnExpiredPagesDeleted,
                  weak_ptr_factory_.GetWeakPtr(), callback, offline_ids.size()));
 }
 
-void OfflinePageStorageManager::SetClockForTesting(
-    std::unique_ptr<base::Clock> clock) {
-  clock_ = std::move(clock);
+void OfflinePageStorageManager::OnExpiredPagesDeleted(
+    const ClearPageCallback& callback,
+    int pages_cleared,
+    DeletePageResult result) {
+  last_clear_time_ = clock_->Now();
+  StorageClearResult clear_result = result == DeletePageResult::SUCCESS
+                                        ? StorageClearResult::SUCCESS
+                                        : StorageClearResult::DELETE_FAILURE;
+  in_progress_ = false;
+  callback.Run(pages_cleared, clear_result);
 }
 
 void OfflinePageStorageManager::GetExpiredPageIds(
     const MultipleOfflinePageItemResult& pages,
-    std::vector<int64_t>& offline_ids) {
+    std::vector<int64_t>& offline_ids,

[fgorski, 2016/05/19 04:14:22]

Could you put this at the end. It feels much better when output parameters are
at the end.

[romax, 2016/05/19 18:34:42]

Done.

+    const ArchiveManager::StorageStats& stats) {
   base::Time now = clock_->Now();
 
   // Creating a map from namespace to a vector of page items.
@@ -64,6 +98,8 @@ void OfflinePageStorageManager::GetExpiredPageIds(
   // 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> pages_left;

[fgorski, 2016/05/19 04:14:22]

how about kept_pages?

[romax, 2016/05/19 18:34:42]

Done.

+  int64_t non_expired_usage = 0;

[fgorski, 2016/05/19 04:14:22]

how about kept_pages_size?

[romax, 2016/05/19 18:34:42]

Done.

 
   for (const auto& page : pages)
     pages_map[page.client_id.name_space].push_back(page);
@@ -85,31 +121,66 @@ void OfflinePageStorageManager::GetExpiredPageIds(
     while (pos < page_list_size &&
            (policy.page_limit == kUnlimitedPages || pos < policy.page_limit) &&
            !ShouldBeExpired(now, page_list.at(pos))) {
+      non_expired_usage += page_list.at(pos).file_size;
+      pages_left.push_back(page_list.at(pos));
       pos++;
     }
 
     for (int i = pos; i < page_list_size; i++)

[fgorski, 2016/05/19 04:14:22]

for (;pos < page_list_size; ++pos)

[romax, 2016/05/19 18:34:42]

Done.

       offline_ids.push_back(page_list.at(i).offline_id);
   }
-}
 
-void OfflinePageStorageManager::OnExpiredPagesDeleted(
-    const ClearPageCallback& callback,
-    int pages_cleared,
-    DeletePageResult result) {
-  in_progress_ = false;
-  callback.Run(pages_cleared, result);
+  // 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 =
+      non_expired_usage -
+      (total_size + free_space) * kOfflinePageStorageClearThreshold;
+  if (space_to_release > 0) {
+    std::sort(pages_left.begin(), pages_left.end(),

[fgorski, 2016/05/19 04:14:22]

make a comment that this lambda sorts in reverse order.

[romax, 2016/05/19 18:34:42]

Done.

+              [](const OfflinePageItem& a, const OfflinePageItem& b) -> bool {
+                return a.last_access_time < b.last_access_time;
+              });
+    int pos = 0;
+    while (space_to_release > 0) {
+      space_to_release -= pages_left.at(pos).file_size;
+      offline_ids.push_back(pages_left.at(pos).offline_id);
+      pos++;
+    }
+  }
 }
 
-bool OfflinePageStorageManager::ShouldClearPages() {
-  return !in_progress_;
+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;
+  if (total_size == 0)
+    return ClearMode::NO_NEED;
+
+  // If the size of all offline pages is more than limit, or it's larger than a
+  // specified percentage of all available storage space on the disk we'll clear
+  // all offline pages.
+  if (total_size >= (total_size + free_space) * kOfflinePageStorageLimit) {

[fgorski, 2016/05/19 04:14:22]

nit: {} not needed.

[romax, 2016/05/19 18:34:41]

Done.

+    return ClearMode::DEFAULT;
+  }
+  // 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) {
+    return ClearMode::DEFAULT;
+  }
+
+  // Otherwise there's no need to clear storage right now.
+  return ClearMode::NO_NEED;
 }
 
 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;
+  return now - page.last_access_time >= policy.expiration_period;
 }
 
 }  // namespace offline_pages