Introducing the WebServiceWorkerCacheStorage.

Introducing the WebServiceWorkerCacheStorage.

Carries the CacheStorage API through to the chrome renderer side. For now, just
returns cache_ids as raw ints to the JS, for lack of an object representing the
Cache.

R=jkarlin@chromium.org,falken@chromium.org,michaeln@chromium.org,jochen@chromium.org
BUG=392621

[gavinp, 2014-07-22 09:03:39]

falken: can you take a first look through this? We can add API reviewers after
you and I sort out that things are in good places.

jkarlin: FYI.

[gavinp, 2014-07-22 09:04:17]

Note: this is downstream of https://codereview.chromium.org/400493005/ , without
that patched in you won't link.

[falken, 2014-07-23 04:55:57]

LGTM

[falken, 2014-07-23 04:56:11]

oops forgot comments

https://codereview.chromium.org/380923002/diff/120001/public/platform/WebServ...
File public/platform/WebServiceWorkerCacheStorage.h (right):

https://codereview.chromium.org/380923002/diff/120001/public/platform/WebServ...
public/platform/WebServiceWorkerCacheStorage.h:13: #include <vector>
Seems <string> and <vector> aren't used?

https://codereview.chromium.org/380923002/diff/120001/public/platform/WebServ...
public/platform/WebServiceWorkerCacheStorage.h:25: CacheStorageErrorExists,
nit: enum values should be prefixed with the name of the enum:
http://www.chromium.org/blink/public-c-api

[jkarlin, 2014-07-23 12:03:32]

https://codereview.chromium.org/380923002/diff/140001/public/platform/WebServ...
File public/platform/WebServiceWorkerCacheStorage.h (right):

