[Offline Pages] Updated clearing logic in storage manager.

components/offline_pages/offline_page_storage_manager.h

Index: components/offline_pages/offline_page_storage_manager.h
diff --git a/components/offline_pages/offline_page_storage_manager.h b/components/offline_pages/offline_page_storage_manager.h
index fd7f206ddfd639a15de483119e396a67ab7c791f..44031d133f5f2c56c657377960feb2d712acc953 100644
--- a/components/offline_pages/offline_page_storage_manager.h
+++ b/components/offline_pages/offline_page_storage_manager.h
@@ -12,6 +12,8 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/time/time.h"
+#include "components/offline_pages/archive_manager.h"

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

you can forward declare that one.

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

I'm also using ArchiveManager::StorageStats, so I guess I have to include this
header anyways?

 #include "components/offline_pages/offline_page_types.h"
 
 namespace base {
@@ -22,6 +24,14 @@ namespace offline_pages {
 
 class ClientPolicyController;
 
+// Limit of the total storage space occupied by offline pages should be 30% of
+// available storage. And we clear storage when it is over the threshold,
+// reducing the usage below threshold.
+static const double kOfflinePageStorageLimit = 0.3;

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

remove static, you are not inside of a class.

Also is it important to expose these values outside of StorageManager?

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

Done.
Moved them into an anonymous namespace.

+static const double kOfflinePageStorageClearThreshold = 0.1;
+static const base::TimeDelta kClearStorageInterval =
+    base::TimeDelta::FromMinutes(10);
+
 // This class is used for storage management of offline pages. It provides
 // a ClearPagesIfNeeded method which is used to clear expired offline pages
 // based on last_access_time and lifetime policy of its namespace.
@@ -48,40 +58,65 @@ class OfflinePageStorageManager {
                                         const DeletePageCallback& callback) = 0;
   };
 
+  enum class StorageClearResult {

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

ClearStorageResult 

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

Done.

+    SUCCESS,         // Cleared successfully.
+    UNNECESSARY,     // No expired pages.
+    DELETE_FAILURE,  // Deletion failed.
+  };
+
   // Callback used when calling ClearPagesIfNeeded.
-  // int: the number of deleted pages.
-  // DeletePageResult: result of deleting pages.
-  typedef base::Callback<void(int, DeletePageResult)> ClearPageCallback;
+  // int: the number of expired pages.
+  // StorageClearResult: result of expiring pages in storage.
+  typedef base::Callback<void(int, StorageClearResult)> ClearPageCallback;

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

ClearPagesCallback

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

Done.

 
   explicit OfflinePageStorageManager(Client* client,
-                                     ClientPolicyController* policy_controller);
+                                     ClientPolicyController* policy_controller,
+                                     ArchiveManager* archive_manager);
 
   ~OfflinePageStorageManager();
 
   // The manager would *try* to clear pages when called. It may not delete any
   // pages (if clearing condition wasn't satisfied).
+  // It clears the storage (expire pages) when it's using more disk space than a
+  // certain limit, or the time elapsed from last time clearing is longer than a
+  // certain interval. Both values are defined above.
   void ClearPagesIfNeeded(const ClearPageCallback& callback);
 
   // Sets the clock for testing.
   void SetClockForTesting(std::unique_ptr<base::Clock> clock);
 
  private:
-  // Selects and removes pages that need to be expired. Triggered as a callback
-  // to |GetAllPages|.
-  void ClearExpiredPages(const ClearPageCallback& callback,
-                         const MultipleOfflinePageItemResult& pages);
+  // Enum indicating how to clear the pages.
+  enum class ClearMode {
+    // Using normal expiration logic to expire pages. Will reduce the storage
+    // usage down below the threshold.
+    DEFAULT,
+    // No need to expire any page (no pages in the model or no expired
+    // pages and we're not exceeding the storage limit.)

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

why do we have unnecessary and no need?

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

I just want to provide a way to identify that we're returning with doing
nothing. I thought about when the return int in the callback is 0 it means we
did nothing but then i realized there might be a deletion failure. So I'm having
this.

+    NO_NEED,

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

NOT_NEEDED

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

Done.

+  };
 
-  // Gets offline IDs of all expired pages and return in |offline_ids|.
-  void GetExpiredPageIds(const MultipleOfflinePageItemResult& pages,
-                         std::vector<int64_t>& offline_ids);
+  // Callback called after getting storage stats from archive manager.
+  void OnGetStorageStatsDone(const ClearPageCallback& callback,
+                             const ArchiveManager::StorageStats& pages);
 
-  // Callback when expired pages has been deleted.
+  // Callback called after getting all pages from client done.
+  void OnGetAllPagesDone(const ClearPageCallback& callback,
+                         const ArchiveManager::StorageStats& storage_stats,
+                         const MultipleOfflinePageItemResult& pages);
+
+  // Callback called after expired pages have been deleted.
   void OnExpiredPagesDeleted(const ClearPageCallback& callback,
                              int pages_to_clear,
                              DeletePageResult result);
 
+  // Gets offline IDs of all expired pages and return in |offline_ids|.
+  void GetExpiredPageIds(const MultipleOfflinePageItemResult& pages,
+                         std::vector<int64_t>& offline_ids,
+                         const ArchiveManager::StorageStats& stats);
+
   // Determine if manager should clear pages.
-  bool ShouldClearPages();
+  ClearMode ShouldClearPages(const ArchiveManager::StorageStats& storage_stats);
 
   // Return true if |page| is expired comparing to |now|.
   bool ShouldBeExpired(const base::Time& now, const OfflinePageItem& page);
@@ -92,8 +127,13 @@ class OfflinePageStorageManager {
   // Not owned.
   ClientPolicyController* policy_controller_;
 
+  // Not owned.
+  ArchiveManager* archive_manager_;
+
   bool in_progress_;
 
+  base::Time last_clear_time_;
+
   // Clock for getting time.
   std::unique_ptr<base::Clock> clock_;