[Offline Pages] Linking storage manager and model with UMAs.

[Offline Pages] Linking storage manager and model with UMAs.

Hooking up storage manager and model, also adding UMAs for expiring pages/clearing storage.

BUG=614895
Committed: https://crrev.com/871851cd7676eabcb85148dc357019d1b365f3f3
Cr-Commit-Position: refs/heads/master@{#396605}

[romax, 2016-05-26 01:37:29]

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

[romax, 2016-05-26 01:37:30]

Hi Jian, this is just kind of a draft... I'm sure you'll have questions for me.
Please take a look and I'm open to talk anytime about this.

[jianli, 2016-05-26 20:37:52]

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

https://codereview.chromium.org/2015793002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:534: // TODO(romax): Report UMA
about successful expiration.
Do you also want to address this comment by adding a UMA?

https://codereview.chromium.org/2015793002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:537: if (iter !=
offline_pages_.end()) {
nit: simpler to do early return for non-match case to avoid extra nesting.

https://codereview.chromium.org/2015793002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:544:
base::TimeDelta::FromDays(60).InMinutes(), 10);
Are 10 buckets enough?

https://codereview.chromium.org/2015793002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2015793002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:34680: +<histogram
name="OfflinePages.ExpirePages.PageLifetime" units="minutes">
You need to add these 2 new histograms to OfflinePagesNamespace for suffix
usages.

[romax, 2016-05-27 02:01:38]

Hi Jian,
I've merged two patches since they're relatively close and small. I've added
some UMAs as well. Please share your thoughts before I add the owner of
histograms.xml.

Thanks,
Yafei

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

https://codereview.chromium.org/2015793002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:534: // TODO(romax): Report UMA
about successful expiration.
On 2016/05/26 20:37:52, jianli wrote:
> Do you also want to address this comment by adding a UMA?

yes, but it would come with the model-storage manager hookup, I think I'll just
do it in this CL.

https://codereview.chromium.org/2015793002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:537: if (iter !=
offline_pages_.end()) {
On 2016/05/26 20:37:52, jianli wrote:
> nit: simpler to do early return for non-match case to avoid extra nesting.

Done.

https://codereview.chromium.org/2015793002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.cc:544:
base::TimeDelta::FromDays(60).InMinutes(), 10);
On 2016/05/26 20:37:52, jianli wrote:
> Are 10 buckets enough?

I misunderstood this argument, changing to 120.

https://codereview.chromium.org/2015793002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2015793002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:34680: +<histogram
name="OfflinePages.ExpirePages.PageLifetime" units="minutes">
On 2016/05/26 20:37:52, jianli wrote:
> You need to add these 2 new histograms to OfflinePagesNamespace for suffix
> usages.

Done.

[jianli, 2016-05-27 19:16:13]

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

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:542:
UMA_HISTOGRAM_COUNTS("OfflinePages.ExpirePage.StoreUpdateResult",
You should use UMA_HISTOGRAM_BOOLEAN.

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:546: auto iter =
offline_pages_.find(offline_id);
nit: add const&

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:554:
base::TimeDelta::FromDays(30).InMinutes(), 120);
I think 50 buckets should be enough.

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:560:
base::TimeDelta::FromDays(30).InMinutes(), 120);
ditto

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:684:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
It just seems to be more natural to put this at the end of function unless the
ordering does matter.

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:689:
base::TimeDelta::FromSeconds(20));
Better define this as constant.

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:693:
CheckForExternalFileDeletion();
Do we have the plan to move this to StorageManager?

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:893: void
OfflinePageModel::OnStorageCleared(const size_t expired_page_count,
nit: add const before size_t is not needed

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:343: // Calls storage manager to
clear expired pages if there's any. Used to
nit: you don't need to explain the implementation details in the function
comment. Probably just enough to say:
  // Clears expired pages if there are any.

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:349: // Callback completing
storage clearing/
nit: / => .

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

https://codereview.chromium.org/2015793002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:34692: +<histogram
name="OfflinePages.ExpirePage.Result"
Suggest to name this as OfflinePages.ClearStorageResult since StorageManager
might not be just used for expiring pages.

[romax, 2016-05-27 20:36:02]

Addressed comments.

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

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:542:
UMA_HISTOGRAM_COUNTS("OfflinePages.ExpirePage.StoreUpdateResult",
On 2016/05/27 19:16:12, jianli wrote:
> You should use UMA_HISTOGRAM_BOOLEAN.

Done.

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:546: auto iter =
offline_pages_.find(offline_id);
On 2016/05/27 19:16:12, jianli wrote:
> nit: add const&

Done.

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:554:
base::TimeDelta::FromDays(30).InMinutes(), 120);
On 2016/05/27 19:16:12, jianli wrote:
> I think 50 buckets should be enough.

I don't have a strong opinion here, but can you elaborate how to estimate the
number of buckets?

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:560:
base::TimeDelta::FromDays(30).InMinutes(), 120);
On 2016/05/27 19:16:12, jianli wrote:
> ditto

