[ServiceWorkerCacheStorage] Blink changes to remove create/get and add open function

[ServiceWorkerCacheStorage] Blink changes to remove create/get and add open function

As per recent spec changes, remove create and get and replace it with open.

CLs in this three-legged-patch:

1. Add open to browser - https://codereview.chromium.org/664433003/
*2. Add open (remove get/create) to Blink - https://codereview.chromium.org/638023003
3. Remove get/create from browser - https://codereview.chromium.org/663503002/

BUG=423942
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183895

[jkarlin, 2014-10-16 14:10:43]

Patchset #1 (id:1) has been deleted

[jkarlin, 2014-10-16 15:27:44]

jkarlin@chromium.org changed reviewers:
+ falken@chromium.org, horo@chromium.org

[jkarlin, 2014-10-16 15:27:45]

horo@chromium.org: Please review changes in all

falken@chromium.org: Please review changes in all

Thanks!

[jkarlin, 2014-10-16 15:28:42]

+cc gavinp: FYI

[jsbell, 2014-10-16 20:30:11]

jsbell@chromium.org changed reviewers:
+ jamesr@chromium.org, jsbell@chromium.org

[jsbell, 2014-10-16 20:30:11]

jsbell@chromium.org changed required reviewers:
+ jamesr@chromium.org

[jsbell, 2014-10-16 20:30:12]

lgtm
You'll need a public/platform OWNERS review - jamesr@ ?

[jamesr, 2014-10-16 20:32:20]

On 2014/10/16 20:30:12, jsbell wrote:
> lgtm
> You'll need a public/platform OWNERS review - jamesr@ ?

I'm not a public/platform OWNER

[jamesr, 2014-10-16 20:32:33]

jamesr@chromium.org changed reviewers:
- jamesr@chromium.org

[jamesr, 2014-10-16 20:32:33]

jamesr@chromium.org changed required reviewers:
- jamesr@chromium.org

[jsbell, 2014-10-16 20:44:37]

jsbell@chromium.org changed reviewers:
+ tkent@chromium.org

[jsbell, 2014-10-16 20:44:37]

jsbell@chromium.org changed required reviewers:
+ tkent@chromium.org

[jsbell, 2014-10-16 20:44:38]

Whoops (sorry jamesr)

tkent@ - OWNERS review for public/platform change?

[tkent, 2014-10-17 00:39:21]

lgtm

[horo, 2014-10-17 00:54:32]

lgtm

[falken, 2014-10-17 01:54:27]

lgtm but see comments

https://codereview.chromium.org/638023003/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js
(right):

https://codereview.chromium.org/638023003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:18: //
origin that also uses an empty cache name.
(although this CL doesn't introduce it) This is a bit frightening, would moving
this test into its own file solve the problem? Collisions are hard to debug.

https://codereview.chromium.org/638023003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:26:
'CacheStorage.open should accept an empty name.');
Do you understand why service_worker.js and service_worker.ts throw an error on
the empty string, yet the spec doesn't seem to mention it? 

https://github.com/slightlyoff/ServiceWorker/blob/db3b3598c2735cf31465dc62bc6...
https://github.com/slightlyoff/ServiceWorker/blob/db3b3598c2735cf31465dc62bc6...

See also jsbell's comment on the commit:
https://github.com/slightlyoff/ServiceWorker/commit/db3b3598c2735cf31465dc62b...

[jkarlin, 2014-10-17 02:05:49]

https://codereview.chromium.org/638023003/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js
(right):

https://codereview.chromium.org/638023003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:18: //
origin that also uses an empty cache name.
On 2014/10/17 01:54:27, falken wrote:
> (although this CL doesn't introduce it) This is a bit frightening, would
moving
> this test into its own file solve the problem? Collisions are hard to debug.

I'll talk to asanka@ about the race and how we might mitigate it.  The fix would
be for another CL though.

https://codereview.chromium.org/638023003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:26:
'CacheStorage.open should accept an empty name.');
On 2014/10/17 01:54:27, falken wrote:
> Do you understand why service_worker.js and service_worker.ts throw an error
on
> the empty string, yet the spec doesn't seem to mention it? 
> 
>
https://github.com/slightlyoff/ServiceWorker/blob/db3b3598c2735cf31465dc62bc6...
>
https://github.com/slightlyoff/ServiceWorker/blob/db3b3598c2735cf31465dc62bc6...
> 
> See also jsbell's comment on the commit:
>
https://github.com/slightlyoff/ServiceWorker/commit/db3b3598c2735cf31465dc62b...

We have a bug on the issue where we discuss that exact commit:
https://code.google.com/p/chromium/issues/detail?id=408359

The argument is that empty keys are allowed in other apis, so should be in SW as
well and it's a mistake that service_worker.js errors on it.

[falken, 2014-10-17 02:10:01]

> I'll talk to asanka@ about the race and how we might mitigate it.  The fix
would
> be for another CL though.

sgtm 


> We have a bug on the issue where we discuss that exact commit:
> https://code.google.com/p/chromium/issues/detail?id=408359
> 

sgtm

[asanka, 2014-10-17 03:08:49]

https://codereview.chromium.org/638023003/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js
(right):

https://codereview.chromium.org/638023003/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/cache-storage-worker.js:18: //
origin that also uses an empty cache name.
On 2014/10/17 02:05:49, jkarlin wrote:
> On 2014/10/17 01:54:27, falken wrote:
> > (although this CL doesn't introduce it) This is a bit frightening, would
> moving
> > this test into its own file solve the problem? Collisions are hard to debug.
> 
> I'll talk to asanka@ about the race and how we might mitigate it.  The fix
would
> be for another CL though.

The collision is because:
* The test specifically requires an empty cache name.
* .caches is per origin.

Moving it to another file won't help unless we also move it to a different
origin. I let this one sit since this is the only test that was planning to use
an empty cache name.

Other tests were updated to use more test specific names to avoid collisions.

[jkarlin, 2014-10-17 09:24:53]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-10-17 09:25:00]

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

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

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

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

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

[jkarlin, 2014-10-17 12:39:01]

I had to change one create to open in the keys test.  Can someone please take a
quick look?  Thanks!

[jsbell, 2014-10-17 15:31:13]

still lgtm

[jkarlin, 2014-10-17 15:34:38]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-10-17 15:35:08]

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

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

Committed patchset #4 (id:70001) as 183895