Content blink::WebServiceWorkerCacheStorage implementation.

Content blink::WebServiceWorkerCacheStorage implementation.

This new object is implemented in content in both the renderer and browser
side. The CacheStorage API methods now pass in to the browser process in
the UI thread, where all API calls are failed as not implemented.

R=jkarlin@chromium.org,falken@chromium.org,jochen@chromium.org,jschuh@chromium.org
BUG=392621
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288413

[gavinp, 2014-07-25 08:00:01]

It turns out it's an #include violation for the blink header to be included on
the browser side, so we need our own enum anyway copying it.

[gavinp, 2014-07-28 02:46:09]

falken, jochen, jkarlin, I think this is ready for review now. Please take a
look.

michaeln: FYI as well! Your help on the upstream issues made me want to point
this CL out to you.

Please note that this is downstream of the still pending blink CL
https://codereview.chromium.org/380923002/ , which provides the
WebServiceWorkerCacheStorage object; so you may still see some rebasing here
around upstream changes, and perhaps it's best reviewed together with changes
there.

https://codereview.chromium.org/379303002/diff/150001/content/common/service_...
File content/common/service_worker/service_worker_messages.h (right):

https://codereview.chromium.org/379303002/diff/150001/content/common/service_...
content/common/service_worker/service_worker_messages.h:8: #include <vector>
Lint pointed out that these were missing, and I had to agree.

[gavinp, 2014-07-28 04:00:16]

+jschuh for security review of IPC.

[falken, 2014-07-28 04:48:59]

lgtm

https://codereview.chromium.org/379303002/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_cache_listener.cc (right):

