[Offline Pages] Two-step expiration in storage manager.

[Offline Pages] Two-step expiration in storage manager.

This patch expanded the two-step expiration from offline page model to
storage manager. Now the storage manager would expire a page and delete
the archived file when it wasn't used long enough and after a certain
time interval remove the page entry from metadata store.
Also added tests for corresponding cases.

BUG=605615
Committed: https://crrev.com/2b43a3e24f906a24f3a85c94892e3824cce521e7
Cr-Commit-Position: refs/heads/master@{#396513}

[romax, 2016-05-24 22:09:33]

romax@chromium.org changed reviewers:
+ fgorski@chromium.org

[romax, 2016-05-24 22:09:34]

PTAL, and we can talk if i'm on the wrong way...

[romax, 2016-05-24 22:37:52]

romax@chromium.org changed reviewers:
+ jianli@chromium.org
- fgorski@chromium.org

[romax, 2016-05-24 22:37:52]

PTAL

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

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:526:
DeletePagesByOfflineId(offline_ids, callback);
I don't think we want to delete offline page metadata record from the store. We
only intend to delete the offline file.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:225: void RemovePageItems(const
std::vector<int64_t>& offline_ids,
I think exposing this capability directly might be a bit risky, since the caller
might be accidentally remove offline metadata record without removing the
backing files. Suggest to narrow it down DeleteExpiredPages.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_storage_manager.cc (right):

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:70: page_ids_to_expire,
clock_->Now(),
"clock_->Now()" passed here will be different from the one used in
GetExpiredPageIds. This might cause subtle issues in the future.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:71:
base::Bind(&OfflinePageStorageManager::OnExpiredPagesDeleted,
I think OnExpiredPagesDeleted should be OnPagesExpired since expired pages are
still kept in the store.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:73:
static_cast<int>(page_ids_to_expire.size()),
Why doing all these casts, instead of simply using size_t?

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:84:
base::Bind(&OfflinePageStorageManager::OnExpiredPagesCleared,
OnExpiredPagesCleared => OnOlderExpiredPagesDeleted

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:102: void
OfflinePageStorageManager::GetExpiredPageIds(
Since now we return not just expired page ids, it would be better to rename this
method to something like GetPageIdsToCleanUp.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:105:
std::vector<int64_t>& page_ids_to_expire,
I don't think passing as reference is allowed in our C++ style guidelines.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:106:
std::vector<int64_t>& page_ids_to_remove) {
ditto

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:120: } else if
(clock_->Now() - page.expiration_time >=
nit: cache clock_->Now() before for loop, to be more deterministic

Indeed you already cache it at line 107

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_storage_manager.h (right):

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:27: // reducing the
usage below the 10% threshold.
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;

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:32: // been expired more
than 21 days it should be deleted from metadata store.
ditto

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:63: const
DeletePageCallback& callback) = 0;
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.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:133: // Gets offline IDs
of all expired pages and return in |offline_ids|.
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:24]

PTAL.
1. Deleted RemovePageItems, using existing DeletePagesByOfflineId, now it will
delete archived files first and then metadata, so if the page IDs are all
expired pages it would delete from store.
2. Made changes related with 1.
3. Replaced int with size_t to avoid static casts.
4. Changed some names of methods.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:526:
DeletePagesByOfflineId(offline_ids, callback);
On 2016/05/24 23:43:32, jianli wrote:
> I don't think we want to delete offline page metadata record from the store.
We
> only intend to delete the offline file.

Done.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:225: void RemovePageItems(const
std::vector<int64_t>& offline_ids,
On 2016/05/24 23:43:32, jianli wrote:
> I think exposing this capability directly might be a bit risky, since the
caller
> might be accidentally remove offline metadata record without removing the
> backing files. Suggest to narrow it down DeleteExpiredPages.

I've added a check in DeletePagesByOfflineId to delete offline_ids. If there's a
file associated it would delete the file, and then it would remove all
offline_ids from in-memory map or store.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_storage_manager.cc (right):

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:70: page_ids_to_expire,
clock_->Now(),
On 2016/05/24 23:43:32, jianli wrote:
> "clock_->Now()" passed here will be different from the one used in
> GetExpiredPageIds. This might cause subtle issues in the future.

Done.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:71:
base::Bind(&OfflinePageStorageManager::OnExpiredPagesDeleted,
On 2016/05/24 23:43:32, jianli wrote:
> I think OnExpiredPagesDeleted should be OnPagesExpired since expired pages are
> still kept in the store.

Done.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:73:
static_cast<int>(page_ids_to_expire.size()),
On 2016/05/24 23:43:32, jianli wrote:
> Why doing all these casts, instead of simply using size_t?

Done.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:84:
base::Bind(&OfflinePageStorageManager::OnExpiredPagesCleared,
On 2016/05/24 23:43:32, jianli wrote:
> OnExpiredPagesCleared => OnOlderExpiredPagesDeleted

I'd prefer to assign names to pages in different lifetime.
Like offline pages becomes 'expired offline pages' when it was expired(archive
deleted and metadata marked as expired), and an 'expired offline page' would
become a 'dead offline page' after 21 days passes since last access. WDYT?

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:102: void
OfflinePageStorageManager::GetExpiredPageIds(
On 2016/05/24 23:43:32, jianli wrote:
> Since now we return not just expired page ids, it would be better to rename
this
> method to something like GetPageIdsToCleanUp.

Done.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:105:
std::vector<int64_t>& page_ids_to_expire,
On 2016/05/24 23:43:32, jianli wrote:
> I don't think passing as reference is allowed in our C++ style guidelines.

Done.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:106:
std::vector<int64_t>& page_ids_to_remove) {
On 2016/05/24 23:43:32, jianli wrote:
> ditto

Done.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:120: } else if
(clock_->Now() - page.expiration_time >=
On 2016/05/24 23:43:32, jianli wrote:
> nit: cache clock_->Now() before for loop, to be more deterministic
> 
> Indeed you already cache it at line 107

Done.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_storage_manager.h (right):

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:27: // reducing the
usage below the 10% threshold.
On 2016/05/24 23:43:32, jianli wrote:
> 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;

Done.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:32: // been expired more
than 21 days it should be deleted from metadata store.
On 2016/05/24 23:43:32, jianli wrote:
> ditto

Done.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:63: const
DeletePageCallback& callback) = 0;
On 2016/05/24 23:43:32, jianli wrote:
> 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.

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.

https://codereview.chromium.org/2006923005/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:133: // Gets offline IDs
of all expired pages and return in |offline_ids|.
On 2016/05/24 23:43:32, jianli wrote:
> Please update the comment. Please also explain the difference between
> page_ids_to_expire and page_ids_to_remove.

Done.

https://codereview.chromium.org/2006923005/diff/20001/components/offline_page...
File components/offline_pages/offline_page_storage_manager.cc (right):

https://codereview.chromium.org/2006923005/diff/20001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:50: base::Time now =
clock_->Now();
this time would be used throughout the whole clear process(except setting
last_clear_time), it would be helpful to keep consistency. WDYT?

https://codereview.chromium.org/2006923005/diff/20001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:68:
std::vector<int64_t>* page_ids_to_expire = new std::vector<int64_t>();
Jian Please take a look about these 2 pointers.. I'm not sure if I'm leaking
memory here.

[jianli, 2016-05-26 21:59:32]

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
File components/offline_pages/archive_manager.h (right):

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/archive_manager.h:50: // exist, or |archive_path| is
empty.
Adding this behavior seems to be confusing.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.cc:528:
archive_manager_->DeleteMultipleArchives(paths_to_delete, callback);
You can just check if paths_to_delete is empty and invoke callback if needed.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
File components/offline_pages/offline_page_storage_manager.cc (right):

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:46: void
OfflinePageStorageManager::OnGetStorageStatsDone(
nit: OnGetStorageStatsDone => OnGetStorageStatsDoneForClearingPages

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:50: base::Time now =
clock_->Now();
nit: now => clear_time
Even simpler, consider adding a member variable clear_time_ to avoid passing it
to many functions.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:54: last_clear_time_ =
clock_->Now();
last_clear_time_ = clear_time_;
clear_time_ = base::Time();

Also, I am wondering if you can remove in_progress_ since you can now check it
with clear_time_.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:63: void
OfflinePageStorageManager::OnGetAllPagesDone(
nit: OnGetAllPagesDone => OnGetAllPagesDoneForClearingPages

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:68:
std::vector<int64_t>* page_ids_to_expire = new std::vector<int64_t>();
Why can't we simply define this as "std::vector<int64_t> page_ids_to_expire;"
and then pass the pointer to GetPageIdsToClear? Using base::Owned really makes
the whole logic and lifetime control too complicated.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:69:
std::vector<int64_t>* page_ids_to_remove = new std::vector<int64_t>();
ditto

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:82: bool success) {
Better to clearly indicate that |success| is for ExpirePages. Please rename it
to expire_succeeded.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:83:
client_->DeletePagesByOfflineId(
Please add a comment before this line to say that we want to delete the outdated
pages regardless whether the expiration succeeded or not.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:98: clear_result =
ClearStorageResult::EXPIRE_FAILURE;
It seems that we can have both expire and deletion failures. Do we want to
handle this?

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:111: const base::Time&
now,
nit: now => clear_time

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
File components/offline_pages/offline_page_storage_manager.h (right):

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:26: // before a storage
clearing.
nit: storage clearing => storage cleanup

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:27: const double
kOfflinePageStorageLimit = 0.3;
If these constants are only needed in .cc file, please move them over.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:30: // Storage manager
will clear storage if it has been longer than this interval
nit: The time that the storage cleanup will be triggered again since the last
cleanup.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:33: // Storage manager
will remove a page from metadata store once the page has been
nit: The time that the page record will be removed from the store since the page
has been expired.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:56: // Asks the client
to delete pages based on |ofline_ids| and invoke
nit: invoke => invokes
also consider adding "upon completion" after |callback|.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:65: // Asks the client
to mark pages with |offline_ids| as expired and the
nit: add "deletes" after "and".

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:128: void
OnDeadPagesCleared(const ClearPagesCallback& callback,
replace Dead with Outdated

[romax, 2016-05-27 00:41:38]

more comments addressed.
Removed logic added in previous patches.
using times instead of passing arguments and in_progress_.
nits and tests.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
File components/offline_pages/archive_manager.h (right):

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/archive_manager.h:50: // exist, or |archive_path| is
empty.
On 2016/05/26 21:59:31, jianli wrote:
> Adding this behavior seems to be confusing.

removed

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.cc:528:
archive_manager_->DeleteMultipleArchives(paths_to_delete, callback);
On 2016/05/26 21:59:31, jianli wrote:
> You can just check if paths_to_delete is empty and invoke callback if needed.

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
File components/offline_pages/offline_page_storage_manager.cc (right):

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:46: void
OfflinePageStorageManager::OnGetStorageStatsDone(
On 2016/05/26 21:59:31, jianli wrote:
> nit: OnGetStorageStatsDone => OnGetStorageStatsDoneForClearingPages

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:50: base::Time now =
clock_->Now();
On 2016/05/26 21:59:31, jianli wrote:
> nit: now => clear_time
> Even simpler, consider adding a member variable clear_time_ to avoid passing
it
> to many functions.

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:54: last_clear_time_ =
clock_->Now();
On 2016/05/26 21:59:31, jianli wrote:
> last_clear_time_ = clear_time_;
> clear_time_ = base::Time();
> 
> Also, I am wondering if you can remove in_progress_ since you can now check it
> with clear_time_. 

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:63: void
OfflinePageStorageManager::OnGetAllPagesDone(
On 2016/05/26 21:59:31, jianli wrote:
> nit: OnGetAllPagesDone => OnGetAllPagesDoneForClearingPages

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:68:
std::vector<int64_t>* page_ids_to_expire = new std::vector<int64_t>();
On 2016/05/26 21:59:31, jianli wrote:
> Why can't we simply define this as "std::vector<int64_t> page_ids_to_expire;"
> and then pass the pointer to GetPageIdsToClear? Using base::Owned really makes
> the whole logic and lifetime control too complicated.

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:69:
std::vector<int64_t>* page_ids_to_remove = new std::vector<int64_t>();
On 2016/05/26 21:59:31, jianli wrote:
> ditto

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:82: bool success) {
On 2016/05/26 21:59:31, jianli wrote:
> Better to clearly indicate that |success| is for ExpirePages. Please rename it
> to expire_succeeded.

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:83:
client_->DeletePagesByOfflineId(
On 2016/05/26 21:59:31, jianli wrote:
> Please add a comment before this line to say that we want to delete the
outdated
> pages regardless whether the expiration succeeded or not.

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:98: clear_result =
ClearStorageResult::EXPIRE_FAILURE;
On 2016/05/26 21:59:31, jianli wrote:
> It seems that we can have both expire and deletion failures. Do we want to
> handle this?

Since it will not be called outside of OfflinePageModel I don't know if we
should handle this (and I don't have an idea about handling this either). But
I've added new ENUM representing this case.
The only case this result would be useful is logging UMA...

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:111: const base::Time&
now,
On 2016/05/26 21:59:31, jianli wrote:
> nit: now => clear_time

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
File components/offline_pages/offline_page_storage_manager.h (right):

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:26: // before a storage
clearing.
On 2016/05/26 21:59:31, jianli wrote:
> nit: storage clearing => storage cleanup

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:27: const double
kOfflinePageStorageLimit = 0.3;
On 2016/05/26 21:59:32, jianli wrote:
> If these constants are only needed in .cc file, please move them over.

per discussion I'm leaving them here.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:30: // Storage manager
will clear storage if it has been longer than this interval
On 2016/05/26 21:59:31, jianli wrote:
> nit: The time that the storage cleanup will be triggered again since the last
> cleanup.

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:33: // Storage manager
will remove a page from metadata store once the page has been
On 2016/05/26 21:59:32, jianli wrote:
> nit: The time that the page record will be removed from the store since the
page
> has been expired.

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:56: // Asks the client
to delete pages based on |ofline_ids| and invoke
On 2016/05/26 21:59:32, jianli wrote:
> nit: invoke => invokes
> also consider adding "upon completion" after |callback|.

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:65: // Asks the client
to mark pages with |offline_ids| as expired and the
On 2016/05/26 21:59:32, jianli wrote:
> nit: add "deletes" after "and".

Done.

https://codereview.chromium.org/2006923005/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:128: void
OnDeadPagesCleared(const ClearPagesCallback& callback,
On 2016/05/26 21:59:32, jianli wrote:
> replace Dead with Outdated

Done.

[jianli, 2016-05-27 00:57:17]

lgtm

https://codereview.chromium.org/2006923005/diff/100001/components/offline_pag...
File components/offline_pages/offline_page_storage_manager.h (right):

https://codereview.chromium.org/2006923005/diff/100001/components/offline_pag...
components/offline_pages/offline_page_storage_manager.h:60: // Asks the client
to get all offline pages and invoke |callback|.
nit: invokes |callback| upon completion.

https://codereview.chromium.org/2006923005/diff/100001/components/offline_pag...
components/offline_pages/offline_page_storage_manager.h:76: BOTH_FAILURE,    //
Both expiration and deletion failed.
Probably better to say EXPIRE_AND_DELETE_FAILURES in case that we might add
other failure case in the future.

https://codereview.chromium.org/2006923005/diff/100001/components/offline_pag...
components/offline_pages/offline_page_storage_manager.h:116: // Callback called
after getting all pages from client done.
nit: remove "done" from comment

https://codereview.chromium.org/2006923005/diff/100001/components/offline_pag...
components/offline_pages/offline_page_storage_manager.h:147: bool
ShouldBeExpired(const OfflinePageItem& page);
add const modifier

https://codereview.chromium.org/2006923005/diff/100001/components/offline_pag...
components/offline_pages/offline_page_storage_manager.h:150: bool
IsInProgress();
nit: add const modifier

[romax, 2016-05-27 02:18:27]

more comments and commit ready if all trybots are green.

https://codereview.chromium.org/2006923005/diff/100001/components/offline_pag...
File components/offline_pages/offline_page_storage_manager.h (right):

https://codereview.chromium.org/2006923005/diff/100001/components/offline_pag...
components/offline_pages/offline_page_storage_manager.h:60: // Asks the client
to get all offline pages and invoke |callback|.
On 2016/05/27 00:57:17, jianli wrote:
> nit: invokes |callback| upon completion.

Done.

https://codereview.chromium.org/2006923005/diff/100001/components/offline_pag...
components/offline_pages/offline_page_storage_manager.h:76: BOTH_FAILURE,    //
Both expiration and deletion failed.
On 2016/05/27 00:57:17, jianli wrote:
> Probably better to say EXPIRE_AND_DELETE_FAILURES in case that we might add
> other failure case in the future.

Done.

https://codereview.chromium.org/2006923005/diff/100001/components/offline_pag...
components/offline_pages/offline_page_storage_manager.h:116: // Callback called
after getting all pages from client done.
On 2016/05/27 00:57:17, jianli wrote:
> nit: remove "done" from comment

Done.

https://codereview.chromium.org/2006923005/diff/100001/components/offline_pag...
components/offline_pages/offline_page_storage_manager.h:147: bool
ShouldBeExpired(const OfflinePageItem& page);
On 2016/05/27 00:57:17, jianli wrote:
> add const modifier

Done.

https://codereview.chromium.org/2006923005/diff/100001/components/offline_pag...
components/offline_pages/offline_page_storage_manager.h:150: bool
IsInProgress();
On 2016/05/27 00:57:17, jianli wrote:
> nit: add const modifier

Done.

[romax, 2016-05-27 18:09:11]

The CQ bit was checked by romax@chromium.org

[romax, 2016-05-27 18:09:11]

The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/2006923005/#ps120001
(title: "more comments, commit ready.")

[commit-bot: I haz the power, 2016-05-27 18:09:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2006923005/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2006923005/120001

[commit-bot: I haz the power, 2016-05-27 18:13:50]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-27 18:15:26]

Description was changed from

==========
[Offline Pages] Two-step expiration in storage manager.

This patch expanded the two-step expiration from offline page model to
storage manager. Now the storage manager would expire a page and delete
the archived file when it wasn't used long enough and after a certain
time interval remove the page entry from metadata store.
Also added tests for corresponding cases.

BUG=605615
==========

to

==========
[Offline Pages] Two-step expiration in storage manager.

This patch expanded the two-step expiration from offline page model to
storage manager. Now the storage manager would expire a page and delete
the archived file when it wasn't used long enough and after a certain
time interval remove the page entry from metadata store.
Also added tests for corresponding cases.

BUG=605615

Committed: https://crrev.com/2b43a3e24f906a24f3a85c94892e3824cce521e7
Cr-Commit-Position: refs/heads/master@{#396513}
==========

[commit-bot: I haz the power, 2016-05-27 18:15:27]

Patchset 7 (id:??) landed as
https://crrev.com/2b43a3e24f906a24f3a85c94892e3824cce521e7
Cr-Commit-Position: refs/heads/master@{#396513}