[ServiceWorkerCache] Implement storage::QuotaClient

[ServiceWorkerCache] Implement storage::QuotaClient

Adds a QuotaClient for the ServiceWorkerCache system.

* Calls to the QuotaManagerProxy like (NotifyStorageAccessed/StorageModified/OriginInUse/etc..) will be in an upcoming CL.

* The ServiceWorkerCacheQuotaClient is registered by the ServiceWorkerCacheStorageManager.  The QuotaClient holds a WeakPtr to the ServiceWorkerCacheStorageManager since the manager is destroyed before the QuotaClient.

* Added a number of unittests.  They fit better into the StorageManager tests file than in their own file as they rely on a lot of the StorageManager infrastructure.

* In order to support QuotaClient the ServiceWorkerCacheStorageManager needs to be able to list the origins it supports. This data is not currently stored so an "origin" member has been added to the CacheStorage index protobuf.  We won't report the origins of existing caches (if any even exist at this point) until their index is next written.

BUG=420159
Committed: https://crrev.com/22fcbd3c898136dfe839de3ca5cb0633c17bb163
Cr-Commit-Position: refs/heads/master@{#300187}

[jkarlin, 2014-10-14 13:25:18]

Patchset #3 (id:100001) has been deleted

[jkarlin, 2014-10-15 18:57:57]

jkarlin@chromium.org changed reviewers:
+ michaeln@chromium.org

[jkarlin, 2014-10-15 18:57:58]

Thanks!

[falken, 2014-10-16 04:37:37]

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

[falken, 2014-10-16 04:37:38]

Drive by nits

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
File content/browser/service_worker/service_worker_cache_quota_client.cc
(right):

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_cache_quota_client.cc:34:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
do you have to call DoesSupport(type)?

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage_manager.cc
(right):

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage_manager.cc:39: bool
DeleteOriginDeleteDir(const base::FilePath& path) {
I see the point of prefixing the helper function with the name of the function
that's calling it, but it makes the name hard to parse at first glance. In this
case, just "DeleteDir" might be better.

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage_manager.h
(right):

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage_manager.h:119: bool
MemoryCache() const { return root_path_.empty(); }
The name doesn't sound like it returns a bool. IsMemoryBacked()?

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
File
content/browser/service_worker/service_worker_cache_storage_manager_unittest.cc
(right):

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage_manager_unittest.cc:43:
base::FilePath(), base::MessageLoopProxy::current(), NULL);
nullptr is recommended for new code: http://chromium-cpp.appspot.com/

[jkarlin, 2014-10-16 11:28:45]

Thanks falken!

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
File content/browser/service_worker/service_worker_cache_quota_client.cc
(right):

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_cache_quota_client.cc:34:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2014/10/16 04:37:37, falken wrote:
> do you have to call DoesSupport(type)?

Done. Thanks!

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage_manager.cc
(right):

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage_manager.cc:39: bool
DeleteOriginDeleteDir(const base::FilePath& path) {
On 2014/10/16 04:37:38, falken wrote:
> I see the point of prefixing the helper function with the name of the function
> that's calling it, but it makes the name hard to parse at first glance. In
this
> case, just "DeleteDir" might be better.

Done.

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage_manager.h
(right):

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage_manager.h:119: bool
MemoryCache() const { return root_path_.empty(); }
On 2014/10/16 04:37:38, falken wrote:
> The name doesn't sound like it returns a bool. IsMemoryBacked()?

Done.

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
File
content/browser/service_worker/service_worker_cache_storage_manager_unittest.cc
(right):

https://codereview.chromium.org/651983002/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage_manager_unittest.cc:43:
base::FilePath(), base::MessageLoopProxy::current(), NULL);
On 2014/10/16 04:37:38, falken wrote:
> nullptr is recommended for new code: http://chromium-cpp.appspot.com/

Done.

[falken, 2014-10-17 02:27:16]

sorry to toss in an architectural question this late

https://codereview.chromium.org/651983002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_cache_quota_client.h (right):

https://codereview.chromium.org/651983002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache_quota_client.h:18: 
Could you add a lightweight comment about who owns this and how many there are?
E.g., is there one per origin? one per profile?

https://codereview.chromium.org/651983002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage_manager.h
(right):

https://codereview.chromium.org/651983002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage_manager.h:40: :
public base::RefCounted<ServiceWorkerCacheStorageManager> {
If there's one SWCacheStorageManager per SWContextCore, it seems like we
shouldn't need refcounting. Could the people that need a pointer to it get it
through ContextCore?
(Refcounting is discouraged by
http://www.chromium.org/developers/smart-pointer-guidelines)

[jkarlin, 2014-10-17 13:58:43]

https://codereview.chromium.org/651983002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_cache_quota_client.h (right):

https://codereview.chromium.org/651983002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache_quota_client.h:18: 
On 2014/10/17 02:27:15, falken wrote:
> Could you add a lightweight comment about who owns this and how many there
are?
> E.g., is there one per origin? one per profile?

Done.

[jkarlin, 2014-10-17 14:57:28]

Patchset #11 (id:280001) has been deleted

[jkarlin, 2014-10-17 15:07:08]

Thanks falken.

https://codereview.chromium.org/651983002/diff/220001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage_manager.h
(right):

https://codereview.chromium.org/651983002/diff/220001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage_manager.h:40: :
public base::RefCounted<ServiceWorkerCacheStorageManager> {
On 2014/10/17 02:27:15, falken wrote:
> If there's one SWCacheStorageManager per SWContextCore, it seems like we
> shouldn't need refcounting. Could the people that need a pointer to it get it
> through ContextCore?
> (Refcounting is discouraged by
> http://www.chromium.org/developers/smart-pointer-guidelines)

Done. 

The QuotaClient must survive until the QuotaManager is destroyed.  The
QuotaManager is destroyed after the SWContextCore is destroyed.  I've changed it
so that the SWCacheQuotaClient holds a WeakPtr to the SWCacheStorageManager. 
This is essentially what AppCacheQuotaClient does as well.

[falken, 2014-10-17 15:52:03]

lgtm! the ref counted part of the CL description needs updating

https://codereview.chromium.org/651983002/diff/260001/content/browser/service...
File content/browser/service_worker/service_worker_cache_quota_client.h (right):

https://codereview.chromium.org/651983002/diff/260001/content/browser/service...
content/browser/service_worker/service_worker_cache_quota_client.h:20: //
ServiceWorkerCacheStorageManager, and therefore one per ServiceWorkerContext.
nit: ServiceWorkerContextCore

https://codereview.chromium.org/651983002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage_manager.h
(right):

https://codereview.chromium.org/651983002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage_manager.h:125: bool
IsMemoryBackend() const { return root_path_.empty(); }
I actually meant to suggest "IsMemoryBacked" (borrowing from
"CreateMemoryBackedBlob" terminology). Maybe IsMemoryBackend is good too? I'll
leave it up to you.

[jkarlin, 2014-10-17 16:14:00]

https://codereview.chromium.org/651983002/diff/260001/content/browser/service...
File content/browser/service_worker/service_worker_cache_quota_client.h (right):

https://codereview.chromium.org/651983002/diff/260001/content/browser/service...
content/browser/service_worker/service_worker_cache_quota_client.h:20: //
ServiceWorkerCacheStorageManager, and therefore one per ServiceWorkerContext.
On 2014/10/17 15:52:03, falken wrote:
> nit: ServiceWorkerContextCore

Done.

https://codereview.chromium.org/651983002/diff/300001/content/browser/service...
File content/browser/service_worker/service_worker_cache_storage_manager.h
(right):

https://codereview.chromium.org/651983002/diff/300001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage_manager.h:125: bool
IsMemoryBackend() const { return root_path_.empty(); }
On 2014/10/17 15:52:03, falken wrote:
> I actually meant to suggest "IsMemoryBacked" (borrowing from
> "CreateMemoryBackedBlob" terminology). Maybe IsMemoryBackend is good too? I'll
> leave it up to you.
Done. MemoryBackend works well for the disk_cache world but I'm happy using
Backed too.

[jkarlin, 2014-10-17 16:23:43]

michaeln: PTAL for owners of storage/  Thanks!

[michaeln, 2014-10-17 21:18:17]

lgtm 2

[jkarlin, 2014-10-17 22:48:07]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-10-17 22:49:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/651983002/320001

[commit-bot: I haz the power, 2014-10-17 23:38:30]

Committed patchset #12 (id:320001)

[commit-bot: I haz the power, 2014-10-17 23:39:24]

Patchset 12 (id:??) landed as
https://crrev.com/22fcbd3c898136dfe839de3ca5cb0633c17bb163
Cr-Commit-Position: refs/heads/master@{#300187}

[kareng, 2014-10-19 05:15:48]

A revert of this CL (patchset #12 id:320001) has been created in
https://codereview.chromium.org/651433005/ by kareng@google.com.

The reason for reverting is: bug 424923, top crash in canary..

[jkarlin, 2014-10-20 11:58:56]

On 2014/10/19 05:15:48, kareng wrote:
> A revert of this CL (patchset #12 id:320001) has been created in
> https://codereview.chromium.org/651433005/ by mailto:kareng@google.com.
> 
> The reason for reverting is: bug 424923, top crash in canary..

I believe that this CL was an innocent bystander and have updated the bug.  Will
try to reland today.