Initial ServiceWorker CacheStorage API polyfill.

Initial ServiceWorker CacheStorage API polyfill.

This provides CacheStorage, the namespace of caches, although without
serialization. The idea is to let us test this API end-to-end early,
and we can replace it with something more sophisticated as we
progress.

R=jsbell@chromium.org,falken@chromium.org,jkarlin@chromium.org,asanka@chromium.org,slightlyoff@chromium.org,jakearchibald@chromium.org
TBR=jochen@chromium.org
BUG=374822
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175988

[gavinp, 2014-06-10 00:25:28]

Here's a very basic CacheStorage API as well, that installs the "caches" object
into the global context.

jsbell, falken, PTAL.

jkarlin, slightlyoff, asanka, jakearchibald: FYI.

[falken, 2014-06-10 17:47:03]

lgtm

https://codereview.chromium.org/329433002/diff/20001/Source/modules/servicewo...
File Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js (right):

https://codereview.chromium.org/329433002/diff/20001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:48:
Object.keys(this.cachesByName).map(function (key) {
I think your other patches removed the space after function, so let's be
consistent with that here and throughout this file.
(To answer your earlier question, there's no Blink JS style guide that I know
of, just http://dev.chromium.org/blink/coding-style)

[gavinp, 2014-06-10 17:51:08]

On 2014/06/10 17:47:03, falken wrote:
> lgtm
> 
>
https://codereview.chromium.org/329433002/diff/20001/Source/modules/servicewo...
> File Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js (right):
> 
>
https://codereview.chromium.org/329433002/diff/20001/Source/modules/servicewo...
> Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:48:
> Object.keys(this.cachesByName).map(function (key) {
> I think your other patches removed the space after function, so let's be
> consistent with that here and throughout this file.
> (To answer your earlier question, there's no Blink JS style guide that I know
> of, just http://dev.chromium.org/blink/coding-style)

Thanks. And, yeah, I will omit spaces after "function", thanks for the catch.

I'm thinking I'll actually narrow this down to my proposed API (which is much
narrower), and quickly start implementing the native API. So I think I'll
reupload and restart the review process on this.

[gavinp, 2014-06-10 18:01:37]

falken,

Here is a change that brings this to the API that I'd prefer. Does this still
LGTM?

I'll keep the conversation with standards and other involved folks alive over
this; I just think we should go forward with what's better for the
implementation for now.

[falken, 2014-06-10 18:36:12]

still lgtm as a first cut. I think it'd be good to start the spec discussion
ASAP on GitHub.

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
File Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js (right):

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:77: return
Promise.resolve(Object.keys(this.cachesByName).map(function (key) {
function space here

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:87:
this.get(cacheName).then(function (cache) {
here too

[jsbell, 2014-06-10 18:36:19]

Initial notes

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
File Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js (right):

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:17: return
Promise.resolve(this.cachesByName[key]);
Overall: this polyfill is doing all of the changes synchronously, then minting
promises.

This should be fine (and not observable), but once we make *any* part of this
async we'll want to make *all* methods async so that the microtasks are ordered
correctly. i.e. we'll want the `return new Promise(function(r,r) { ... do
everything here ... });` pattern, even though it's wordier.

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:26: return
Promise.reject(false);
Should resolve(false);

reject() should only be used when a synchronous API would raise an exception.

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:32: // FIXME:
Engage standarisation on adding this method to the spec.
typo: standar*d*isation

(Also, insert 's' vs. 'z' joke about UK vs. US English here.)

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:44:
this.cachesByName[toKey] = this.cachesByName[fromKey];
I assume it's intentional that no checking is done on cachesByName[toKey], i.e.
if it previously existed it is silently dropped?

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:92:
global.CacheStorage = global.CacheStorage || CacheStorage;
Intentional to expose constructor for this, in the simplified API case?

[gavinp, 2014-06-10 20:03:09]

Thanks for the reviews.

jsbell: how's this?

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
File Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js (right):

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:17: return
Promise.resolve(this.cachesByName[key]);
On 2014/06/10 18:36:19, jsbell wrote:
> Overall: this polyfill is doing all of the changes synchronously, then minting
> promises.
> 
> This should be fine (and not observable), but once we make *any* part of this
> async we'll want to make *all* methods async so that the microtasks are
ordered
> correctly. i.e. we'll want the `return new Promise(function(r,r) { ... do
> everything here ... });` pattern, even though it's wordier.

Aha, yes, good point. This way, when the user chains these operations using
.then(), ..., the operation is not executed until the correct time.

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:26: return
Promise.reject(false);
On 2014/06/10 18:36:19, jsbell wrote:
> Should resolve(false);
> 
> reject() should only be used when a synchronous API would raise an exception.

Done.

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:32: // FIXME:
Engage standarisation on adding this method to the spec.
On 2014/06/10 18:36:19, jsbell wrote:
> typo: standar*d*isation
> 
> (Also, insert 's' vs. 'z' joke about UK vs. US English here.)

Done. While I'm religious about US english in identifier names ("did that
function have colour or color?"), I'm not as good in comments.

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:44:
this.cachesByName[toKey] = this.cachesByName[fromKey];
On 2014/06/10 18:36:19, jsbell wrote:
> I assume it's intentional that no checking is done on cachesByName[toKey],
i.e.
> if it previously existed it is silently dropped?

Yes. Akin to doing "set" followed by "delete."

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:77: return
Promise.resolve(Object.keys(this.cachesByName).map(function (key) {
On 2014/06/10 18:36:12, falken wrote:
> function space here

Done.

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:87:
this.get(cacheName).then(function (cache) {
On 2014/06/10 18:36:12, falken wrote:
> here too

Done.

https://codereview.chromium.org/329433002/diff/90001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:92:
global.CacheStorage = global.CacheStorage || CacheStorage;
On 2014/06/10 18:36:19, jsbell wrote:
> Intentional to expose constructor for this, in the simplified API case?

Fair point, no real need. Done.

[jsbell, 2014-06-10 22:26:34]

Overall lgtm but some bugs and nits.

I can has tests next?

https://codereview.chromium.org/329433002/diff/120001/Source/modules/servicew...
File Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js (right):

https://codereview.chromium.org/329433002/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:20: }
Nit: missing ; after the function expression in the assignment statement (here
and all the other instances below)

https://codereview.chromium.org/329433002/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:70: return
Promise.resolve(this.cachesByName.keys());
`Object.prototype.keys` is not defined.

Do you mean `Object.keys(this.cachesByName)` ?

https://codereview.chromium.org/329433002/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:84:
this.get(cacheName).then(function(cache) {
Missing `return` at the start of this?

[gavinp, 2014-06-11 16:58:30]

You _can_ has tests. See crbug.com/374802 , I'm laying track and Asanka is
running a train along it.

https://codereview.chromium.org/329433002/diff/120001/Source/modules/servicew...
File Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js (right):

https://codereview.chromium.org/329433002/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:20: }
On 2014/06/10 22:26:34, jsbell wrote:
> Nit: missing ; after the function expression in the assignment statement (here
> and all the other instances below)

Done.

https://codereview.chromium.org/329433002/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:70: return
Promise.resolve(this.cachesByName.keys());
On 2014/06/10 22:26:34, jsbell wrote:
> `Object.prototype.keys` is not defined.
> 
> Do you mean `Object.keys(this.cachesByName)` ?

Done.

https://codereview.chromium.org/329433002/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/polyfills/cacheStoragePolyfill.js:84:
this.get(cacheName).then(function(cache) {
On 2014/06/10 22:26:33, jsbell wrote:
> Missing `return` at the start of this?

Done.

[gavinp, 2014-06-11 22:02:05]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-06-11 22:02:51]

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

[commit-bot: I haz the power, 2014-06-11 22:07:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7477)

[commit-bot: I haz the power, 2014-06-11 22:10:18]

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

[commit-bot: I haz the power, 2014-06-11 22:10:19]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/7478)

[gavinp, 2014-06-11 22:10:32]

+jochen as TBD for modules.gyp changes.

[gavinp, 2014-06-11 22:10:36]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-06-11 22:10:51]

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

[commit-bot: I haz the power, 2014-06-12 05:35:57]

Change committed as 175988