[Offline Pages] Two-step expiration 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 47f560120d929cfe10b80358f35b631c65796aae..fe976d075ecab78e9ff411f46de38eab4aac329c 100644
--- a/components/offline_pages/offline_page_storage_manager.h
+++ b/components/offline_pages/offline_page_storage_manager.h
@@ -24,10 +24,14 @@ namespace offline_pages {
 
 // 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.
+// reducing the usage below the 10% threshold.

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

nit: try to avoid mentioning the value in the comments because otherwise you
will have to update the comments if these constants get changed later. How
about:
// When the total storage space occupied by offline pages exceeds the
// threshold below, in terms of % of available storage, the storage clean
// up will be performed to reduce the usage.
const double kOfflinePageStorageLimit = 0.3;
// The target threshold, in terms of of available storage, which the storage
// clean up will try to reduce the usage to.
const double kOfflinePageStorageClearThreshold = 0.1;

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

Done.

 const double kOfflinePageStorageLimit = 0.3;
 const double kOfflinePageStorageClearThreshold = 0.1;
+// If it's more than 10 minutes since the last storage clearing, it's a good
+// time to clear storage again to check for expired pages. And if a page has
+// been expired more than 21 days it should be deleted from metadata store.

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

ditto

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

Done.

 const base::TimeDelta kClearStorageInterval = base::TimeDelta::FromMinutes(10);
+const base::TimeDelta kRemovePageItemInterval = base::TimeDelta::FromDays(21);
 
 class ArchiveManager;
 class ClientPolicyController;
@@ -52,10 +56,17 @@ class OfflinePageStorageManager {
     virtual void GetAllPages(
         const MultipleOfflinePageItemCallback& callback) = 0;
 
-    // Asks the client to delete pages based on |offline_ids| and invoke
-    // |callback|.
-    virtual void DeletePagesByOfflineId(const std::vector<int64_t>& offline_ids,
-                                        const DeletePageCallback& callback) = 0;
+    // Asks the client to mark pages with |offline_ids| as expired and the
+    // associated archive files.
+    virtual void ExpirePages(const std::vector<int64_t>& offline_ids,
+                             const base::Time& expiration_time,
+                             const DeletePageCallback& callback) = 0;

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

nit: try not to use DeletePageCallback since Delete in the name is not really
consistent with Expire here. Probably add a new callback and enum result. Or
rename DeletePageResult to UpdatePageResult.

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

Done.
I realized there's no need to add a enum here. I'm going to change it to a
callback with only bool indicating if the expiration is successful or not.

+
+    // Asks the client to remove page items with |offline_ids| from metadata
+    // store and in memory mapping.
+    virtual void RemovePageItems(
+        const std::vector<int64_t>& offline_ids,
+        const base::Callback<void(bool)>& callback) = 0;
   };
 
   enum class ClearStorageResult {
@@ -67,7 +78,9 @@ class OfflinePageStorageManager {
   // Callback used when calling ClearPagesIfNeeded.
   // int: the number of expired pages.
   // ClearStorageResult: result of expiring pages in storage.
-  typedef base::Callback<void(int, ClearStorageResult)> ClearPagesCallback;
+  // bool: true if removing dead pages from metadata store is successful.
+  typedef base::Callback<void(int, ClearStorageResult, bool)>
+      ClearPagesCallback;
 
   explicit OfflinePageStorageManager(Client* client,
                                      ClientPolicyController* policy_controller,
@@ -108,12 +121,20 @@ class OfflinePageStorageManager {
   // Callback called after expired pages have been deleted.
   void OnExpiredPagesDeleted(const ClearPagesCallback& callback,
                              int pages_to_clear,
+                             const std::vector<int64_t>& page_ids_to_remove,
                              DeletePageResult result);
 
+  // Callback called after clearing outdated pages from client.
+  void OnExpiredPagesCleared(const ClearPagesCallback& callback,
+                             int pages_cleared,
+                             DeletePageResult result,
+                             bool success);
+
   // Gets offline IDs of all expired pages and return in |offline_ids|.

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

Please update the comment. Please also explain the difference between
page_ids_to_expire and page_ids_to_remove.

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

Done.

   void GetExpiredPageIds(const MultipleOfflinePageItemResult& pages,
                          const ArchiveManager::StorageStats& stats,
-                         std::vector<int64_t>& offline_ids);
+                         std::vector<int64_t>& page_ids_to_expire,
+                         std::vector<int64_t>& page_ids_to_remove);
 
   // Determine if manager should clear pages.
   ClearMode ShouldClearPages(const ArchiveManager::StorageStats& storage_stats);