Done.

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:684:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2016/05/27 19:16:12, jianli wrote:
> It just seems to be more natural to put this at the end of function unless the
> ordering does matter.

Done.

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:689:
base::TimeDelta::FromSeconds(20));
On 2016/05/27 19:16:12, jianli wrote:
> Better define this as constant.

Done.

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:693:
CheckForExternalFileDeletion();
On 2016/05/27 19:16:13, jianli wrote:
> Do we have the plan to move this to StorageManager?

I think that would be the plan... I'll do that in a future patch.

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:893: void
OfflinePageModel::OnStorageCleared(const size_t expired_page_count,
On 2016/05/27 19:16:12, jianli wrote:
> nit: add const before size_t is not needed

Done.

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:343: // Calls storage manager to
clear expired pages if there's any. Used to
On 2016/05/27 19:16:13, jianli wrote:
> nit: you don't need to explain the implementation details in the function
> comment. Probably just enough to say:
>   // Clears expired pages if there are any.

Done.

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:349: // Callback completing
storage clearing/
On 2016/05/27 19:16:13, jianli wrote:
> nit: / => .

Done.

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

https://codereview.chromium.org/2015793002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:34692: +<histogram
name="OfflinePages.ExpirePage.Result"
On 2016/05/27 19:16:13, jianli wrote:
> Suggest to name this as OfflinePages.ClearStorageResult since StorageManager
> might not be just used for expiring pages.

Done.

[jianli, 2016-05-27 20:46:21]

lgtm

Please also update the issue body.

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

https://codereview.chromium.org/2015793002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:554:
base::TimeDelta::FromDays(30).InMinutes(), 120);
On 2016/05/27 20:36:01, romax wrote:
> On 2016/05/27 19:16:12, jianli wrote:
> > I think 50 buckets should be enough.
> 
> I don't have a strong opinion here, but can you elaborate how to estimate the
> number of buckets?

We should only try to use as many buckets as needed, in order not to incur
unnecessary storage cost in the server. Probably our UMA reviewer could better
explain this.

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

https://codereview.chromium.org/2015793002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.cc:36: const base::TimeDelta
kStorageManagerStartingDelay =
nit: Add an empty line before this line
Also please add a comment

[romax, 2016-05-27 21:05:04]

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

[romax, 2016-05-27 21:05:05]

Adding holte@ for histogram changes. PTAL. Thanks!

[Steven Holte, 2016-05-27 21:10:48]

lgtm

[romax, 2016-05-27 21:19:25]

Description was changed from

==========
[Offline Pages] Adding UMAs for expiring page.

Adding UMAs for expiring pages.

BUG=614895
==========

to

==========
[Offline Pages] Linking storage manager and model with UMAs.

Hooking up storage manager and model, also adding UMAs for expiring
pages/clearing storage.

BUG=614895
==========

[romax, 2016-05-27 22:45:37]

The CQ bit was checked by romax@chromium.org

[romax, 2016-05-27 22:45:38]

The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org,
holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2015793002/#ps80001
(title: "fixing typo in histograms.xml")

[commit-bot: I haz the power, 2016-05-27 22:46:13]

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

[commit-bot: I haz the power, 2016-05-28 00:07:11]

Description was changed from

==========
[Offline Pages] Linking storage manager and model with UMAs.

Hooking up storage manager and model, also adding UMAs for expiring
pages/clearing storage.

BUG=614895
==========

to

==========
[Offline Pages] Linking storage manager and model with UMAs.

Hooking up storage manager and model, also adding UMAs for expiring
pages/clearing storage.

BUG=614895
==========

[commit-bot: I haz the power, 2016-05-28 00:07:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-28 00:09:02]

Description was changed from

==========
[Offline Pages] Linking storage manager and model with UMAs.

Hooking up storage manager and model, also adding UMAs for expiring
pages/clearing storage.

BUG=614895
==========

to

==========
[Offline Pages] Linking storage manager and model with UMAs.

Hooking up storage manager and model, also adding UMAs for expiring
pages/clearing storage.

BUG=614895

Committed: https://crrev.com/871851cd7676eabcb85148dc357019d1b365f3f3
Cr-Commit-Position: refs/heads/master@{#396605}
==========

[commit-bot: I haz the power, 2016-05-28 00:09:03]

Patchset 5 (id:??) landed as
https://crrev.com/871851cd7676eabcb85148dc357019d1b365f3f3
Cr-Commit-Position: refs/heads/master@{#396605}