Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1)

Issue 433793002: Introducing the WebServiceWorkerCache. (Closed)

Created:
6 years, 4 months ago by gavinp
Modified:
6 years, 3 months ago
CC:
blink-reviews, michaeln, jsbell+serviceworker_chromium.org, jamesr, tzik, serviceworker-reviews, nhiroki, abarth-chromium, falken, dglazkov+blink, kinuko+serviceworker, horo+watch_chromium.org, alecflett+watch_chromium.org
Project:
blink
Visibility:
Public.

Description

Introducing the WebServiceWorkerCache. This CL provides the new WebServiceWorkerCache object, which connects the ServiceWorker Cache between blink and content. There's two notable exceptions in the implementation; the various Cache.add() and Cache.addAll() methods are not implemented (they should do fetches in blink and provide the responses to this API through the batch call), and Response bodies via blobs is not implemented. R=dominicc@chromium.org,falken@chromium.org,jkarlin@chromium.org BUG=399178 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182604

Patch Set 1 #

Patch Set 2 : closer #

Patch Set 3 : rename back to Cache, fill in most parts #

Patch Set 4 : closer #

Patch Set 5 : closer and narrower #

Patch Set 6 : fix running with partial implementation #

Total comments: 42

Patch Set 7 : update for content side #

Patch Set 8 : remediate #

Total comments: 6

Patch Set 9 : partial remediation #

Total comments: 27

Patch Set 10 : rebase #

Patch Set 11 : rebase #

Patch Set 12 : closer #

Total comments: 10

Patch Set 13 : rebase, some changes pulled out #

Total comments: 2

Patch Set 14 : rebase on top up new upstream CLs #

Patch Set 15 : fix build #

Patch Set 16 : add cacheName query param #

Patch Set 17 : test isn't quite working #

Total comments: 3

Patch Set 18 : closer #

Patch Set 19 : rebasing and fixing build #

Patch Set 20 : for stitching use #

Patch Set 21 : reviewable #

Patch Set 22 : fix build #

Total comments: 22

Patch Set 23 : rebase only #

Patch Set 24 : style remediation #

Patch Set 25 : rebase on top of style nits cl #

Total comments: 1

Patch Set 26 : rebase, including jsbell's oilpan fixes #

Patch Set 27 : improve and pass tests #

Total comments: 15

Patch Set 28 : remediate to dominicc review #

Total comments: 18

Patch Set 29 : remediate #

Patch Set 30 : rebase to trunk #

Patch Set 31 : narrow #

Patch Set 32 : one last review comment #

Patch Set 33 : remove crazy proxy thing #

Patch Set 34 : rebase on top of NameToCacheMap impl #

Patch Set 35 : cleaner and oilpan #

Unified diffs Side-by-side diffs Delta from patch set Stats (+908 lines, -39 lines) Patch
M Source/modules/modules.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 1 chunk +1 line, -0 lines 0 comments Download
M Source/modules/serviceworkers/Cache.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 3 chunks +16 lines, -7 lines 0 comments Download
M Source/modules/serviceworkers/Cache.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 2 chunks +277 lines, -29 lines 0 comments Download
A Source/modules/serviceworkers/CacheTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +540 lines, -0 lines 0 comments Download
A public/platform/WebServiceWorkerCache.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +68 lines, -0 lines 0 comments Download
M public/platform/WebServiceWorkerCacheStorage.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 2 chunks +6 lines, -3 lines 0 comments Download

Messages

