[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..1ee2a8c8803646311ff69dc698eee653166c352a 100644
--- a/components/offline_pages/offline_page_storage_manager.cc
+++ b/components/offline_pages/offline_page_storage_manager.cc
@@ -7,6 +7,7 @@
 #include <algorithm>
 
 #include "base/bind.h"
+#include "base/sys_info.h"
 #include "base/time/clock.h"
 #include "base/time/default_clock.h"
 #include "base/time/time.h"
@@ -14,14 +15,17 @@
 #include "components/offline_pages/offline_page_client_policy.h"
 #include "components/offline_pages/offline_page_item.h"
 #include "components/offline_pages/offline_page_types.h"
+#include "components/prefs/pref_service.h"
 
 namespace offline_pages {
 
 OfflinePageStorageManager::OfflinePageStorageManager(
     Client* client,
-    ClientPolicyController* policy_controller)
+    ClientPolicyController* policy_controller,
+    PrefService* prefs)
     : client_(client),
       policy_controller_(policy_controller),
+      prefs_(prefs),
       in_progress_(false),
       clock_(new base::DefaultClock()),
       weak_ptr_factory_(this) {}
@@ -30,7 +34,7 @@ OfflinePageStorageManager::~OfflinePageStorageManager() {}
 
 void OfflinePageStorageManager::ClearPagesIfNeeded(
     const ClearPageCallback& callback) {
-  if (!ShouldClearPages())
+  if (in_progress_)
     return;
   in_progress_ = true;
   client_->GetAllPages(base::Bind(&OfflinePageStorageManager::ClearExpiredPages,
@@ -41,8 +45,28 @@ void OfflinePageStorageManager::ClearExpiredPages(
     const ClearPageCallback& callback,
     const MultipleOfflinePageItemResult& pages) {
   DCHECK(in_progress_);
+
+  ClearMode mode = ShouldClearPages(pages);
   std::vector<int64_t> offline_ids;
-  GetExpiredPageIds(pages, offline_ids);
+
+  switch (mode) {
+    case ClearMode::CLEAR_ALL:
+      for (const auto& page : pages)
+        offline_ids.push_back(page.offline_id);
+      break;
+    case ClearMode::DEFAULT:
+      GetExpiredPageIds(pages, offline_ids);
+      break;
+    case ClearMode::NO_NEED:
+      in_progress_ = false;
+      if (prefs_) {
+        prefs_->SetInt64(kOfflinePageStorageLastClearedTime,
+                         clock_->Now().ToInternalValue());
+      }
+      callback.Run(0, StorageClearResult::UNNECESSARY);
+      return;
+  }
+
   client_->DeletePagesByOfflineId(
       offline_ids,
       base::Bind(&OfflinePageStorageManager::OnExpiredPagesDeleted,
@@ -97,19 +121,56 @@ void OfflinePageStorageManager::OnExpiredPagesDeleted(
     const ClearPageCallback& callback,
     int pages_cleared,
     DeletePageResult result) {
+  if (prefs_) {
+    prefs_->SetInt64(kOfflinePageStorageLastClearedTime,
+                     clock_->Now().ToInternalValue());
+  }
+  StorageClearResult clear_result = result == DeletePageResult::SUCCESS
+                                        ? StorageClearResult::SUCCESS
+                                        : StorageClearResult::DELETE_FAIL;
   in_progress_ = false;
-  callback.Run(pages_cleared, result);
+  callback.Run(pages_cleared, clear_result);
 }
 
-bool OfflinePageStorageManager::ShouldClearPages() {
-  return !in_progress_;
+OfflinePageStorageManager::ClearMode
+OfflinePageStorageManager::ShouldClearPages(
+    const MultipleOfflinePageItemResult& pages) {
+  if (pages.size() == 0)
+    return ClearMode::NO_NEED;
+
+  // If it's been more than the pre-defined gap since the last time we clear the

[fgorski, 2016/05/17 05:33:29]

interval

[romax, 2016/05/18 02:36:19]

Done.

+  // storage, we should clear pages.
+  if (prefs_) {
+    int64_t last_clear_time =
+        prefs_->GetInt64(kOfflinePageStorageLastClearedTime);
+    if (last_clear_time == 0 ||
+        clock_->Now() - base::Time::FromInternalValue(last_clear_time) >=
+            kClearStorageTimeGap) {
+      return ClearMode::DEFAULT;
+    }
+  }
+
+  // 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.
+  int64_t total_size = 0;
+  for (const auto& page : pages)
+    total_size += page.file_size;
+
+  int64_t free_space = base::SysInfo::AmountOfFreeDiskSpace(pages[0].file_path);

[fgorski, 2016/05/17 05:33:29]

This requires IO Thread and to my knowledge you are on UI...

+  if (total_size > kOfflinePageStorageLimit ||
+      total_size >
+          (total_size + free_space) * kOfflinePageStorageLimitPercentage) {
+    return ClearMode::CLEAR_ALL;

[fgorski, 2016/05/17 05:33:29]

This is too aggressive. Why would you remove everything?

[romax, 2016/05/18 02:36:19]

will change per discussion

+  }
+  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