[Offline pages] Moving disk size related calls to Archive Manager, reorganizing UMA

[Offline pages] Moving disk size related calls to Archive Manager, reorganizing UMA

* Obsoletes 3 offline page UMAs
* Moves UMA reporting and triggers for saving/deleting pages to model
* Adds capability to get free space and total page size to ArchiveManager

BUG=610870
Committed: https://crrev.com/1dd3fa4cec92adb64af5e4a484bcd27074d60b4a
Cr-Commit-Position: refs/heads/master@{#394566}

[fgorski, 2016-05-17 23:20:08]

fgorski@chromium.org changed reviewers:
+ jianli@chromium.org, rkaplow@chromium.org

[fgorski, 2016-05-17 23:20:09]

PTAL

[jianli, 2016-05-18 00:06:00]

Please update summary for affected hisrograms in xml file. For example, we
should say "after the page is deleted", instead of "when the page is being
deleted".

Note that UMA reviewer: we're going to relaunch offline pages in a different
study group. So changing the UMA semantics a bit works fine for us because we
will get the data only from the new study group .

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

https://codereview.chromium.org/1988973002/diff/20001/components/offline_page...
components/offline_pages/archive_manager.h:26: // Structure for passing around
information related to disk space consumed by
nit: no need to comment since struct name should be clear

https://codereview.chromium.org/1988973002/diff/20001/components/offline_page...
components/offline_pages/archive_manager.h:28: struct StorageSizes {
nit: StorageStats

https://codereview.chromium.org/1988973002/diff/20001/components/offline_page...
components/offline_pages/archive_manager.h:63: // space available, for UMA
reporting.
nit: remove UMA reporting in case we might use this info for other purpose

// Gets stats about archive storage, i.e., total archive sizes and free disk
space.

https://codereview.chromium.org/1988973002/diff/20001/components/offline_page...
components/offline_pages/archive_manager.h:64: virtual void GetStorageSizes(
nit: GetStorageStats

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

https://codereview.chromium.org/1988973002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:82: void
ReportStorageUMAAfterSave(
ReportStorageHistogramsAfterSave

https://codereview.chromium.org/1988973002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:83: const
ArchiveManager::StorageSizes storage_sizes) {
nit: add &

https://codereview.chromium.org/1988973002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:84: int mb = 1024 * 1024;
nit: const int or inline it

[rkaplow, 2016-05-18 03:47:02]

lgtm

Acknowledge the comment about it being ok to change the meaning of the metrics
because you are changing the experiment - however in general I still prefer
renaming the metrics. If someone is looking at this metric that isn't
study-split, the data will change before and after this and it will be unclear
why.

If you are sure no one will be confused by this then it is ok, but I might
suggest updating the description to note the change at least.

[fgorski, 2016-05-18 18:15:00]

Addressed. PTAL

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

https://codereview.chromium.org/1988973002/diff/20001/components/offline_page...
components/offline_pages/archive_manager.h:26: // Structure for passing around
information related to disk space consumed by
On 2016/05/18 00:06:00, jianli wrote:
> nit: no need to comment since struct name should be clear

Done.

https://codereview.chromium.org/1988973002/diff/20001/components/offline_page...
components/offline_pages/archive_manager.h:28: struct StorageSizes {
On 2016/05/18 00:05:59, jianli wrote:
> nit: StorageStats

Done.

https://codereview.chromium.org/1988973002/diff/20001/components/offline_page...
components/offline_pages/archive_manager.h:63: // space available, for UMA
reporting.
On 2016/05/18 00:06:00, jianli wrote:
> nit: remove UMA reporting in case we might use this info for other purpose
> 
> // Gets stats about archive storage, i.e., total archive sizes and free disk
> space.

Done.

https://codereview.chromium.org/1988973002/diff/20001/components/offline_page...
components/offline_pages/archive_manager.h:64: virtual void GetStorageSizes(
On 2016/05/18 00:05:59, jianli wrote:
> nit: GetStorageStats

Done.

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

https://codereview.chromium.org/1988973002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:82: void
ReportStorageUMAAfterSave(
On 2016/05/18 00:06:00, jianli wrote:
> ReportStorageHistogramsAfterSave

Done.

https://codereview.chromium.org/1988973002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:83: const
ArchiveManager::StorageSizes storage_sizes) {
On 2016/05/18 00:06:00, jianli wrote:
> nit: add &

Done.

https://codereview.chromium.org/1988973002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:84: int mb = 1024 * 1024;
On 2016/05/18 00:06:00, jianli wrote:
> nit: const int or inline it

Done.

[jianli, 2016-05-18 21:50:52]

lgtm

[fgorski, 2016-05-18 21:53:53]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-05-18 21:53:53]

The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/1988973002/#ps60001
(title: "Addressing feedback")

[commit-bot: I haz the power, 2016-05-18 21:54:39]

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

[commit-bot: I haz the power, 2016-05-18 22:01:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-18 22:02:47]

Description was changed from

==========
[Offline pages] Moving disk size related calls to Archive Manager, reorganizing
UMA

* Obsoletes 3 offline page UMAs
* Moves UMA reporting and triggers for saving/deleting pages to model
* Adds capability to get free space and total page size to ArchiveManager

BUG=610870
==========

to

==========
[Offline pages] Moving disk size related calls to Archive Manager, reorganizing
UMA

* Obsoletes 3 offline page UMAs
* Moves UMA reporting and triggers for saving/deleting pages to model
* Adds capability to get free space and total page size to ArchiveManager

BUG=610870

Committed: https://crrev.com/1dd3fa4cec92adb64af5e4a484bcd27074d60b4a
Cr-Commit-Position: refs/heads/master@{#394566}
==========

[commit-bot: I haz the power, 2016-05-18 22:02:48]

Patchset 4 (id:??) landed as
https://crrev.com/1dd3fa4cec92adb64af5e4a484bcd27074d60b4a
Cr-Commit-Position: refs/heads/master@{#394566}