Add a counter to calculate the total size of site data.

Add a counter to calculate the total size of site data.

To compute the total size of site data in browsing data, this CL adds following classes.
  SiteDataSizeCollector: collects size information of site data from storage backends using BrowsingData*Helper.
  SiteDataCounter: works as a BrowsingDataCounter using the size collector above.

BUG=591958
Committed: https://crrev.com/cc2718d95cc02f86c1258d8ef497892e7b346a22
Cr-Commit-Position: refs/heads/master@{#403236}

[fukino, 2016-06-23 05:25:53]

Description was changed from

==========
Add a counter to calculate the total size of site data.

BUG=591958
==========

to

==========
Add a counter to calculate the total size of site data.

To compute the total size of site data in browsing data, this CL adds following
classes.
  SiteDataSizeCollector: collects size information of site data from storage
backends using BrowsingData*Helper. 
  SiteDataCounter: works as a BrowsingDataCounter using the size collector
above.

BUG=591958
==========

[fukino, 2016-06-23 05:27:27]

fukino@chromium.org changed reviewers:
+ msramek@chromium.org

[fukino, 2016-06-23 05:27:28]

Hi Martin,
I haven't added tests yet, but could you check the current approach?

Here are my thoughts about the approach:

- I was going to count files/directories directly, but I changed my mind to use
BrowsingData*Helper. It is because using helpers will make it easier to be
consistent with what users see in "Cookies and site data" dialog.

- As an alternative approach, I considerd building CookieTreeModel and
traversing the tree to count the size. But I didd not take the approach since
building the cookie tree is unnecessary work and calculating sizes using
BrowsingData*Helper looks simple enough. 

- We can't get size of Cookies, Channel IDs, and Flash LSOs, so I count the file
size for them. However, even when there is no cookies, there is a sqlite cookie
file with non-zero size. My ad-hoc approach for this problem is that counting
the file size only when at least one cookie is found, to return 0 bytes when no
cookies.

[msramek, 2016-06-23 21:30:35]

The approach looks good. I left some comments, and of course, I'll wait for the
tests :)

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/site_data_counter.cc (right):

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/site_data_counter.cc:23:
weak_ptr_factory_.GetWeakPtr()));
nit: alignment.

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/site_data_counter.h (right):

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/site_data_counter.h:25: void Count() override;
nit: // BrowsingDataCounter:

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/site_data_size_collector.cc (right):

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/site_data_size_collector.cc:24: void
SiteDataSizeCollector::Fetch(const FetchCallback& callback) {
Looking at BrowsingDataRemover::REMOVE_SITE_DATA, there are a few items that are
not represented in this class:

- REMOVE_SITE_USAGE_DATA -> that's the data stored in
CONTENT_SETTINGS_TYPE_APP_BANNER and CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT (see
WebsiteSettingsRegistry)

- REMOVE_WEB_APP_DATA -> see WebappDataStorage.java

- REMOVE_WEB_SQL_DATA -> that's actually hidden in the Quota model, so it's OK

- REMOVE_WEBRTC_IDENTITY -> that one is about to be removed in
https://codereview.chromium.org/2033353002/, so it's OK

In addition, when you write tests, can you add a test that lists the items from
REMOVE_SITE_DATA that are already included in this class, and compares them with
REMOVE_SITE_DATA? I'd like to assure that people keep this class in sync with
what is in BrowsingDataRemover.

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/site_data_size_collector.cc:156: void
SiteDataSizeCollector::OnQuotaModelInfoLoaded(
Quota model consists of these things:

https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.c...

Which means that you're counting some things, such as IndexedDB, twice.

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/site_data_size_collector.h (right):

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/site_data_size_collector.h:96: // Default storage
partition of the browser context.
nit: "...of this profile." (it's not called browser context at this layer)

[msramek, 2016-06-24 11:50:52]

Oh, and I totally forgot... this doesn't support time ranges.

As you can see in the CBD dialog, the user can pick one of several time period
options, and we need to show the data volume for that period. That means
supporting counting limited to a time period between
[SiteDataCounter::GetPeriodStart(), base::Time::Now()).

[fukino, 2016-06-27 11:54:59]

On 2016/06/24 11:50:52, msramek wrote:
> Oh, and I totally forgot... this doesn't support time ranges.
> 
> As you can see in the CBD dialog, the user can pick one of several time period
> options, and we need to show the data volume for that period. That means
> supporting counting limited to a time period between
> [SiteDataCounter::GetPeriodStart(), base::Time::Now()).

Thank you for the review, Martin!

Regarding time ranges, it looks it needs a certain amount of work, and I'm
afraid I won't be able to support it for M53 (this week)...

For M53, our new app, storage manager, needs only a "good estimate" about how
much space is used for site data, so I want to implement the minimum function
for M53 to meet the deadline. 
Should I move the implementation to count the size to somewhere inside inside
our app, or can I continue to work on this CL and add support for time ranges
later in M54?

[msramek, 2016-06-27 12:03:34]

On 2016/06/27 11:54:59, fukino wrote:
> On 2016/06/24 11:50:52, msramek wrote:
> > Oh, and I totally forgot... this doesn't support time ranges.
> > 
> > As you can see in the CBD dialog, the user can pick one of several time
period
> > options, and we need to show the data volume for that period. That means
> > supporting counting limited to a time period between
> > [SiteDataCounter::GetPeriodStart(), base::Time::Now()).
> 
> Thank you for the review, Martin!
> 
> Regarding time ranges, it looks it needs a certain amount of work, and I'm
> afraid I won't be able to support it for M53 (this week)...
> 
> For M53, our new app, storage manager, needs only a "good estimate" about how
> much space is used for site data, so I want to implement the minimum function
> for M53 to meet the deadline. 
> Should I move the implementation to count the size to somewhere inside inside
> our app, or can I continue to work on this CL and add support for time ranges
> later in M54?

I think you can leave SiteDataSizeCollector as it is, and use it for
the storage manager in M53. It can definitely stay in this codebase.

Then for M54, you can implement SiteDataCounter to be not only a wrapper around
SiteDataSizeCollector, but to also count the additional data types that I
pointed
out (if you don't want them in SiteDataSizeCollector). And add the time ranges.
I would pull it out of this CL for now.

Does that work for you?

[fukino, 2016-06-27 12:13:09]

On 2016/06/27 12:03:34, msramek wrote:
> On 2016/06/27 11:54:59, fukino wrote:
> > On 2016/06/24 11:50:52, msramek wrote:
> > > Oh, and I totally forgot... this doesn't support time ranges.
> > > 
> > > As you can see in the CBD dialog, the user can pick one of several time
> period
> > > options, and we need to show the data volume for that period. That means
> > > supporting counting limited to a time period between
> > > [SiteDataCounter::GetPeriodStart(), base::Time::Now()).
> > 
> > Thank you for the review, Martin!
> > 
> > Regarding time ranges, it looks it needs a certain amount of work, and I'm
> > afraid I won't be able to support it for M53 (this week)...
> > 
> > For M53, our new app, storage manager, needs only a "good estimate" about
how
> > much space is used for site data, so I want to implement the minimum
function
> > for M53 to meet the deadline. 
> > Should I move the implementation to count the size to somewhere inside
inside
> > our app, or can I continue to work on this CL and add support for time
ranges
> > later in M54?
> 
> I think you can leave SiteDataSizeCollector as it is, and use it for
> the storage manager in M53. It can definitely stay in this codebase.
> 
> Then for M54, you can implement SiteDataCounter to be not only a wrapper
around
> SiteDataSizeCollector, but to also count the additional data types that I
> pointed
> out (if you don't want them in SiteDataSizeCollector). And add the time
ranges.
> I would pull it out of this CL for now.
> 
> Does that work for you?

Yes, it works perfectly for me.
For additional data types, I'd like to add it to SiteDataSizeCollector to count
them in storage manager, too.
I'll continue to work on this CL to add tests and correct the code about quota
info. Thanks!

[fukino, 2016-06-30 09:21:25]

Martin, could you take another look?

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/site_data_counter.cc (right):

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/site_data_counter.cc:23:
weak_ptr_factory_.GetWeakPtr()));
On 2016/06/23 21:30:35, msramek wrote:
> nit: alignment.

Done.

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/site_data_counter.h (right):

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/site_data_counter.h:25: void Count() override;
On 2016/06/23 21:30:35, msramek wrote:
> nit: // BrowsingDataCounter:

Done.

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/site_data_size_collector.cc (right):

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/site_data_size_collector.cc:24: void
SiteDataSizeCollector::Fetch(const FetchCallback& callback) {
On 2016/06/23 21:30:35, msramek wrote:
> Looking at BrowsingDataRemover::REMOVE_SITE_DATA, there are a few items that
are
> not represented in this class:
> 
> - REMOVE_SITE_USAGE_DATA -> that's the data stored in
> CONTENT_SETTINGS_TYPE_APP_BANNER and CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT
(see
> WebsiteSettingsRegistry)
> 
> - REMOVE_WEB_APP_DATA -> see WebappDataStorage.java
> 
> - REMOVE_WEB_SQL_DATA -> that's actually hidden in the Quota model, so it's OK
> 
> - REMOVE_WEBRTC_IDENTITY -> that one is about to be removed in
> https://codereview.chromium.org/2033353002/, so it's OK
> 
> In addition, when you write tests, can you add a test that lists the items
from
> REMOVE_SITE_DATA that are already included in this class, and compares them
with
> REMOVE_SITE_DATA? I'd like to assure that people keep this class in sync with
> what is in BrowsingDataRemover.

I'll include SITE_USAGE_DATA and WEB_APP_DATA, and add a test to ensure all
types are included in a follow-up CL for M54.

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/site_data_size_collector.cc:156: void
SiteDataSizeCollector::OnQuotaModelInfoLoaded(
On 2016/06/23 21:30:35, msramek wrote:
> Quota model consists of these things:
> 
>
https://cs.chromium.org/chromium/src/content/browser/storage_partition_impl.c...
> 
> Which means that you're counting some things, such as IndexedDB, twice.

It seems all sizes in quota model are already counted by other helpers, so I
simply removed the BrowsingDataQuotaHelper.

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/site_data_size_collector.h (right):

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/site_data_size_collector.h:96: // Default storage
partition of the browser context.
On 2016/06/23 21:30:35, msramek wrote:
> nit: "...of this profile." (it's not called browser context at this layer)

I removed storage_partition_.
Only storage_partition_->GetPath() is needed, so I changed the class to keep the
path.

[msramek, 2016-06-30 13:58:11]

LGTM after you address all comments.

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/site_data_size_collector.cc (right):

https://codereview.chromium.org/2092663002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/site_data_size_collector.cc:24: void
SiteDataSizeCollector::Fetch(const FetchCallback& callback) {
On 2016/06/30 09:21:24, fukino wrote:
> On 2016/06/23 21:30:35, msramek wrote:
> > Looking at BrowsingDataRemover::REMOVE_SITE_DATA, there are a few items that
> are
> > not represented in this class:
> > 
> > - REMOVE_SITE_USAGE_DATA -> that's the data stored in
> > CONTENT_SETTINGS_TYPE_APP_BANNER and CONTENT_SETTINGS_TYPE_SITE_ENGAGEMENT
> (see
> > WebsiteSettingsRegistry)
> > 
> > - REMOVE_WEB_APP_DATA -> see WebappDataStorage.java
> > 
> > - REMOVE_WEB_SQL_DATA -> that's actually hidden in the Quota model, so it's
OK
> > 
> > - REMOVE_WEBRTC_IDENTITY -> that one is about to be removed in
> > https://codereview.chromium.org/2033353002/, so it's OK
> > 
> > In addition, when you write tests, can you add a test that lists the items
> from
> > REMOVE_SITE_DATA that are already included in this class, and compares them
> with
> > REMOVE_SITE_DATA? I'd like to assure that people keep this class in sync
with
> > what is in BrowsingDataRemover.
> 
> I'll include SITE_USAGE_DATA and WEB_APP_DATA, and add a test to ensure all
> types are included in a follow-up CL for M54.

Sounds good. Please add a TODO or file a bug for that.

Also, please split the counter to a separate CL. Without the remaining two
items, we cannot add it to the UI yet anyway; and since we're currently doing
some refactoring around counters, it would unnecessarily create merge conflicts.

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/site_data_size_collector.cc (right):

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector.cc:127: int64_t size = 0;
nit: Please use |size| or |total_size| in all methods for consistency.

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector.cc:132:
base::GetFileSize(cookie_file_path, &size);
This callback is on the IO thread, right? Can you DCHECK that before calling
GetFileSize()?

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector.cc:178:
base::GetFileSize(channel_id_file_path, &size);
Ditto here.

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector.cc:203: // Consider pepper
data directory size only when at least one Flash LSO is
Flash is not the only plugin; shouldn't we check instead if
PluginServiceImpl::GetPluginInfoArray() is nonempty?

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector.cc:205: base::FilePath
pepper_data_dir_path = default_storage_partition_path_
And here.

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/site_data_size_collector.h (right):

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector.h:75: const
ServiceWorkerUsageInfoList& service_worker_info);
nit: In the C++ file, this is called |service_worker_info_list|. Also
|cache_storage_info| -> |cache_storage_info_list| etc.

https://codereview.chromium.org/2092663002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/site_data_size_collector_unittest.cc (right):

https://codereview.chromium.org/2092663002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector_unittest.cc:127:
TEST_F(SiteDataSizeCollectorTest, FetchCookie1) {
nit: s/FetchCookie1/FetchCookie/

https://codereview.chromium.org/2092663002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector_unittest.cc:139:
EXPECT_EQ(static_cast<int64_t>(arraysize(kCookieFileData)), fetched_size_);
Please add a comment to explain that AddCookieSamples() actually doesn't write
the "A=1" cookie to the file, only triggers the condition to take the file into
account (also in the other instances below).

https://codereview.chromium.org/2092663002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector_unittest.cc:149: //
Fetched size should be 0 if there is no cookies.
nit: is no cookie / are no cookies

[fukino, 2016-06-30 16:16:13]

Thank you!

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/site_data_size_collector.cc (right):

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector.cc:127: int64_t size = 0;
On 2016/06/30 13:58:11, msramek wrote:
> nit: Please use |size| or |total_size| in all methods for consistency.

Done.

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector.cc:132:
base::GetFileSize(cookie_file_path, &size);
On 2016/06/30 13:58:11, msramek wrote:
> This callback is on the IO thread, right? Can you DCHECK that before calling
> GetFileSize()?

Oops! OnCookiesModelInfoLoaded is called UI thread.
I updated this CL to interact with files on blocking pool.

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector.cc:178:
base::GetFileSize(channel_id_file_path, &size);
On 2016/06/30 13:58:11, msramek wrote:
> Ditto here.

Done.

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector.cc:203: // Consider pepper
data directory size only when at least one Flash LSO is
On 2016/06/30 13:58:10, msramek wrote:
> Flash is not the only plugin; shouldn't we check instead if
> PluginServiceImpl::GetPluginInfoArray() is nonempty?

I could not find out what types of plugin data exist and where the files are
written.
Let me go with the Flash-only support and leave a TODO comment for now.

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector.cc:205: base::FilePath
pepper_data_dir_path = default_storage_partition_path_
On 2016/06/30 13:58:10, msramek wrote:
> And here.

Done.

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
File chrome/browser/browsing_data/site_data_size_collector.h (right):

https://codereview.chromium.org/2092663002/diff/40001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector.h:75: const
ServiceWorkerUsageInfoList& service_worker_info);
On 2016/06/30 13:58:11, msramek wrote:
> nit: In the C++ file, this is called |service_worker_info_list|. Also
> |cache_storage_info| -> |cache_storage_info_list| etc.

Done.

https://codereview.chromium.org/2092663002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/site_data_size_collector_unittest.cc (right):

https://codereview.chromium.org/2092663002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector_unittest.cc:127:
TEST_F(SiteDataSizeCollectorTest, FetchCookie1) {
On 2016/06/30 13:58:11, msramek wrote:
> nit: s/FetchCookie1/FetchCookie/

Done.

https://codereview.chromium.org/2092663002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector_unittest.cc:139:
EXPECT_EQ(static_cast<int64_t>(arraysize(kCookieFileData)), fetched_size_);
On 2016/06/30 13:58:11, msramek wrote:
> Please add a comment to explain that AddCookieSamples() actually doesn't write
> the "A=1" cookie to the file, only triggers the condition to take the file
into
> account (also in the other instances below).

Done.

https://codereview.chromium.org/2092663002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/site_data_size_collector_unittest.cc:149: //
Fetched size should be 0 if there is no cookies.
On 2016/06/30 13:58:11, msramek wrote:
> nit: is no cookie / are no cookies

Done.

[fukino, 2016-06-30 18:27:43]

The CQ bit was checked by fukino@chromium.org

[fukino, 2016-06-30 18:27:44]

The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2092663002/#ps80001
(title: "Address review comments.")

[commit-bot: I haz the power, 2016-06-30 18:28:18]

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-06-30 19:05:09]

Description was changed from

==========
Add a counter to calculate the total size of site data.

To compute the total size of site data in browsing data, this CL adds following
classes.
  SiteDataSizeCollector: collects size information of site data from storage
backends using BrowsingData*Helper. 
  SiteDataCounter: works as a BrowsingDataCounter using the size collector
above.

BUG=591958
==========

to

==========
Add a counter to calculate the total size of site data.

To compute the total size of site data in browsing data, this CL adds following
classes.
  SiteDataSizeCollector: collects size information of site data from storage
backends using BrowsingData*Helper. 
  SiteDataCounter: works as a BrowsingDataCounter using the size collector
above.

BUG=591958
==========

[commit-bot: I haz the power, 2016-06-30 19:05:10]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-30 19:08:25]

Description was changed from

==========
Add a counter to calculate the total size of site data.

To compute the total size of site data in browsing data, this CL adds following
classes.
  SiteDataSizeCollector: collects size information of site data from storage
backends using BrowsingData*Helper. 
  SiteDataCounter: works as a BrowsingDataCounter using the size collector
above.

BUG=591958
==========

to

==========
Add a counter to calculate the total size of site data.

To compute the total size of site data in browsing data, this CL adds following
classes.
  SiteDataSizeCollector: collects size information of site data from storage
backends using BrowsingData*Helper.
  SiteDataCounter: works as a BrowsingDataCounter using the size collector
above.

BUG=591958

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

[commit-bot: I haz the power, 2016-06-30 19:08:26]

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