https://codereview.chromium.org/379303002/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_cache_listener.cc:47: 
nit: extra newline

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_context_client.cc (right):

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_context_client.cc:127:
EmbeddedWorkerContextClient:: cacheStorage() {
nit: space after ::

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
File content/renderer/service_worker/service_worker_cache_storage_dispatcher.h
(right):

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.h:22: //
to the browser process and runs  callbacks at operation completion.
nit: extra space after "runs"

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
content/renderer/service_worker/service_worker_script_context.cc:130: class
WebServiceWorkerCacheStorage;
this is not meant to be here?

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
File content/renderer/service_worker/service_worker_script_context.h (right):

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
content/renderer/service_worker/service_worker_script_context.h:90: 
nit: extra newline

[gavinp, 2014-07-28 04:55:27]

Thanks falken

https://codereview.chromium.org/379303002/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_cache_listener.cc (right):

https://codereview.chromium.org/379303002/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_cache_listener.cc:47: 
On 2014/07/28 04:48:58, falken wrote:
> nit: extra newline

Done.

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_context_client.cc (right):

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_context_client.cc:127:
EmbeddedWorkerContextClient:: cacheStorage() {
On 2014/07/28 04:48:58, falken wrote:
> nit: space after ::

Done.

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
File content/renderer/service_worker/service_worker_cache_storage_dispatcher.h
(right):

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.h:22: //
to the browser process and runs  callbacks at operation completion.
On 2014/07/28 04:48:58, falken wrote:
> nit: extra space after "runs"

Done.

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
File content/renderer/service_worker/service_worker_script_context.cc (right):

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
content/renderer/service_worker/service_worker_script_context.cc:130: class
WebServiceWorkerCacheStorage;
On 2014/07/28 04:48:59, falken wrote:
> this is not meant to be here?

Done.

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
File content/renderer/service_worker/service_worker_script_context.h (right):

https://codereview.chromium.org/379303002/diff/190001/content/renderer/servic...
content/renderer/service_worker/service_worker_script_context.h:90: 
On 2014/07/28 04:48:59, falken wrote:
> nit: extra newline

Done.

[gavinp, 2014-07-28 07:22:59]

This CL is now downstream of https://codereview.chromium.org/424643002/ , the
rename for API conformance with spec changes that have happened in the interim.

[jochen (gone - plz use gerrit), 2014-07-28 08:53:08]

it would be nice to have unit tests for the listener. It currently doesn't do a
lot, so feel free to defer until the class actually does something. but then it
needs tests.

lgtm with nits

[jochen (gone - plz use gerrit), 2014-07-28 11:15:51]

https://codereview.chromium.org/379303002/diff/250001/content/browser/service...
File content/browser/service_worker/service_worker_stores_listener.h (right):

https://codereview.chromium.org/379303002/diff/250001/content/browser/service...
content/browser/service_worker/service_worker_stores_listener.h:40: };
disallow copy/assign

https://codereview.chromium.org/379303002/diff/250001/content/renderer/servic...
File content/renderer/service_worker/service_worker_fetch_stores_dispatcher.h
(right):

https://codereview.chromium.org/379303002/diff/250001/content/renderer/servic...
content/renderer/service_worker/service_worker_fetch_stores_dispatcher.h:27:
ServiceWorkerFetchStoresDispatcher(
explicit

https://codereview.chromium.org/379303002/diff/250001/content/renderer/servic...
content/renderer/service_worker/service_worker_fetch_stores_dispatcher.h:53:
virtual void dispatchGet(FetchStoresWithFetchStoreCallbacks* callbacks,
no OVERRIDE on blink API implementations

https://codereview.chromium.org/379303002/diff/250001/content/renderer/servic...
content/renderer/service_worker/service_worker_fetch_stores_dispatcher.h:78: };
disallow copy/assign

[michaeln, 2014-07-28 22:43:31]

lgtm2, great to see the cache coming alive!

[jschuh, 2014-07-29 19:43:23]

This is just stubs, so rubberstamp lgtm for IPC security. But, you'll need a
security review for the CL with the implementation, because that's the
potentially risky part.

[jochen (gone - plz use gerrit), 2014-08-05 08:58:46]

do you want another review for this as well?

[gavinp, 2014-08-05 09:06:47]

On 2014/08/05 08:58:46, jochen wrote:
> do you want another review for this as well?

I have definitely taken a lot of renames through here, so that would be great.
The tries fail because the upstream in WebKit isn't landed.

[jochen (gone - plz use gerrit), 2014-08-05 09:16:42]

https://codereview.chromium.org/379303002/diff/290001/content/renderer/servic...
File content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc
(right):

https://codereview.chromium.org/379303002/diff/290001/content/renderer/servic...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc:18:
ServiceWorkerCacheStorageDispatcher::~ServiceWorkerCacheStorageDispatcher() { }
what happens with callbacks that still exist at this point? Should you invoke
onError on all of them? Or DCHECK that there aren't any?

https://codereview.chromium.org/379303002/diff/290001/content/renderer/servic...
File content/renderer/service_worker/service_worker_cache_storage_dispatcher.h
(right):

https://codereview.chromium.org/379303002/diff/290001/content/renderer/servic...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.h:27:
ServiceWorkerCacheStorageDispatcher(
explicit

https://codereview.chromium.org/379303002/diff/290001/content/renderer/servic...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.h:53:
virtual void dispatchGet(CacheStorageWithCacheCallbacks* callbacks,
don't put override on blink api implementations

https://codereview.chromium.org/379303002/diff/290001/content/renderer/servic...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.h:78: };
disallow copy/assign

[gavinp, 2014-08-05 23:51:07]

Thanks for the review jochen.

PTAL, how's this now?

https://codereview.chromium.org/379303002/diff/290001/content/renderer/servic...
File content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc
(right):

https://codereview.chromium.org/379303002/diff/290001/content/renderer/servic...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.cc:18:
ServiceWorkerCacheStorageDispatcher::~ServiceWorkerCacheStorageDispatcher() { }
On 2014/08/05 09:16:41, jochen wrote:
> what happens with callbacks that still exist at this point? Should you invoke
> onError on all of them? Or DCHECK that there aren't any?

I think DCHECK() that there are none is not right; the service worker lifetime
isn't bound by the completion of requests as far as I know, and besides, the
cache API can be used outside of the context of requests.

So I'm inclined to say onError() for now, and hopefully falken@ or dominicc@
will confirm this?

https://codereview.chromium.org/379303002/diff/290001/content/renderer/servic...
File content/renderer/service_worker/service_worker_cache_storage_dispatcher.h
(right):

https://codereview.chromium.org/379303002/diff/290001/content/renderer/servic...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.h:27:
ServiceWorkerCacheStorageDispatcher(
On 2014/08/05 09:16:41, jochen wrote:
> explicit

Done.

https://codereview.chromium.org/379303002/diff/290001/content/renderer/servic...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.h:53:
virtual void dispatchGet(CacheStorageWithCacheCallbacks* callbacks,
On 2014/08/05 09:16:41, jochen wrote:
> don't put override on blink api implementations

Done.

https://codereview.chromium.org/379303002/diff/290001/content/renderer/servic...
content/renderer/service_worker/service_worker_cache_storage_dispatcher.h:78: };
On 2014/08/05 09:16:41, jochen wrote:
> disallow copy/assign

Done.

[jochen (gone - plz use gerrit), 2014-08-06 15:41:45]

lgtm given that either falken or dominicc are happy with this

[falken, 2014-08-07 03:33:31]

On 2014/08/06 15:41:45, jochen (slow - soon OOO) wrote:
> lgtm given that either falken or dominicc are happy with this

lgtm for the reasons gavinp mentioned. It's possible SW can die while there's an
outstanding request.

[gavinp, 2014-08-08 17:12:30]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-08-08 17:14:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gavinp@chromium.org/379303002/310001

[commit-bot: I haz the power, 2014-08-08 19:25:00]

Change committed as 288413