[Offline Pages] Removes two-step expiration related.

[Offline Pages] Removes two-step expiration related.

Removed the two-step expiration related code from offline pages. Also
fixed the tests related. Now the pages which live longer than the time
specified in policy would be deleted from both metadata store and disk.

BUG=661721
Committed: https://crrev.com/cb27e63bd49fc1069fa4bbcf06ab6b50fd6d71ac
Cr-Commit-Position: refs/heads/master@{#435381}

[romax, 2016-11-17 23:46:43]

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

[romax, 2016-11-17 23:46:43]

I've made it a new Cl in order to avoid some errors in diff after merging with
your change. PTAL!
Thanks!

[romax, 2016-11-17 23:47:10]

The CQ bit was checked by romax@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 23:49:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 23:56:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 23:56:31]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[fgorski, 2016-11-18 00:13:03]

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

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

Initial pass through

https://codereview.chromium.org/2512073002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_bookmark_observer.h
(right):

https://codereview.chromium.org/2512073002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_bookmark_observer.h:41: //
Does work for actual expiring.
fix comments to, please

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_item.h (left):

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_item.h:62: // The URL of the page. This is
the last committed URL. In the case that
don't remove valid comments.

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_metadata_store_impl_unittest.cc
(left):

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:381:
offline_page.original_url = GURL(kOriginalTestURL);
be careful about what you are removing. original_url should stay

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:224:
ASSERT_FALSE(connection.DoesColumnExist(OFFLINE_PAGES_TABLE_V1, "version"));
don't touch M55 code, Jian has appropriate fix in one of his patches.

I believe your change will land in M56, please create a method for that

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:695:
OfflinePageItem offline_page_2(GURL("https://other.page.com"), 5678LL,
given line 710, does this compile?

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_metadata_store_sql.cc (left):

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_sql.cc:79: "access_count,
expiration_time, client_namespace, client_id, "
this will revive expired pages.

Shouldn't we delete them as part of the upgrade?

Leaving it to metadata consistency check is an alternative strategy.

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_sql.cc:93: "access_count,
expiration_time, client_namespace, client_id, "
ditto

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_sql.cc:142: }
your expiration_time check should go here...

