Introducing the ServiceWorker Cache object.

Introducing the ServiceWorker Cache object.

This object is the proxy for the ServiceWorker cache; it's created
entirely through the CacheStorage interface.

R=falken@chromium.org,jkarlin@chromium.org
BUG=399178
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179871

[gavinp, 2014-07-31 05:47:33]

This CL is downstream of https://codereview.chromium.org/424643002/ . 

yhirano: PTAL at V8Binding.h
falken: PTAL at everything!
haraken, jkarlin: FYI.

**** About V8Binding.h:

The V8Binding.h change was interesting. I decided against using a [Custom]
binding here, but the IDL compilers mechanism for handling variadic IDL required
an instantiation of NativeValueTraits<>.

I'm a bit upset about the testing situation. I could add a usage of (any...) to
TestObject.idl, but that wouldn't test anything; I didn't change the code that's
output in this CL, and TestObject.cpp is never compiled in our build. So...
that's not going to work.

Should I add something to V8BindingTest.cpp? I couldn't see tests for any of the
rest of this interface.

I'd love advice.


**** About the rest:

Just another empty implementation waiting for its downstream public interface
CL. Can't run tries on it because the upstream CL is still waiting for review.

[haraken, 2014-07-31 08:03:21]

The change to bindings/ looks good.

> Should I add something to V8BindingTest.cpp? I couldn't see tests for any of
the rest of this interface.

The reason why we don't have such tests in V8BindingTest.cpp is just because
V8BindingTest.cpp is invented very recently. If you can add a test to
V8BindingTest.cpp, that would be great.

[gavinp, 2014-07-31 08:32:38]

On 2014/07/31 08:03:21, haraken wrote:
> The change to bindings/ looks good.
> 
> > Should I add something to V8BindingTest.cpp? I couldn't see tests for any of
> the rest of this interface.
> 
> The reason why we don't have such tests in V8BindingTest.cpp is just because
> V8BindingTest.cpp is invented very recently. If you can add a test to
> V8BindingTest.cpp, that would be great.

Definitely. I'll take the V8Binding changes and move them in to a separate CL
for you and Hirano-san to review. While I'm there I'll add a bit more testing
around these types.

Do you think we want any no-compile tests as well? For template expansions it
can occasionally be helpful to verify NON-expansions.

[haraken, 2014-07-31 08:38:45]

On 2014/07/31 08:32:38, gavinp wrote:
> On 2014/07/31 08:03:21, haraken wrote:
> > The change to bindings/ looks good.
> > 
> > > Should I add something to V8BindingTest.cpp? I couldn't see tests for any
of
> > the rest of this interface.
> > 
> > The reason why we don't have such tests in V8BindingTest.cpp is just because
> > V8BindingTest.cpp is invented very recently. If you can add a test to
> > V8BindingTest.cpp, that would be great.
> 
> Definitely. I'll take the V8Binding changes and move them in to a separate CL
> for you and Hirano-san to review. While I'm there I'll add a bit more testing
> around these types.
> 
> Do you think we want any no-compile tests as well? For template expansions it
> can occasionally be helpful to verify NON-expansions.

Yes, having a good coverage in TestObject.idl is a good thing.

[falken, 2014-07-31 15:28:22]

FYI it looks like the name is back to "Cache":
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#ca...

Discussion on https://github.com/slightlyoff/ServiceWorker/issues/301

https://codereview.chromium.org/434543002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/FetchStore.h (right):

https://codereview.chromium.org/434543002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchStore.h:25: // See
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#cache
This comment is better in the IDL file.

https://codereview.chromium.org/434543002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/FetchStore.idl (right):

https://codereview.chromium.org/434543002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchStore.idl:39: // custom binding here. See
http://crbug.com/240176
The comment says we use a custom binding, but it looks like we're not? It looks
the same as the other methods.

https://codereview.chromium.org/434543002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchStore.idl:41: [CallWith=ScriptState] Promise
add(any... requests);
The spec has add and addAll. Why are we different?

https://codereview.chromium.org/434543002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchStore.idl:55: [CallWith=ScriptState] Promise
batch(sequence<Dictionary> batchOperations);
batch doesn't seem in the spec, what's up?

[gavinp, 2014-08-05 06:58:30]

The name change is an issue; I'm going to upload a new CL that combines a few in
progress ones, rather than do a name change in a series of uncommitted CLs.

[gavinp, 2014-08-06 00:55:38]

I didn't end up joining any CLs, so here we have this object with the name
change. I'll continue updating as I'm getting crashes on basic tests.

[gavinp, 2014-08-06 02:00:05]

-haraken -yhirano, the bindings change is gone.

falken: PTAL. This is now on an up to date version of the spec. It's been
changing... quickly.

https://codereview.chromium.org/434543002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/FetchStore.idl (right):

https://codereview.chromium.org/434543002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchStore.idl:39: // custom binding here. See
http://crbug.com/240176
On 2014/07/31 15:28:21, falken wrote:
> The comment says we use a custom binding, but it looks like we're not? It
looks
> the same as the other methods.

It's not quite custom, just generic. The use "any..." required upstream changes
to V8Binding.h, you can see those in https://codereview.chromium.org/440243003/
, but, because the API keeps changing, we no longer need "any..." arguments, so
that upstream CL is now closed.

https://codereview.chromium.org/434543002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchStore.idl:41: [CallWith=ScriptState] Promise
add(any... requests);
On 2014/07/31 15:28:21, falken wrote:
> The spec has add and addAll. Why are we different?

Because the spec has been changing many times per day, and I wrote this IDL at
the beginning of writing this CL. I'll update to the most recent spec.