https://codereview.chromium.org/380923002/diff/140001/public/platform/WebServ...
public/platform/WebServiceWorkerCacheStorage.h:22: enum ErrorReason {
Please add 'CacheStorageErrorNoError' as the first entry to this so that the
caching functions can return an 'OK' error code.

[michaeln, 2014-07-23 21:21:08]

https://codereview.chromium.org/380923002/diff/140001/public/platform/WebServ...
File public/platform/WebServiceWorker.h (right):

https://codereview.chromium.org/380923002/diff/140001/public/platform/WebServ...
public/platform/WebServiceWorker.h:53: virtual WebServiceWorkerCacheStorage*
cacheStorage() { return 0; }
this method seems out of place here. wassup with this one?

[jkarlin, 2014-07-24 13:20:16]

https://codereview.chromium.org/380923002/diff/140001/public/platform/WebServ...
File public/platform/WebServiceWorkerCacheStorage.h (right):

https://codereview.chromium.org/380923002/diff/140001/public/platform/WebServ...
public/platform/WebServiceWorkerCacheStorage.h:26: CacheStorageErrorLast =
CacheStorageErrorExists
Please append CacheStorageErrorStorage to the enum for things like backend
failures (e.g., couldn't mkdir).

[gavinp, 2014-07-25 07:44:03]

https://codereview.chromium.org/380923002/diff/120001/public/platform/WebServ...
File public/platform/WebServiceWorkerCacheStorage.h (right):

https://codereview.chromium.org/380923002/diff/120001/public/platform/WebServ...
public/platform/WebServiceWorkerCacheStorage.h:13: #include <vector>
On 2014/07/23 04:56:10, falken wrote:
> Seems <string> and <vector> aren't used?

Done.

https://codereview.chromium.org/380923002/diff/120001/public/platform/WebServ...
public/platform/WebServiceWorkerCacheStorage.h:25: CacheStorageErrorExists,
On 2014/07/23 04:56:10, falken wrote:
> nit: enum values should be prefixed with the name of the enum:
> http://www.chromium.org/blink/public-c-api

Done.

https://codereview.chromium.org/380923002/diff/140001/public/platform/WebServ...
File public/platform/WebServiceWorker.h (right):

https://codereview.chromium.org/380923002/diff/140001/public/platform/WebServ...
public/platform/WebServiceWorker.h:53: virtual WebServiceWorkerCacheStorage*
cacheStorage() { return 0; }
On 2014/07/23 21:21:07, michaeln wrote:
> this method seems out of place here. wassup with this one?

It's cruft from an attempt at a different path. Good catch.

https://codereview.chromium.org/380923002/diff/140001/public/platform/WebServ...
File public/platform/WebServiceWorkerCacheStorage.h (right):

https://codereview.chromium.org/380923002/diff/140001/public/platform/WebServ...
public/platform/WebServiceWorkerCacheStorage.h:22: enum ErrorReason {
On 2014/07/23 12:03:32, jkarlin_OOO wrote:
> Please add 'CacheStorageErrorNoError' as the first entry to this so that the
> caching functions can return an 'OK' error code.

(your name still shows OOO from your vacation...)

I think because the error reason is only provided on callbacks that fail, having
OK here would be a mistake. If it's being extended downstream by someone that's
OK, even if it is annoying to extend an enum.

https://codereview.chromium.org/380923002/diff/140001/public/platform/WebServ...
public/platform/WebServiceWorkerCacheStorage.h:26: CacheStorageErrorLast =
CacheStorageErrorExists
On 2014/07/24 13:20:16, jkarlin_OOO wrote:
> Please append CacheStorageErrorStorage to the enum for things like backend
> failures (e.g., couldn't mkdir).

Tricky decision. I can see the argument; this failure can occur, and why not let
the blink code handle the mapping from all the various types of failures to the
API requirements. In one sense, that's keeping API logic in blink which is good.

On the other hand, why have the WebFoo interfaces include implementation details
from the embedder? Isn't "NotFound" the right return here? Can anyone else chime
in on the right call?

I am kind of inclined to do what Josh is suggesting, add more values here, and
map them into exceptions from the serviceworker spec in CacheStorage.cpp?

[gavinp, 2014-07-25 07:45:00]

+abarth for API review

Thanks everyone for your comments. I've remediated and brought this up to trunk.
The downstream CLs in Chrome are now working, so it's ready to land this.

[michaeln, 2014-07-25 20:25:19]

lgtm, some drive-by-comments below

https://codereview.chromium.org/380923002/diff/200001/Source/modules/servicew...
File Source/modules/serviceworkers/CacheStorage.cpp (right):

https://codereview.chromium.org/380923002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/CacheStorage.cpp:100: PassRefPtr<CacheStorage>
CacheStorage::create(blink::WebServiceWorkerCacheStorage* webCacheStorage)
shouldn't need the blink:: here

https://codereview.chromium.org/380923002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/CacheStorage.cpp:170:
CacheStorage::CacheStorage(blink::WebServiceWorkerCacheStorage* webCacheStorage)
: m_webCacheStorage(webCacheStorage)
or here

https://codereview.chromium.org/380923002/diff/200001/Source/modules/servicew...
File Source/modules/serviceworkers/CacheStorage.h (right):

https://codereview.chromium.org/380923002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/CacheStorage.h:38:
CacheStorage(blink::WebServiceWorkerCacheStorage*);
explicit

https://codereview.chromium.org/380923002/diff/200001/public/platform/WebServ...
File public/platform/WebServiceWorker.h (right):

https://codereview.chromium.org/380923002/diff/200001/public/platform/WebServ...
public/platform/WebServiceWorker.h:43: class WebServiceWorkerCacheStorage;
still needed?

https://codereview.chromium.org/380923002/diff/200001/public/platform/WebServ...
File public/platform/WebServiceWorkerCacheStorage.h (right):

https://codereview.chromium.org/380923002/diff/200001/public/platform/WebServ...
public/platform/WebServiceWorkerCacheStorage.h:13: namespace WebCore { class
ServiceWorker; }
needed?

https://codereview.chromium.org/380923002/diff/200001/public/platform/WebServ...
public/platform/WebServiceWorkerCacheStorage.h:17: // An interface to the
CacheStorage API, implemented by the embedder and passed in to blink.
very helpful to describe who implements vs who consumes

[gavinp, 2014-07-28 02:10:17]

Thanks for the review, Michael. Could you look at my re-wording of the comment
you asked about? I think I addressed your concerns, but if I haven't, could you
call it out a bit more bluntly for me?

The latest upload also splits out the WebServiceWorkerCacheStorageError enum
into its own header; this lets it be included in content/common and
content/browser downstream, which is useful. But Josh Karlin's earlier questions
about what values rightly lie in this enum is still out there, should it map
closely to API errors, or include a wide variety of errors that embedders
experience?

https://codereview.chromium.org/380923002/diff/200001/Source/modules/servicew...
File Source/modules/serviceworkers/CacheStorage.cpp (right):

https://codereview.chromium.org/380923002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/CacheStorage.cpp:100: PassRefPtr<CacheStorage>
CacheStorage::create(blink::WebServiceWorkerCacheStorage* webCacheStorage)
On 2014/07/25 20:25:19, michaeln wrote:
> shouldn't need the blink:: here

Done, and one more below.

https://codereview.chromium.org/380923002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/CacheStorage.cpp:170:
CacheStorage::CacheStorage(blink::WebServiceWorkerCacheStorage* webCacheStorage)
: m_webCacheStorage(webCacheStorage)
On 2014/07/25 20:25:19, michaeln wrote:
> or here

Done.

https://codereview.chromium.org/380923002/diff/200001/Source/modules/servicew...
File Source/modules/serviceworkers/CacheStorage.h (right):

https://codereview.chromium.org/380923002/diff/200001/Source/modules/servicew...
Source/modules/serviceworkers/CacheStorage.h:38:
CacheStorage(blink::WebServiceWorkerCacheStorage*);
On 2014/07/25 20:25:19, michaeln wrote:
> explicit

Done.

https://codereview.chromium.org/380923002/diff/200001/public/platform/WebServ...
File public/platform/WebServiceWorker.h (right):

https://codereview.chromium.org/380923002/diff/200001/public/platform/WebServ...
public/platform/WebServiceWorker.h:43: class WebServiceWorkerCacheStorage;
On 2014/07/25 20:25:19, michaeln wrote:
> still needed?

Nope. This whole file should not be a diff and not in the CL any more.

https://codereview.chromium.org/380923002/diff/200001/public/platform/WebServ...
File public/platform/WebServiceWorkerCacheStorage.h (right):

https://codereview.chromium.org/380923002/diff/200001/public/platform/WebServ...
public/platform/WebServiceWorkerCacheStorage.h:13: namespace WebCore { class
ServiceWorker; }
On 2014/07/25 20:25:19, michaeln wrote:
> needed?

Nope.

https://codereview.chromium.org/380923002/diff/200001/public/platform/WebServ...
public/platform/WebServiceWorkerCacheStorage.h:17: // An interface to the
CacheStorage API, implemented by the embedder and passed in to blink.
On 2014/07/25 20:25:19, michaeln wrote:
> very helpful to describe who implements vs who consumes

Just tried to make the comment more verbose; does the new working make this more
clear?

[gavinp, 2014-07-28 05:34:59]

Naturally, the spec has evolved in the meantime. To avoid churn on the
Chrome/content interface, we should probably update the unlanded content-side CL
before proceeding, but I'm OK with leaving this CL as it is, and following it up
with a "bring it up to spec" CL right after if everyone here is OK with that,
too?

[michaeln, 2014-07-28 22:35:26]

The code comment was good before and still is after. I really wasn't asking for
any clarification, just saying that I liked that you had indicated the
'directionality' so-to-speak.

> Thanks for the review, Michael. Could you look at my re-wording of the comment
> you asked about? I think I addressed your concerns, but if I haven't, could
you
> call it out a bit more bluntly for me?

[gavinp, 2014-07-29 02:03:22]

+jochen as you have more context

-abarth (please feel free to drive by!!)

Jochen,

This introduces public API changes which actually are going to be immediately
changed by https://codereview.chromium.org/424643002/ , a downstream CL you are
also on. Is this good practice or should we somehow merge / split apart the two
components of these CLs?

[jochen (gone - plz use gerrit), 2014-07-29 11:45:48]

On 2014/07/29 at 02:03:22, gavinp wrote:
> +jochen as you have more context
> 
> -abarth (please feel free to drive by!!)
> 
> Jochen,
> 
> This introduces public API changes which actually are going to be immediately
changed by https://codereview.chromium.org/424643002/ , a downstream CL you are
also on. Is this good practice or should we somehow merge / split apart the two
components of these CLs?

uh, hum.

yeah, adding an API to immediately rename it sounds like a bad idea.

can you somehow consolidate all those changes?

[gavinp, 2014-07-30 01:36:38]

On 2014/07/29 11:45:48, jochen wrote:
> On 2014/07/29 at 02:03:22, gavinp wrote:
> > +jochen as you have more context
> > 
> > -abarth (please feel free to drive by!!)
> > 
> > Jochen,
> > 
> > This introduces public API changes which actually are going to be
immediately
> changed by https://codereview.chromium.org/424643002/ , a downstream CL you
are
> also on. Is this good practice or should we somehow merge / split apart the
two
> components of these CLs?
> 
> uh, hum.
> 
> yeah, adding an API to immediately rename it sounds like a bad idea.
> 
> can you somehow consolidate all those changes?

Sure. The downstream rename also includes API updates that I didn't want to put
in the same CL, but I guess I'll make the monster.

[gavinp, 2014-07-30 01:41:43]

On 2014/07/30 01:36:38, gavinp wrote:
> On 2014/07/29 11:45:48, jochen wrote:
> > On 2014/07/29 at 02:03:22, gavinp wrote:
> > > +jochen as you have more context
> > > 
> > > -abarth (please feel free to drive by!!)
> > > 
> > > Jochen,
> > > 
> > > This introduces public API changes which actually are going to be
> immediately
> > changed by https://codereview.chromium.org/424643002/ , a downstream CL you
> are
> > also on. Is this good practice or should we somehow merge / split apart the
> two
> > components of these CLs?
> > 
> > uh, hum.
> > 
> > yeah, adding an API to immediately rename it sounds like a bad idea.
> > 
> > can you somehow consolidate all those changes?
> 
> Sure. The downstream rename also includes API updates that I didn't want to
put
> in the same CL, but I guess I'll make the monster.

I'm closing this issue in advance of uploading the monster.