it means that all of the above where checked and we must be in M56 or later.

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_sql.h:37: // * In M56 we
dropped expiration_time and added original_url.
I think you should consider your patch for M57 no M56.

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:50: // NOTE: always keep
this entry at the end. Add new result types only
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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:100: void
OnExpiredPagesCleared(const ClearStorageCallback& callback,
Expired... part should probably go.

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:114: bool
IsExpired(const OfflinePageItem& page) const;
Explain why we are still using the concept of expired? Is that for cached pages?

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

https://codereview.chromium.org/2512073002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
(right):

https://codereview.chromium.org/2512073002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:169:
RunUntilIdle();
This line can also be removed since SavePage waits.

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_sql.cc:129: if
(!db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "expiration_time") &&
M56 table doesn't have expiration_time and title. How do we handle this?

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.h:224: // Clears expired pages
if there are any.
Update comment.

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:179: bool
OfflinePageStorageManager::IsExpired(const OfflinePageItem& page) const {
Do we really need this?

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:183: clear_time_ -
page.last_access_time >= policy.expiration_period;
Do you also want to update the policy to remove this field?

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:46: enum class
ClearStorageResult {
Do you want to update the histogram for this?

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

Addressed comments, PTAL, thanks!
Also I think I somehow messed up the diff between #1 and #2, there are lots of
changes that wasn't added in my patch. Sorry for the inconvenience!

https://codereview.chromium.org/2512073002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_bookmark_observer.h
(right):

https://codereview.chromium.org/2512073002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_bookmark_observer.h:41: //
Does work for actual expiring.
On 2016/11/18 00:13:04, fgorski wrote:
> fix comments to, please

Done.

https://codereview.chromium.org/2512073002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
(right):

https://codereview.chromium.org/2512073002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:169:
RunUntilIdle();
On 2016/11/18 00:15:48, jianli wrote:
> This line can also be removed since SavePage waits.

Done.

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_item.h (left):

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_item.h:62: // The URL of the page. This is
the last committed URL. In the case that
On 2016/11/18 00:13:04, fgorski wrote:
> don't remove valid comments.

Done.

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_metadata_store_impl_unittest.cc
(left):

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:381:
offline_page.original_url = GURL(kOriginalTestURL);
On 2016/11/18 00:13:04, fgorski wrote:
> be careful about what you are removing. original_url should stay

Done.

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:224:
ASSERT_FALSE(connection.DoesColumnExist(OFFLINE_PAGES_TABLE_V1, "version"));
On 2016/11/18 00:13:04, fgorski wrote:
> don't touch M55 code, Jian has appropriate fix in one of his patches.
> 
> I believe your change will land in M56, please create a method for that

Done.

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:695:
OfflinePageItem offline_page_2(GURL("https://other.page.com"), 5678LL,
On 2016/11/18 00:13:04, fgorski wrote:
> given line 710, does this compile?

Done.

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_metadata_store_sql.cc (left):

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_sql.cc:79: "access_count,
expiration_time, client_namespace, client_id, "
On 2016/11/18 00:13:04, fgorski wrote:
> this will revive expired pages.
> 
> Shouldn't we delete them as part of the upgrade?
> 
> Leaving it to metadata consistency check is an alternative strategy.

I lean towards letting consistency check do the work. But let me know if you
have other concerns. The comment is added in offline_page_metadata_store_sql.h.
Let me know if we need a more visible place to have the statement.

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_sql.cc:93: "access_count,
expiration_time, client_namespace, client_id, "
On 2016/11/18 00:13:04, fgorski wrote:
> ditto

Done.

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_sql.cc:129: if
(!db->DoesColumnExist(OFFLINE_PAGES_TABLE_NAME, "expiration_time") &&
On 2016/11/18 00:15:48, jianli wrote:
> M56 table doesn't have expiration_time and title. How do we handle this?

M56 should have title. It was added in M54

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_sql.cc:142: }
On 2016/11/18 00:13:04, fgorski wrote:
> your expiration_time check should go here...
> 
> it means that all of the above where checked and we must be in M56 or later.

Done.

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_metadata_store_sql.h:37: // * In M56 we
dropped expiration_time and added original_url.
On 2016/11/18 00:13:04, fgorski wrote:
> I think you should consider your patch for M57 no M56.

Done.

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.h:224: // Clears expired pages
if there are any.
On 2016/11/18 00:15:48, jianli wrote:
> Update comment.

we still have pages which have been living longer than the time defined in
expiration_policy. These pages would be called expired pages.

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:179: bool
OfflinePageStorageManager::IsExpired(const OfflinePageItem& page) const {
On 2016/11/18 00:15:48, jianli wrote:
> Do we really need this?

Yes it's still a helper function to return if the page is expired. WRT 'expire'
I have a comment above.

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.cc:183: clear_time_ -
page.last_access_time >= policy.expiration_period;
On 2016/11/18 00:15:48, jianli wrote:
> Do you also want to update the policy to remove this field?

No, there should still be a expiration_period for a given page.

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:46: enum class
ClearStorageResult {
On 2016/11/18 00:15:48, jianli wrote:
> Do you want to update the histogram for this?

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.

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:50: // NOTE: always keep
this entry at the end. Add new result types only
On 2016/11/18 00:13:04, fgorski wrote:
> 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

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

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:100: void
OnExpiredPagesCleared(const ClearStorageCallback& callback,
On 2016/11/18 00:13:04, fgorski wrote:
> Expired... part should probably go.

Acknowledged.

https://codereview.chromium.org/2512073002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_storage_manager.h:114: bool
IsExpired(const OfflinePageItem& page) const;
On 2016/11/18 00:13:04, fgorski wrote:
> Explain why we are still using the concept of expired? Is that for cached
pages?

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.

[romax, 2016-11-18 20:54:31]

Patchset #2 (id:20001) has been deleted

[romax, 2016-11-18 21:04:40]

I've followed Filip's suggestion to delete the patch which has unnecessary
changes and now please take a look of the diff between #2 and #3.
Thanks!

[fgorski, 2016-11-22 00:03:00]

lgtm with coments (mostly nits).

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

https://codereview.chromium.org/2512073002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:269:
ASSERT_TRUE(connection.DoesColumnExist(OFFLINE_PAGES_TABLE_V1, "title"));
just the expiration time is what you care about in here. (you should be focused
on the delta.

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

https://codereview.chromium.org/2512073002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.h:39: //   removed when
metadata consistency check happens.
Good! Probably worth commenting about that on the other end
(MetadataConsistencyCheck) so that we don't remove that method and lose the
functionality, without realizing it.

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

https://codereview.chromium.org/2512073002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:562: if
(id_page_pair.second.client_id == client_id) {
nit: since both body and condition are one liners, you don't need {}

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

https://codereview.chromium.org/2512073002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model_impl.h:196: // Callbacks called
after orphaned archives deleted. Orphaned archives are
nit: archives *are* deleted. since you are fixing it, pleas turn it into a
sentence.

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

https://codereview.chromium.org/2512073002/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:105: if
(!IsExpired(page)) {
nit: remove {} after if and else.

https://codereview.chromium.org/2512073002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2512073002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:94744: +  <int value="2" label="Page
expiration failed (deprecated Nov.18 2016)"/>
nit: just 11/2016 is probably enough.

[romax, 2016-11-23 23:32:01]

romax@chromium.org changed reviewers:
+ dpapad@chromium.org, haraken@chromium.org

[romax, 2016-11-23 23:32:03]

Addressed comments, also adding owner reviewers:

haraken@chromium.org: Please review changes for histograms

dpapad@chromium.org: Please review changes in webui

Thanks!

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

https://codereview.chromium.org/2512073002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:269:
ASSERT_TRUE(connection.DoesColumnExist(OFFLINE_PAGES_TABLE_V1, "title"));
On 2016/11/22 00:02:59, fgorski wrote:
> just the expiration time is what you care about in here. (you should be
focused
> on the delta.

Done.

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

https://codereview.chromium.org/2512073002/diff/60001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.h:39: //   removed when
metadata consistency check happens.
On 2016/11/22 00:02:59, fgorski wrote:
> Good! Probably worth commenting about that on the other end
> (MetadataConsistencyCheck) so that we don't remove that method and lose the
> functionality, without realizing it.

Done.

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

https://codereview.chromium.org/2512073002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:562: if
(id_page_pair.second.client_id == client_id) {
On 2016/11/22 00:02:59, fgorski wrote:
> nit: since both body and condition are one liners, you don't need {}

Done.

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

https://codereview.chromium.org/2512073002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model_impl.h:196: // Callbacks called
after orphaned archives deleted. Orphaned archives are
On 2016/11/22 00:03:00, fgorski wrote:
> nit: archives *are* deleted. since you are fixing it, pleas turn it into a
> sentence.

Done.

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

https://codereview.chromium.org/2512073002/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:105: if
(!IsExpired(page)) {
On 2016/11/22 00:03:00, fgorski wrote:
> nit: remove {} after if and else.

Done.

https://codereview.chromium.org/2512073002/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2512073002/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:94744: +  <int value="2" label="Page
expiration failed (deprecated Nov.18 2016)"/>
On 2016/11/22 00:03:00, fgorski wrote:
> nit: just 11/2016 is probably enough.

Done.

[haraken, 2016-11-24 01:40:51]

haraken@chromium.org changed reviewers:
+ isherman@chromium.org

[haraken, 2016-11-24 01:40:52]

> mailto:haraken@chromium.org: Please review changes for histograms

(I'm allowed to review UseCounter-only changes.)

+isherman

[dpapad, 2016-11-28 18:55:07]

LGTM for chrome/browser/ui/webui/

[Ilya Sherman, 2016-11-29 02:44:21]

https://codereview.chromium.org/2512073002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2512073002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:39665: +<histogram
name="OfflinePages.ClearStorageBatchSize">
nit: Please add a units attribute

https://codereview.chromium.org/2512073002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:39687: +    enum="BooleanSuccess">
Hmm, is BooleanSuccess still the right enum?

[Ilya Sherman, 2016-11-29 02:45:00]

histograms.xml lgtm with the below nits addressed

On 2016/11/29 02:44:21, Ilya Sherman wrote:
>
https://codereview.chromium.org/2512073002/diff/80001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/2512073002/diff/80001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:39665: +<histogram
> name="OfflinePages.ClearStorageBatchSize">
> nit: Please add a units attribute
> 
>
https://codereview.chromium.org/2512073002/diff/80001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:39687: +    enum="BooleanSuccess">
> Hmm, is BooleanSuccess still the right enum?

[romax, 2016-11-29 19:38:16]

Addressed Ilya's comments and going to merge with tip and start a dryrun.

https://codereview.chromium.org/2512073002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2512073002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:39665: +<histogram
name="OfflinePages.ClearStorageBatchSize">
On 2016/11/29 02:44:20, Ilya Sherman wrote:
> nit: Please add a units attribute

Acknowledged.
I think I got the name wrong it should be 'count' rather than 'size'. But for
consistency I'd like to keep it for now, WDYT?

https://codereview.chromium.org/2512073002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:39687: +    enum="BooleanSuccess">
On 2016/11/29 02:44:20, Ilya Sherman wrote:
> Hmm, is BooleanSuccess still the right enum?

Done.

[romax, 2016-11-29 20:39:12]

The CQ bit was checked by romax@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 20:39:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2016-11-29 21:06:29]

https://codereview.chromium.org/2512073002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2512073002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:39665: +<histogram
name="OfflinePages.ClearStorageBatchSize">
On 2016/11/29 19:38:16, romax wrote:
> On 2016/11/29 02:44:20, Ilya Sherman wrote:
> > nit: Please add a units attribute
> 
> Acknowledged.
> I think I got the name wrong it should be 'count' rather than 'size'. But for
> consistency I'd like to keep it for now, WDYT?

I think the name is okay either way.  I'd still suggest adding units="pages",
though.

[commit-bot: I haz the power, 2016-11-29 21:07:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 21:07:04]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[romax, 2016-11-29 21:31:29]

The CQ bit was checked by romax@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 21:32:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 23:36:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 23:36:01]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[romax, 2016-11-30 00:24:16]

Addressed all comments. I'll commit when dry run passes.
Thanks for the reviewing!

https://codereview.chromium.org/2512073002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2512073002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:39665: +<histogram
name="OfflinePages.ClearStorageBatchSize">
On 2016/11/29 21:06:28, Ilya Sherman wrote:
> On 2016/11/29 19:38:16, romax wrote:
> > On 2016/11/29 02:44:20, Ilya Sherman wrote:
> > > nit: Please add a units attribute
> > 
> > Acknowledged.
> > I think I got the name wrong it should be 'count' rather than 'size'. But
for
> > consistency I'd like to keep it for now, WDYT?
> 
> I think the name is okay either way.  I'd still suggest adding units="pages",
> though.

Done.
Sorry I think I just realized the point of adding units :(

[romax, 2016-11-30 00:56:06]

The CQ bit was checked by romax@chromium.org

[romax, 2016-11-30 00:56:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
fgorski@chromium.org, dpapad@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2512073002/#ps160001
(title: "adding unit in histograms.")

[commit-bot: I haz the power, 2016-11-30 00:57:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 02:59:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 02:59:09]

Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[romax, 2016-11-30 18:34:29]

The CQ bit was checked by romax@chromium.org

[commit-bot: I haz the power, 2016-11-30 18:35:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 19:46:01]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480530869255240,
"parent_rev": "dbf6e115ce0ca1c345df648ae05b181390005996", "commit_rev":
"5fde987f41a356662374ccbaf33a5fd3b3990ec1"}

[commit-bot: I haz the power, 2016-11-30 19:46:37]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-11-30 19:49:52]

Description was changed from

==========
[Offline Pages] Removes two-step expiration related.

Removed the two-step expiration related code from offline pages. Also
fixed the tests related. Now the pages which live longer than the time
specified in policy would be deleted from both metadata store and disk.

BUG=661721
==========

to

==========
[Offline Pages] Removes two-step expiration related.

Removed the two-step expiration related code from offline pages. Also
fixed the tests related. Now the pages which live longer than the time
specified in policy would be deleted from both metadata store and disk.

BUG=661721

Committed: https://crrev.com/cb27e63bd49fc1069fa4bbcf06ab6b50fd6d71ac
Cr-Commit-Position: refs/heads/master@{#435381}
==========

[commit-bot: I haz the power, 2016-11-30 19:49:54]

Patchset 8 (id:??) landed as
https://crrev.com/cb27e63bd49fc1069fa4bbcf06ab6b50fd6d71ac
Cr-Commit-Position: refs/heads/master@{#435381}