Cache Storage API: Hook up to chrome://settings/cookies

Cache Storage API: Hook up to chrome://settings/cookies

Expose each origin's usage (bytes, last modified) of the Service
Worker Cache Storage API in the cookie tree UI and allow
privacy-concerned individuals to nuke it from orbit.

BUG=466371
Committed: https://crrev.com/4c354468b01a2389a886ce21d1869582ac20cbe8
Cr-Commit-Position: refs/heads/master@{#344891}

Committed: https://crrev.com/e1fe969b4080d3990dd4712e64411717159917b1
Cr-Commit-Position: refs/heads/master@{#344944}

[jsbell, 2015-08-17 23:46:50]

omfg I hate this code.

michaeln@, jkarlin@ - can you take an initial look before I rope in other
owners?

[jkarlin, 2015-08-18 13:22:36]

I looked at the content/ code only. That part lgtm with nits.

https://codereview.chromium.org/1297093002/diff/1/content/browser/cache_stora...
File content/browser/cache_storage/cache_storage_manager.cc (right):

https://codereview.chromium.org/1297093002/diff/1/content/browser/cache_stora...
content/browser/cache_storage/cache_storage_manager.cc:241: // TODO(jsbell):
Should this be filtering by origin?
Eek, yes!

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
File content/public/browser/cache_storage_context.h (right):

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
content/public/browser/cache_storage_context.h:10: #include "base/basictypes.h"
not needed?

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
content/public/browser/cache_storage_context.h:13: #include
"content/common/content_export.h"
not needed?

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
content/public/browser/cache_storage_context.h:22: typedef
base::Callback<void(const std::vector<CacheStorageUsageInfo>&
using instead of typedef

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
content/public/browser/cache_storage_context.h:32: virtual
~CacheStorageContext() {}
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
File content/public/browser/cache_storage_usage_info.h (right):

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
content/public/browser/cache_storage_usage_info.h:8: #include
"base/files/file_path.h"
not needed

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
content/public/browser/cache_storage_usage_info.h:16: class CONTENT_EXPORT
CacheStorageUsageInfo {
Looks like you mean for this to be a struct not a class.

[jsbell, 2015-08-18 18:42:43]

https://codereview.chromium.org/1297093002/diff/1/content/browser/cache_stora...
File content/browser/cache_storage/cache_storage_manager.cc (right):

https://codereview.chromium.org/1297093002/diff/1/content/browser/cache_stora...
content/browser/cache_storage/cache_storage_manager.cc:241: // TODO(jsbell):
Should this be filtering by origin?
On 2015/08/18 13:22:35, jkarlin wrote:
> Eek, yes!

Done, and added unit tests.

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
File content/public/browser/cache_storage_context.h (right):

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
content/public/browser/cache_storage_context.h:10: #include "base/basictypes.h"
On 2015/08/18 13:22:35, jkarlin wrote:
> not needed?

Done.

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
content/public/browser/cache_storage_context.h:13: #include
"content/common/content_export.h"
On 2015/08/18 13:22:35, jkarlin wrote:
> not needed?

Done.

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
content/public/browser/cache_storage_context.h:22: typedef
base::Callback<void(const std::vector<CacheStorageUsageInfo>&
On 2015/08/18 13:22:36, jkarlin wrote:
> using instead of typedef

Done.

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
content/public/browser/cache_storage_context.h:32: virtual
~CacheStorageContext() {}
On 2015/08/18 13:22:35, jkarlin wrote:
> DISALLOW_COPY_AND_ASSIGN?

It looks like this is not done for the pure virtual classes in content/public
since it complicates implementations.

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
File content/public/browser/cache_storage_usage_info.h (right):

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
content/public/browser/cache_storage_usage_info.h:8: #include
"base/files/file_path.h"
On 2015/08/18 13:22:36, jkarlin wrote:
> not needed

Done.

https://codereview.chromium.org/1297093002/diff/1/content/public/browser/cach...
content/public/browser/cache_storage_usage_info.h:16: class CONTENT_EXPORT
CacheStorageUsageInfo {
On 2015/08/18 13:22:36, jkarlin wrote:
> Looks like you mean for this to be a struct not a class.

Done.

[michaeln, 2015-08-18 22:25:52]

lgtm

> omfg I hate this code
me too, /content is ok but /chrome is another story

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

https://codereview.chromium.org/1297093002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_cache_storage_helper.cc:67:
cache_storage_info_.push_back(origin);
is the cache_storagse_info_ data member really needed, look like you may be able
create the list as a local and then pass it NotifyOnUIThread in the bind
closure?

https://codereview.chromium.org/1297093002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_cache_storage_helper.cc:89:
CannedBrowsingDataCacheStorageHelper::PendingCacheStorageUsageInfo::
i hope this canned stuff is dead stripped, i don't like this code either

https://codereview.chromium.org/1297093002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_cache_storage_helper.h (right):

https://codereview.chromium.org/1297093002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_cache_storage_helper.h:79: // Deletes
Cache Storages for an origin the IO thread.
origin -on- the IO thread

but consider removing the comments on these private methods, the names are very
descriptive

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

https://codereview.chromium.org/1297093002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:1742:
BlockUntilOriginDataRemoved(BrowsingDataRemover::EVERYTHING,
nothing to do with this cl, but EVERYTHING is not the best name for the "all
time" time period

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

https://codereview.chromium.org/1297093002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/cookies_tree_model.cc:577:
container->cache_storage_info_list_.erase(cache_storage_info_);
does this invalidate our cache_storage_info_ data member?

https://codereview.chromium.org/1297093002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/mock_browsing_data_cache_storage_helper.h
(right):

https://codereview.chromium.org/1297093002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/mock_browsing_data_cache_storage_helper.h:20: class
MockBrowsingDataCacheStorageHelper
+1 when in rome but... (insert munch scream image)... why both 'canned' and
'mock' test oriented derivatives???

[jsbell, 2015-08-19 01:06:19]

Initial simplification per review. I didn't attempt to merge the Canned/Mock
classes.

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

https://codereview.chromium.org/1297093002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_cache_storage_helper.cc:67:
cache_storage_info_.push_back(origin);
On 2015/08/18 22:25:51, michaeln wrote:
> is the cache_storagse_info_ data member really needed, look like you may be
able
> create the list as a local and then pass it NotifyOnUIThread in the bind
> closure?

Done. Excellent suggestion, definitely simplifies things.

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

https://codereview.chromium.org/1297093002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/cookies_tree_model.cc:577:
container->cache_storage_info_list_.erase(cache_storage_info_);
On 2015/08/18 22:25:51, michaeln wrote:
> does this invalidate our cache_storage_info_ data member?

Yes, but this is called just before this node itself is deleted, see
DeleteAllStoredObjects / DeleteCookieNode

[michaeln, 2015-08-19 21:11:03]

still lgtm

[jsbell, 2015-08-19 21:31:26]

jsbell@chromium.org changed reviewers:
+ bauerb@chromium.org, isherman@chromium.org, jam@chromium.org

[jsbell, 2015-08-19 21:31:27]

bauerb@ - can you please review changes in
chrome/browser/{android,content_settings,extensions,resources,ui}

isherman@ - can you please review the histogram changes?

jam@ - can you please review the content/ changes?

Thanks, all!

[Ilya Sherman, 2015-08-20 00:21:13]

histograms lgtm

[Bernhard Bauer, 2015-08-20 11:18:53]

LGTM, but I found some nits:

https://codereview.chromium.org/1297093002/diff/60001/chrome/browser/browsing...
File chrome/browser/browsing_data/mock_browsing_data_cache_storage_helper.h
(right):

https://codereview.chromium.org/1297093002/diff/60001/chrome/browser/browsing...
chrome/browser/browsing_data/mock_browsing_data_cache_storage_helper.h:20: class
MockBrowsingDataCacheStorageHelper
On 2015/08/18 22:25:52, michaeln wrote:
> +1 when in rome but... (insert munch scream image)... why both 'canned' and
> 'mock' test oriented derivatives???

FWIW, canned isn't actually for testing -- it's for showing data set by the
current page ("Show cookies and site data" from the Page Info bubble).

https://codereview.chromium.org/1297093002/diff/120001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/1297093002/diff/120001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:91: // storage, webSQL,
service workers, caches storage, and plugin data.
Nit: "cache storage"

https://codereview.chromium.org/1297093002/diff/120001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_manager.h (right):

https://codereview.chromium.org/1297093002/diff/120001/content/browser/cache_...
content/browser/cache_storage/cache_storage_manager.h:116: static
std::vector<CacheStorageUsageInfo> GetAllOriginsUsageOnTaskRunner(
Could this move to an anonymous namespace in the .cc file?

https://codereview.chromium.org/1297093002/diff/120001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_manager_unittest.cc (right):

https://codereview.chromium.org/1297093002/diff/120001/content/browser/cache_...
content/browser/cache_storage/cache_storage_manager_unittest.cc:229:
scoped_ptr<base::RunLoop> loop(new base::RunLoop());
Just allocate the RunLoop directly on the stack.

https://codereview.chromium.org/1297093002/diff/120001/content/public/browser...
File content/public/browser/storage_partition.h (right):

https://codereview.chromium.org/1297093002/diff/120001/content/public/browser...
content/public/browser/storage_partition.h:52: class CacheStorageContext;
Can you keep these ordered alphabetically?

[jsbell, 2015-08-20 19:04:51]

https://codereview.chromium.org/1297093002/diff/120001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/1297093002/diff/120001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover.h:91: // storage, webSQL,
service workers, caches storage, and plugin data.
On 2015/08/20 11:18:53, Bernhard Bauer wrote:
> Nit: "cache storage"

Done.

https://codereview.chromium.org/1297093002/diff/120001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_manager.h (right):

https://codereview.chromium.org/1297093002/diff/120001/content/browser/cache_...
content/browser/cache_storage/cache_storage_manager.h:116: static
std::vector<CacheStorageUsageInfo> GetAllOriginsUsageOnTaskRunner(
On 2015/08/20 11:18:53, Bernhard Bauer wrote:
> Could this move to an anonymous namespace in the .cc file?

Yes... need to make ConstructOriginPath public though.

https://codereview.chromium.org/1297093002/diff/120001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_manager_unittest.cc (right):

https://codereview.chromium.org/1297093002/diff/120001/content/browser/cache_...
content/browser/cache_storage/cache_storage_manager_unittest.cc:229:
scoped_ptr<base::RunLoop> loop(new base::RunLoop());
On 2015/08/20 11:18:53, Bernhard Bauer wrote:
> Just allocate the RunLoop directly on the stack.

Done. (Here and elsewhere.)

https://codereview.chromium.org/1297093002/diff/120001/content/public/browser...
File content/public/browser/storage_partition.h (right):

https://codereview.chromium.org/1297093002/diff/120001/content/public/browser...
content/public/browser/storage_partition.h:52: class CacheStorageContext;
On 2015/08/20 11:18:53, Bernhard Bauer wrote:
> Can you keep these ordered alphabetically?

Done.

[jam, 2015-08-20 22:57:08]

lgtm

https://codereview.chromium.org/1297093002/diff/140001/content/public/browser...
File content/public/browser/cache_storage_usage_info.cc (right):

https://codereview.chromium.org/1297093002/diff/140001/content/public/browser...
content/public/browser/cache_storage_usage_info.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
to check: is this file needed to avoid clang errors? or can you inline the
methods that are needed in the cc?

https://codereview.chromium.org/1297093002/diff/140001/content/public/browser...
content/public/browser/cache_storage_usage_info.cc:20: const
CacheStorageUsageInfo& other) = default;
we need all these?

[jsbell, 2015-08-20 23:46:13]

https://codereview.chromium.org/1297093002/diff/140001/content/public/browser...
File content/public/browser/cache_storage_usage_info.cc (right):

https://codereview.chromium.org/1297093002/diff/140001/content/public/browser...
content/public/browser/cache_storage_usage_info.cc:1: // Copyright 2015 The
Chromium Authors. All rights reserved.
On 2015/08/20 22:57:08, jam wrote:
> to check: is this file needed to avoid clang errors? or can you inline the
> methods that are needed in the cc?

Apparently the ctor can be inlined. Could be because it's now a struct, but
either way I'm guilty of copy/pasting. Removed!

https://codereview.chromium.org/1297093002/diff/140001/content/public/browser...
content/public/browser/cache_storage_usage_info.cc:20: const
CacheStorageUsageInfo& other) = default;
On 2015/08/20 22:57:08, jam wrote:
> we need all these?

Apparently we don't. Removed!

[jsbell, 2015-08-21 16:47:31]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2015-08-21 16:47:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkarlin@chromium.org, michaeln@chromium.org, bauerb@chromium.org,
isherman@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1297093002/#ps160001
(title: "Remove cache_storage_usage_info.cc")

[commit-bot: I haz the power, 2015-08-21 16:48:46]

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

[commit-bot: I haz the power, 2015-08-21 16:57:02]

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

[commit-bot: I haz the power, 2015-08-21 16:57:03]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jsbell, 2015-08-21 16:58:10]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2015-08-21 16:58:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, bauerb@chromium.org, jkarlin@chromium.org,
isherman@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1297093002/#ps180001
(title: "Rebased")

[commit-bot: I haz the power, 2015-08-21 16:58:32]

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

[commit-bot: I haz the power, 2015-08-21 18:12:37]

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

[commit-bot: I haz the power, 2015-08-21 18:12:38]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[jsbell, 2015-08-21 18:59:17]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2015-08-21 19:00:51]

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

[commit-bot: I haz the power, 2015-08-21 20:11:32]

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

[commit-bot: I haz the power, 2015-08-21 20:11:34]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[jsbell, 2015-08-21 22:05:14]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2015-08-21 22:06:02]

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

[commit-bot: I haz the power, 2015-08-21 22:49:09]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-08-21 22:49:59]

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

[alexmos, 2015-08-21 23:23:02]

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

[alexmos, 2015-08-21 23:23:03]

I had to revert this due to compile failures on the waterfall:
https://chromium.googlesource.com/chromium/src/+/9db1c3724bccfe63532a203e4e0a...

[jsbell, 2015-08-21 23:53:47]

On 2015/08/21 23:23:03, alexmos wrote:
> I had to revert this due to compile failures on the waterfall:
>
https://chromium.googlesource.com/chromium/src/+/9db1c3724bccfe63532a203e4e0a...

Thanks.

As diagnosed by dfalcantara@, estark@ landed (also via the CQ) an incompatible
change just before mine went in. One line fix in PS#11

[jsbell, 2015-08-21 23:54:09]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2015-08-21 23:54:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, bauerb@chromium.org, jkarlin@chromium.org,
jam@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1297093002/#ps200001
(title: "Rebase onto https://codereview.chromium.org/1297023004")

[commit-bot: I haz the power, 2015-08-21 23:55:06]

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

[commit-bot: I haz the power, 2015-08-22 01:02:54]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-08-22 01:03:33]

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