[Offline Pages] Removes two-step expiration related.

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 28fa130002123d0122577f9485ba15f8ece573ec..957dab872296f323642b2254f4cec4b29f345049 100644
--- a/components/offline_pages/offline_page_storage_manager.h
+++ b/components/offline_pages/offline_page_storage_manager.h
@@ -25,19 +25,16 @@ namespace offline_pages {
 // Maximum % of total available storage that will be occupied by offline pages
 // before a storage clearup.
 const double kOfflinePageStorageLimit = 0.3;
-// The target % of storage usage we try to reach below when expiring pages.
+// The target % of storage usage we try to reach below when deleting pages.
 const double kOfflinePageStorageClearThreshold = 0.1;
 // The time that the storage cleanup will be triggered again since the last one.
 const base::TimeDelta kClearStorageInterval = base::TimeDelta::FromMinutes(10);
-// The time that the page record will be removed from the store since the page
-// has been expired.
-const base::TimeDelta kRemovePageItemInterval = base::TimeDelta::FromDays(21);
 
 class ClientPolicyController;
 class OfflinePageModel;
 
 // This class is used for storage management of offline pages. It provides
-// a ClearPagesIfNeeded method which is used to clear expired offline pages
+// a ClearPagesIfNeeded method which is used to clear outdated offline pages
 // based on last_access_time and lifetime policy of its namespace.
 // It has its own throttle mechanism so calling the method would not be
 // guaranteed to clear the pages immediately.
@@ -47,11 +44,9 @@ class OfflinePageModel;
 class OfflinePageStorageManager {
  public:
   enum class ClearStorageResult {

[jianli, 2016/11/18 00:15:48]

Do you want to update the histogram for this?

[romax, 2016/11/18 20:50:49]

Yes, I'm following the comment from Filip below to change names. I guess I have
to do the same change in histograms.xml as well.

-    SUCCESS,                     // Cleared successfully.
-    UNNECESSARY,                 // No expired pages.
-    EXPIRE_FAILURE,              // Expiration failed.
-    DELETE_FAILURE,              // Deletion failed.
-    EXPIRE_AND_DELETE_FAILURES,  // Both expiration and deletion failed.
+    SUCCESS,        // Cleared successfully.
+    UNNECESSARY,    // No expired pages.
+    CLEAR_FAILURE,  // Deletion failed.
     // NOTE: always keep this entry at the end. Add new result types only

[fgorski, 2016/11/18 00:13:04]

Read this note before making changes.

Mark the impossible values as DEPRECATED_ but leave them in.
Do the same in histograms.

Keep the name DELETE_FAILURE.

...

Alternatively define a new enum

[romax, 2016/11/18 20:50:49]

Done but not sure, can you please take a close look?

     // immediately above this line. Make sure to update the corresponding
     // histogram enum accordingly.
@@ -59,8 +54,8 @@ class OfflinePageStorageManager {
   };
 
   // Callback used when calling ClearPagesIfNeeded.
-  // size_t: the number of expired pages.
-  // ClearStorageResult: result of expiring pages in storage.
+  // size_t: the number of cleared pages.
+  // ClearStorageResult: result of clearing pages in storage.
   typedef base::Callback<void(size_t, ClearStorageResult)> ClearStorageCallback;
 
   explicit OfflinePageStorageManager(OfflinePageModel* model,
@@ -82,11 +77,11 @@ class OfflinePageStorageManager {
  private:
   // Enum indicating how to clear the pages.
   enum class ClearMode {
-    // Using normal expiration logic to expire pages. Will reduce the storage
+    // Using normal expiration logic to clear 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.)
+    // No need to clear any page (no pages in the model or no expired pages and
+    // we're not exceeding the storage limit.)
     NOT_NEEDED,
   };
 
@@ -101,32 +96,22 @@ class OfflinePageStorageManager {
       const ArchiveManager::StorageStats& storage_stats,
       const MultipleOfflinePageItemResult& pages);
 
-  // Callback called after expired pages have been deleted.
-  void OnPagesExpired(const ClearStorageCallback& callback,
-                      size_t pages_to_clear,
-                      const std::vector<int64_t>& page_ids_to_remove,
-                      bool expiration_succeeded);
-
-  // Callback called after clearing outdated pages from model.
-  void OnOutdatedPagesCleared(const ClearStorageCallback& callback,
-                              size_t pages_cleared,
-                              bool expiration_succeeded,
-                              DeletePageResult result);
-
-  // Gets offline IDs of both pages that should be expired and the ones that
-  // need to be removed from metadata store. |page_ids_to_expire| will have
-  // the pages to be expired, |page_ids_to_remove| will have the pages to be
-  // removed.
+  // Callback called after clearing expired pages from model.
+  void OnExpiredPagesCleared(const ClearStorageCallback& callback,

[fgorski, 2016/11/18 00:13:04]

Expired... part should probably go.

[romax, 2016/11/18 20:50:49]

Acknowledged.

+                             size_t pages_cleared,
+                             DeletePageResult result);
+
+  // Gets offline IDs of pages that should be cleared based on current |stats|
+  // and return the IDs in |page_ids_to_clear|.
   void GetPageIdsToClear(const MultipleOfflinePageItemResult& pages,
                          const ArchiveManager::StorageStats& stats,
-                         std::vector<int64_t>* page_ids_to_expire,
-                         std::vector<int64_t>* page_ids_to_remove);
+                         std::vector<int64_t>* page_ids_to_clear);
 
   // Determines if manager should clear pages.
   ClearMode ShouldClearPages(const ArchiveManager::StorageStats& storage_stats);
 
-  // Returns true if |page| is expired comparing to |clear_time_|.
-  bool ShouldBeExpired(const OfflinePageItem& page) const;
+  // Returns true if |page| is should be cleared based on |clear_time_|.
+  bool IsExpired(const OfflinePageItem& page) const;

[fgorski, 2016/11/18 00:13:04]

Explain why we are still using the concept of expired? Is that for cached pages?

[romax, 2016/11/18 20:50:49]

Yes expiration should still be a valid term for the pages which have been living
longer than the value defined in the expiration_policy. And this mechanism is
only for Offline Page Cache at the moment.

 
   // Returns true if we're currently doing a cleanup.
   bool IsInProgress() const;