Total messages: 55 (3 generated)
gavinp
jkarlin, dominicc, falken: Please take a look over this for architecture; I'm not sending it ...
6 years, 4 months ago (2014-08-08 20:34:40 UTC) #1
gavinp
On 2014/08/08 20:34:40, gavinp wrote: > jkarlin, dominicc, falken: Please take a look over this ...
6 years, 4 months ago (2014-08-08 21:09:42 UTC) #2
dominicc (has gone to gerrit)
Comments inline. https://codereview.chromium.org/433793002/diff/100001/Source/modules/serviceworkers/Cache.cpp File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/serviceworkers/Cache.cpp#newcode122 Source/modules/serviceworkers/Cache.cpp:122: // This management mechanism insures that we ...
6 years, 4 months ago (2014-08-11 01:40:42 UTC) #3
gavinp
https://codereview.chromium.org/433793002/diff/100001/Source/modules/serviceworkers/Cache.cpp File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/serviceworkers/Cache.cpp#newcode122 Source/modules/serviceworkers/Cache.cpp:122: // This management mechanism insures that we get the ...
6 years, 4 months ago (2014-08-13 23:25:24 UTC) #4
dominicc (has gone to gerrit)
More comments inline. I just looked at your responses and the diffs to patchset 6, ...
6 years, 4 months ago (2014-08-14 02:35:13 UTC) #5
gavinp
https://codereview.chromium.org/433793002/diff/100001/Source/modules/serviceworkers/Cache.cpp File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/100001/Source/modules/serviceworkers/Cache.cpp#newcode140 Source/modules/serviceworkers/Cache.cpp:140: // FIXME: We should throw the caught error. On ...
6 years, 4 months ago (2014-08-14 22:34:07 UTC) #6
dominicc (has gone to gerrit)
More comments inline. Going to look at the Chrome side next. I think once you ...
6 years, 4 months ago (2014-08-15 06:20:24 UTC) #7
gavinp
On 2014/08/15 06:20:24, dominicc wrote: > More comments inline. > > Going to look at ...
6 years, 4 months ago (2014-08-19 20:12:38 UTC) #8
gavinp
This should be a lot closer. **IMPORTANT**: I have no idea how to create a ...
6 years, 4 months ago (2014-08-19 21:09:37 UTC) #9
jsbell
On 2014/08/19 21:09:37, gavinp wrote: > This should be a lot closer. > > **IMPORTANT**: ...
6 years, 4 months ago (2014-08-20 00:13:52 UTC) #10
jsbell
https://codereview.chromium.org/433793002/diff/160001/Source/modules/serviceworkers/Response.cpp File Source/modules/serviceworkers/Response.cpp (right): https://codereview.chromium.org/433793002/diff/160001/Source/modules/serviceworkers/Response.cpp#newcode36 Source/modules/serviceworkers/Response.cpp:36: TrackExceptionState exceptionState; On 2014/08/19 21:09:37, gavinp wrote: > On ...
6 years, 4 months ago (2014-08-20 00:22:04 UTC) #11
gavinp
On 2014/08/20 00:13:52, jsbell wrote: > On 2014/08/19 21:09:37, gavinp wrote: > > This should ...
6 years, 4 months ago (2014-08-20 13:50:34 UTC) #12
dominicc (has gone to gerrit)
I think this patch is unnecessarily large and could be broken up. The size makes ...
6 years, 4 months ago (2014-08-21 06:37:56 UTC) #13
gavinp
I'm proceeding to carve this up as much as possible; a few upstream changes have ...
6 years, 3 months ago (2014-08-28 13:39:36 UTC) #14
jsbell
https://codereview.chromium.org/433793002/diff/260001/Source/modules/serviceworkers/Cache.cpp File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/260001/Source/modules/serviceworkers/Cache.cpp#newcode144 Source/modules/serviceworkers/Cache.cpp:144: // return ScriptPromise::reject(DOMException::create(NotFoundError, "The specified Service Worker resource was ...
6 years, 3 months ago (2014-08-28 23:48:45 UTC) #15
jsbell
On 2014/08/28 23:48:45, jsbell wrote: > The bindings code automagically converts throws exceptions to rejections ...
6 years, 3 months ago (2014-08-29 00:44:53 UTC) #16
jsbell
On 2014/08/29 00:44:53, jsbell wrote: > .... but apparently this isn't working as I expected, ...
6 years, 3 months ago (2014-08-29 00:53:42 UTC) #17
gavinp
On 2014/08/29 00:53:42, jsbell wrote: > On 2014/08/29 00:44:53, jsbell wrote: > > .... but ...
6 years, 3 months ago (2014-08-29 16:27:48 UTC) #18
gavinp
jsbell: i've cleaned this up quite a bit, but my last upload "test isn't quite ...
6 years, 3 months ago (2014-08-29 20:03:26 UTC) #19
jsbell
https://codereview.chromium.org/433793002/diff/360001/Source/modules/serviceworkers/CacheTest.cpp File Source/modules/serviceworkers/CacheTest.cpp (right): https://codereview.chromium.org/433793002/diff/360001/Source/modules/serviceworkers/CacheTest.cpp#newcode85 Source/modules/serviceworkers/CacheTest.cpp:85: ScriptPromise blah = cache->match(scriptState(), url, Dictionary()); On 2014/08/29 20:03:26, ...
6 years, 3 months ago (2014-08-29 21:26:43 UTC) #20
gavinp
https://codereview.chromium.org/433793002/diff/360001/Source/modules/serviceworkers/CacheTest.cpp File Source/modules/serviceworkers/CacheTest.cpp (right): https://codereview.chromium.org/433793002/diff/360001/Source/modules/serviceworkers/CacheTest.cpp#newcode85 Source/modules/serviceworkers/CacheTest.cpp:85: ScriptPromise blah = cache->match(scriptState(), url, Dictionary()); On 2014/08/29 21:26:43, ...
6 years, 3 months ago (2014-09-02 14:42:48 UTC) #21
gavinp
This latest upload should be much closer to what we finally want; I've carved out ...
6 years, 3 months ago (2014-09-03 16:44:24 UTC) #22
jsbell
style nits https://codereview.chromium.org/433793002/diff/460001/Source/modules/serviceworkers/Cache.cpp File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/460001/Source/modules/serviceworkers/Cache.cpp#newcode39 Source/modules/serviceworkers/Cache.cpp:39: CacheMatchCallbacks(PassRefPtr<ScriptPromiseResolver> resolver) : m_resolver(resolver) { } style: ...
6 years, 3 months ago (2014-09-03 19:15:01 UTC) #23
jsbell
Few more nits. I've been distracted with meetings, so I can't page enough into my ...
6 years, 3 months ago (2014-09-04 00:28:08 UTC) #24
gavinp
TY for your style nits Josh. https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServiceWorkerCache.h File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/100001/public/platform/WebServiceWorkerCache.h#newcode29 public/platform/WebServiceWorkerCache.h:29: class ProxyInterface { ...
6 years, 3 months ago (2014-09-04 19:47:34 UTC) #25
jsbell
This will need significant rebasing due to (1) Oilpan being turned on for modules/serviceworker and ...
6 years, 3 months ago (2014-09-05 22:48:14 UTC) #26
jsbell
Also, the Cache needs to be GarbageCollectedFinalized since it has an OwnPtr member. Which results ...
6 years, 3 months ago (2014-09-05 23:04:50 UTC) #27
jsbell
Yeah, the assumption (on the chromium side) that ServiceWorkerCacheStorageDispatcher will outlive WebCache objects is now ...
6 years, 3 months ago (2014-09-05 23:15:32 UTC) #28
jsbell
On 2014/09/05 23:15:32, jsbell wrote: > Yeah, the assumption (on the chromium side) that > ...
6 years, 3 months ago (2014-09-05 23:34:53 UTC) #29
gavinp
On 2014/09/05 23:34:53, jsbell wrote: > On 2014/09/05 23:15:32, jsbell wrote: > > Yeah, the ...
6 years, 3 months ago (2014-09-16 21:46:46 UTC) #30
jsbell
> Folks, can you please take a look at this? Looking. FYI when running asanka's ...
6 years, 3 months ago (2014-09-17 00:02:38 UTC) #31
jsbell
On 2014/09/17 00:02:38, jsbell wrote: > > Folks, can you please take a look at ...
6 years, 3 months ago (2014-09-17 00:17:11 UTC) #32
dominicc (has gone to gerrit)
Some comments inline. https://codereview.chromium.org/433793002/diff/560001/Source/modules/serviceworkers/Cache.cpp File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/560001/Source/modules/serviceworkers/Cache.cpp#newcode130 Source/modules/serviceworkers/Cache.cpp:130: Request* request = Request::create(scriptState->executionContext(), originalRequest, exceptionState); ...
6 years, 3 months ago (2014-09-17 12:53:58 UTC) #33
gavinp
On 2014/09/17 00:17:11, jsbell wrote: > On 2014/09/17 00:02:38, jsbell wrote: > > > Folks, ...
6 years, 3 months ago (2014-09-17 14:36:39 UTC) #34
gavinp
TY everyone for your close looks. This is much cleaner now. https://codereview.chromium.org/433793002/diff/560001/Source/modules/serviceworkers/Cache.cpp File Source/modules/serviceworkers/Cache.cpp (right): ...
6 years, 3 months ago (2014-09-17 15:41:53 UTC) #35
jsbell
lgtm but I think we should do some refactoring after landing to bring this more ...
6 years, 3 months ago (2014-09-18 21:45:00 UTC) #37
tkent
https://codereview.chromium.org/433793002/diff/580001/Source/modules/serviceworkers/Cache.cpp File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/580001/Source/modules/serviceworkers/Cache.cpp#newcode91 Source/modules/serviceworkers/Cache.cpp:91: Vector<Request*> requests; This should be HeapVector<Member<Request>>. https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServiceWorkerCache.h File public/platform/WebServiceWorkerCache.h ...
6 years, 3 months ago (2014-09-18 23:35:56 UTC) #38
jsbell
https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServiceWorkerCache.h File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServiceWorkerCache.h#newcode29 public/platform/WebServiceWorkerCache.h:29: // Blink can store its internal interface to this ...
6 years, 3 months ago (2014-09-19 16:22:47 UTC) #39
jsbell
I think if we just renamed 'ProxyInterface' -> 'ScriptProxy' or even just 'Proxy' it'd clearer ...
6 years, 3 months ago (2014-09-19 16:35:35 UTC) #40
gavinp
On 2014/09/19 16:35:35, jsbell wrote: > I think if we just renamed 'ProxyInterface' -> 'ScriptProxy' ...
6 years, 3 months ago (2014-09-19 20:20:17 UTC) #41
gavinp
Thanks for the reviews. jsbell: your suggestions about followups are well on point. However, this ...
6 years, 3 months ago (2014-09-22 16:05:24 UTC) #42
jamesr
https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServiceWorkerCache.h File public/platform/WebServiceWorkerCache.h (right): https://codereview.chromium.org/433793002/diff/580001/public/platform/WebServiceWorkerCache.h#newcode33 public/platform/WebServiceWorkerCache.h:33: virtual inline ~ProxyInterface() = 0; On 2014/09/22 16:05:24, gavinp ...
6 years, 3 months ago (2014-09-22 16:36:03 UTC) #44
jsbell
https://codereview.chromium.org/433793002/diff/580001/Source/modules/serviceworkers/Cache.cpp File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/580001/Source/modules/serviceworkers/Cache.cpp#newcode91 Source/modules/serviceworkers/Cache.cpp:91: Vector<Request*> requests; On 2014/09/22 16:05:24, gavinp wrote: > On ...
6 years, 3 months ago (2014-09-22 16:42:50 UTC) #45
jsbell
https://codereview.chromium.org/433793002/diff/580001/Source/modules/serviceworkers/Cache.cpp File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/580001/Source/modules/serviceworkers/Cache.cpp#newcode91 Source/modules/serviceworkers/Cache.cpp:91: Vector<Request*> requests; On 2014/09/22 16:42:50, jsbell wrote: > On ...
6 years, 3 months ago (2014-09-22 16:45:34 UTC) #46
gavinp
The latest upload gets oilpan correct on some of the callbacks per tkent's review. As ...
6 years, 3 months ago (2014-09-23 15:42:25 UTC) #47
jsbell
Clarifying for drive-bys: > The downside is that we'll NEVER destruct caches. ... for the ...
6 years, 3 months ago (2014-09-23 17:29:40 UTC) #48
tkent
lgtm https://codereview.chromium.org/433793002/diff/580001/Source/modules/serviceworkers/Cache.cpp File Source/modules/serviceworkers/Cache.cpp (right): https://codereview.chromium.org/433793002/diff/580001/Source/modules/serviceworkers/Cache.cpp#newcode91 Source/modules/serviceworkers/Cache.cpp:91: Vector<Request*> requests; On 2014/09/22 16:45:34, jsbell wrote: > ...
6 years, 3 months ago (2014-09-23 23:56:26 UTC) #49
gavinp
Thanks for the reviews everyone. I'll land this later today.
6 years, 3 months ago (2014-09-24 13:59:39 UTC) #50
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/433793002/720001
6 years, 3 months ago (2014-09-24 14:49:05 UTC) #52
commit-bot: I haz the power
Committed patchset #35 (id:720001) as 182604
6 years, 3 months ago (2014-09-24 15:01:27 UTC) #53
gavinp
On 2014/09/24 15:01:27, I haz the power (commit-bot) wrote: > Committed patchset #35 (id:720001) as ...
6 years, 3 months ago (2014-09-24 15:16:07 UTC) #54
Jens Widell
6 years, 3 months ago (2014-09-24 15:35:48 UTC) #55
Message was sent while issue was closed.
On 2014/09/24 15:16:07, gavinp wrote:
> On 2014/09/24 15:01:27, I haz the power (commit-bot) wrote:
> > Committed patchset #35 (id:720001) as 182604
> 
> This is causing a build failure in
> http://build.chromium.org/p/chromium.webkit/builders/Linux%20GN/builds/6752 ,
so
> I'm baking up a revert.

This conflicted with my https://codereview.chromium.org/555133003/ which landed
around the same time as this. It changed the signature of toImplArray<>() in
V8Binding.h, called in CacheTest.cpp here.

See
https://codereview.chromium.org/555133003/diff/200001/Source/bindings/core/v8...
for how to fix the issue.

Powered by Google App Engine
This is Rietveld 408576698