https://codereview.chromium.org/434543002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchStore.idl:55: [CallWith=ScriptState] Promise
batch(sequence<Dictionary> batchOperations);
On 2014/07/31 15:28:21, falken wrote:
> batch doesn't seem in the spec, what's up?

Ditto

[falken, 2014-08-06 04:46:28]

lgtm with nits.

By the way, it turns out that the API OWNERs review for IDL file changes is not
required for unshipped features. So an API OWNER review is optional for this
patch.

https://codereview.chromium.org/434543002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/FetchStore.idl (right):

https://codereview.chromium.org/434543002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchStore.idl:39: // custom binding here. See
http://crbug.com/240176
On 2014/08/06 02:00:05, gavinp wrote:
> On 2014/07/31 15:28:21, falken wrote:
> > The comment says we use a custom binding, but it looks like we're not? It
> looks
> > the same as the other methods.
> 
> It's not quite custom, just generic. The use "any..." required upstream
changes
> to V8Binding.h, you can see those in
https://codereview.chromium.org/440243003/
> , but, because the API keeps changing, we no longer need "any..." arguments,
so
> that upstream CL is now closed.

Ah I see. "custom binding" made me think of Blink IDL's [Custom] attribute.

https://codereview.chromium.org/434543002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/Cache.cpp (right):

https://codereview.chromium.org/434543002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Cache.cpp:15: ScriptPromise
RejectAsNotImplemented(ScriptState* scriptState)
blink function names start with lowercase

https://codereview.chromium.org/434543002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/Cache.h (right):

https://codereview.chromium.org/434543002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Cache.h:9: #include
"bindings/core/v8/ScriptState.h"
Since ScriptState is forward declared below, don't need this?

https://codereview.chromium.org/434543002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Cache.h:26: // See
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#cache
Please move this comment to the .idl file. Our other spec links are in the .idl
file (except for CacheStorage.h).

[gavinp, 2014-08-08 19:55:26]

Thank you for your review Matt!

https://codereview.chromium.org/434543002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/Cache.cpp (right):

https://codereview.chromium.org/434543002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Cache.cpp:15: ScriptPromise
RejectAsNotImplemented(ScriptState* scriptState)
On 2014/08/06 04:46:28, falken wrote:
> blink function names start with lowercase

Done.

https://codereview.chromium.org/434543002/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/Cache.h (right):

https://codereview.chromium.org/434543002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Cache.h:9: #include
"bindings/core/v8/ScriptState.h"
On 2014/08/06 04:46:28, falken wrote:
> Since ScriptState is forward declared below, don't need this?

Done.

https://codereview.chromium.org/434543002/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/Cache.h:26: // See
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#cache
On 2014/08/06 04:46:28, falken wrote:
> Please move this comment to the .idl file. Our other spec links are in the
.idl
> file (except for CacheStorage.h).

Ahh, good point. I'll also upload a style CL to move that comment in
CacheStorage.h, along with a few other nits I've run across but not bundled in
to any other CL. Done.

[gavinp, 2014-08-08 19:55:30]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-08-08 19:56:48]

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

[commit-bot: I haz the power, 2014-08-08 19:57:04]

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

[commit-bot: I haz the power, 2014-08-08 19:57:06]

Failed to apply patch for Source/modules/serviceworkers/CacheStorage.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/serviceworkers/CacheStorage.cpp
  Hunk #3 FAILED at 98.
  1 out of 3 hunks FAILED -- saving rejects to file
Source/modules/serviceworkers/CacheStorage.cpp.rej

Patch:       Source/modules/serviceworkers/CacheStorage.cpp
Index: Source/modules/serviceworkers/CacheStorage.cpp
diff --git a/Source/modules/serviceworkers/CacheStorage.cpp
b/Source/modules/serviceworkers/CacheStorage.cpp
index
8f30d9936e717acb6ef3b0f387dc9d8341534777..3604bb40ae611aee230d5982f18916191d25425a
100644
--- a/Source/modules/serviceworkers/CacheStorage.cpp
+++ b/Source/modules/serviceworkers/CacheStorage.cpp
@@ -7,6 +7,7 @@
 
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "bindings/core/v8/ScriptState.h"
+#include "modules/serviceworkers/Cache.h"
 #include "public/platform/WebServiceWorkerCacheError.h"
 #include "public/platform/WebServiceWorkerCacheStorage.h"
 
@@ -58,9 +59,7 @@ public:
 
     virtual void onSuccess(WebServiceWorkerCache* cache) OVERRIDE
     {
-        // FIXME: There should be a blink side of the Cache object
implementation here, rather than
-        // this nonsensical return.
-        m_resolver->resolve("succesfully returned a cache");
+        m_resolver->resolve(Cache::fromWebServiceWorkerCache(cache));
     }
 
     virtual void onError(WebServiceWorkerCacheError* reason) OVERRIDE
@@ -99,7 +98,7 @@ private:
 
 PassRefPtrWillBeRawPtr<CacheStorage>
CacheStorage::create(WebServiceWorkerCacheStorage* webCacheStorage)
 {
-   return adoptRefWillBeNoop(new CacheStorage(webCacheStorage));
+    return adoptRefWillBeNoop(new CacheStorage(webCacheStorage));
 }
 
 ScriptPromise CacheStorage::get(ScriptState* scriptState, const String&
cacheName)

[gavinp, 2014-08-08 20:09:56]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-08-08 20:10:26]

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

[commit-bot: I haz the power, 2014-08-08 21:47:40]

Change committed as 179871