Add more plumbing for FetchEvent including WebServiceWorkerResponse

Add more plumbing for FetchEvent including WebServiceWorkerResponse

This enables the browser to dispatch a FetchEvent
and receive a response.

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

[falken, 2014-03-03 06:00:04]

PTAL

The corresponding Chromium change, which depends on this, is
https://codereview.chromium.org/178343011/

[kinuko, 2014-03-03 13:05:36]

lgtm

https://codereview.chromium.org/185423005/diff/1/public/web/WebServiceWorkerR...
File public/web/WebServiceWorkerResponse.h (right):

https://codereview.chromium.org/185423005/diff/1/public/web/WebServiceWorkerR...
public/web/WebServiceWorkerResponse.h:14: #endif
nit: you don't need this when you include WebPrivatePtr.h

https://codereview.chromium.org/185423005/diff/1/public/web/WebServiceWorkerR...
public/web/WebServiceWorkerResponse.h:42: #endif
No need to surround this with BLINK_IMPLEMENTATION

[kinuko, 2014-03-03 13:06:13]

(+cc serviceworker-reviews)

[alecflett, 2014-03-03 18:27:36]

The more I see classes like WebServiceWorkerResponse, I wonder if we can avoid
having the extra WebCore::Response object and instead just deal with
WebServiceWorkerResponse instances directly (and have them manage their own
members?) - this is another thing we did in IndexeDB that made maintenance much
easier.

I know for some APIs it's useful to have the extra layer of indirection to deal
with WebKitAPI churn, but this one just seems like a bag-of-properties that we
can add and remove from any time - by removing WebCore::Response we can have
fewer/simpler 3-sided patches to expose a new property. (And simply less code!)

[jsbell, 2014-03-03 20:15:35]

On 2014/03/03 18:27:36, alecflett wrote:
> The more I see classes like WebServiceWorkerResponse, I wonder if we can avoid
> having the extra WebCore::Response object and instead just deal with
> WebServiceWorkerResponse instances directly (and have them manage their own
> members?) - this is another thing we did in IndexeDB that made maintenance
much
> easier.

+1

Might be worth asking aklein for opinions on this, as he's been doing some
flattening in this area.

[kinuko, 2014-03-04 00:16:41]

On 2014/03/03 20:15:35, jsbell wrote:
> On 2014/03/03 18:27:36, alecflett wrote:
> > The more I see classes like WebServiceWorkerResponse, I wonder if we can
avoid
> > having the extra WebCore::Response object and instead just deal with
> > WebServiceWorkerResponse instances directly (and have them manage their own
> > members?) - this is another thing we did in IndexeDB that made maintenance
> much
> > easier.
> 
> +1
> 
> Might be worth asking aklein for opinions on this, as he's been doing some
> flattening in this area.

Sounds good. Do you have some pointers where we can look into in IDB code? Will
the Web* class directly implement .idl then (with implementedAs?)?

[jsbell, 2014-03-04 00:22:00]

On 2014/03/04 00:16:41, kinuko wrote:
> Sounds good. Do you have some pointers where we can look into in IDB code?
Will
> the Web* class directly implement .idl then (with implementedAs?)?

For IDB it was the other way around. Initially we had public/platform/WebIDB*.h
interfaces and Source/web/WebIDB*Impl.h/.cpp implementations which just deferred
to Source/modules/indexeddb/IDB*.h/.cpp. Following the Blink fork we could have
Source/modules depend directly on public/platform, so we made the classes in
modules/indexeddb implement the public/platform interfaces directly and deleted
the intermediate Source/web/WebIDB* layer.

[alecflett, 2014-03-04 00:23:25]

On 2014/03/04 00:16:41, kinuko wrote:
> On 2014/03/03 20:15:35, jsbell wrote:
> > On 2014/03/03 18:27:36, alecflett wrote:
> > > The more I see classes like WebServiceWorkerResponse, I wonder if we can
> avoid
> > > having the extra WebCore::Response object and instead just deal with
> > > WebServiceWorkerResponse instances directly (and have them manage their
own
> > > members?) - this is another thing we did in IndexeDB that made maintenance
> > much
> > > easier.
> > 
> > +1
> > 
> > Might be worth asking aklein for opinions on this, as he's been doing some
> > flattening in this area.
> 
> Sounds good. Do you have some pointers where we can look into in IDB code?
Will
> the Web* class directly implement .idl then (with implementedAs?)?

I forgot how much of this was exposed to IDL - and  was about to say "yes" but
then I remembered that the IDL stuff is expected to be refcounted, while the
Web* generally isn't.. I mean I guess as long as Blink owns this, it's not
terrible. 

One advantage that the IDB stuff has is that the objects exposed to script (like
IDBObjectStore) are in fact different types of objects (objects that
synchronously return requests) than the objects used in the backend
(WebIDBTransaction, etc ])  and are objects with asynchronous calls that start
operations in the backend, and callback through the Web* interfaces as well.

So probably nothing we can do in the short term, but we can think about it as we
go forward.

[falken, 2014-03-04 03:20:18]

Upon discussing with kinuko@ and nhiroki@, it sounds like it'd be better to move
WebServiceWorkerResponse to public/platform from public/web so it can be used
more widely (e.g., by the Cache including for testing). So I'll revise the patch
to do that.

[falken, 2014-03-04 10:44:24]

Moved to public/platform. PTAL.

[kinuko, 2014-03-04 12:45:09]

lgtm

https://codereview.chromium.org/185423005/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/Response.h (right):

https://codereview.chromium.org/185423005/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/Response.h:37: void
getWebServiceWorkerResponse(blink::WebServiceWorkerResponse&);
nit: get? populate?

[falken, 2014-03-04 14:27:20]

kinuko: Thanks!
tkent: Can you please review as public/ and web/ OWNER?

https://codereview.chromium.org/185423005/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/Response.h (right):

https://codereview.chromium.org/185423005/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/Response.h:37: void
getWebServiceWorkerResponse(blink::WebServiceWorkerResponse&);
On 2014/03/04 12:45:09, kinuko wrote:
> nit: get? populate?

Agreed, will change to populate.

[tkent, 2014-03-04 23:50:55]

lgtm

[tkent, 2014-03-04 23:51:01]

The CQ bit was unchecked by tkent@chromium.org

[commit-bot: I haz the power, 2014-03-04 23:51:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/185423005/60001

[falken, 2014-03-05 00:39:32]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-03-05 00:43:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/185423005/80001

[commit-bot: I haz the power, 2014-03-05 20:55:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/185423005/80001

[commit-bot: I haz the power, 2014-03-06 03:39:55]

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

[commit-bot: I haz the power, 2014-03-06 03:39:55]

Try jobs failed on following builders: win_blink_compile

[kinuko, 2014-03-06 14:29:43]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-03-06 14:30:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/185423005/110001

[falken, 2014-03-06 15:08:25]

It seems the win compile failure was due to BLINK_EXPORT instead of
BLINK_PLATFORM_EXPORT.

tkent: Can you please check the most recent
public/platform/WebServiceWorkerResponse.h? (view the delta from patchset 6 to
see the change). TBR as it's a simple build fix and the cq bit was already
checked...

[commit-bot: I haz the power, 2014-03-06 15:20:32]

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

[commit-bot: I haz the power, 2014-03-06 15:20:33]

Try jobs failed on following builders: win_blink_rel

[falken, 2014-03-06 15:47:12]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-03-06 15:47:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/185423005/110001

[commit-bot: I haz the power, 2014-03-06 15:59:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/185423005/110001

[commit-bot: I haz the power, 2014-03-06 16:31:40]

Change committed as 168646