Start to implement FetchEvent for ServiceWorker

Start to implement FetchEvent for ServiceWorker

Now a service worker can handle a fetch event and call event.respondWith
to pass a response back.

Currently dispatching the event and receiving the response is not hooked
up in Chromium side. The plan is to use the Response object from the
service worker's script to create a ServiceWorkerFetchResponse that
is sent in an IPC message back to the browser.

No test is added. It will be tested in ServiceWorkerVersionBrowserTest
once hooked up in Chrome (I've done this locally). Testing
Response in a layout test would be ideal but we currently don't have
a way to do a layout test in the service worker's context.

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

[falken, 2014-02-26 03:22:23]

kinuko@: Could you take a look please?

The corresponding Chrome-side change is:
https://codereview.chromium.org/178343011/

[kinuko, 2014-02-26 05:42:49]

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/RespondWithObserver.cpp (right):

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/RespondWithObserver.cpp:25: };
(these promise-waiter functions could be probably generalized a bit further..?)

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/RespondWithObserver.cpp:84: {
Shouldn't we check executionContext() is non-null here? (If not we wouldn't need
to inherit from LifecycleObserver)

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/RespondWithObserver.cpp:87: observeContext(0);
I don't think this is necessary

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/Response.cpp (right):

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/Response.cpp:32: return response;
I prefer defining ResponseInit in a separate header, like
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

so that we can handle default-values more gracefully, and ResponseInit
definition can be more isolated.

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

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/Response.h:18: static PassRefPtr<Response>
create(const Dictionary&);
nit: can you also write the param name (responseInit) if style-checker doesn't
warn, as 'Dictionary' doesn't give much info?

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

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/Response.idl:12: // FIXME: Spec uses ByteString
for these. We must perform the DOMString -> ByteString conversion manually,
We don't have ByteString support anywhere yet?

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/Response.idl:17: // FIXME: Spec will eventually
define "HeaderMap" interface.
I think HeaderMap just inherits MapClass, which is in WebIDL (therefore the
nicer way to handle this is support it in binding layer)

https://codereview.chromium.org/180843003/diff/1/public/web/WebServiceWorkerF...
File public/web/WebServiceWorkerFetchResponse.h (right):

https://codereview.chromium.org/180843003/diff/1/public/web/WebServiceWorkerF...
public/web/WebServiceWorkerFetchResponse.h:21: class
WebServiceWorkerFetchResponse {
It's a bit questionable if we want to share this interface /w other APIs that
rely on Response object, e.g. Cache and Fetch(). If so probably we'd better
consider which layer this should be in (public/web or public/platform), if this
interface should have 'fetch' in its name, if the type Fallback is appropriate
here etc.

[falken, 2014-02-27 06:45:09]

Thanks! PTAL. I decided to take your original suggestion and move the web/ stuff
to different patch for easier reviewing.

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/RespondWithObserver.cpp (right):

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/RespondWithObserver.cpp:25: };
On 2014/02/26 05:42:49, kinuko wrote:
> (these promise-waiter functions could be probably generalized a bit
further..?)

Yes, this copied WaitUntilObserver. Let me do a refactoring in a future patch.

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/RespondWithObserver.cpp:84: {
On 2014/02/26 05:42:49, kinuko wrote:
> Shouldn't we check executionContext() is non-null here? (If not we wouldn't
need
> to inherit from LifecycleObserver)

Oops, right. I guess this happens if the SW context dies before the respondWith
Promise is settled, or it died right before didDispatchEvent.

Fixed to be aware of destroyed executionContext.

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/RespondWithObserver.cpp:87: observeContext(0);
On 2014/02/26 05:42:49, kinuko wrote:
> I don't think this is necessary

Done.

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/Response.cpp (right):

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/Response.cpp:32: return response;
On 2014/02/26 05:42:49, kinuko wrote:
> I prefer defining ResponseInit in a separate header, like
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> so that we can handle default-values more gracefully, and ResponseInit
> definition can be more isolated.

Done. Good idea, that's much better.

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

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/Response.h:18: static PassRefPtr<Response>
create(const Dictionary&);
On 2014/02/26 05:42:49, kinuko wrote:
> nit: can you also write the param name (responseInit) if style-checker doesn't
> warn, as 'Dictionary' doesn't give much info?

Done.

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

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/Response.idl:12: // FIXME: Spec uses ByteString
for these. We must perform the DOMString -> ByteString conversion manually,
On 2014/02/26 05:42:49, kinuko wrote:
> We don't have ByteString support anywhere yet?

I don't think so. XHR code does it manually. I filed crbug.com/347426 with some
info.

https://codereview.chromium.org/180843003/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/Response.idl:17: // FIXME: Spec will eventually
define "HeaderMap" interface.
On 2014/02/26 05:42:49, kinuko wrote:
> I think HeaderMap just inherits MapClass, which is in WebIDL (therefore the
> nicer way to handle this is support it in binding layer)

I updated the FIXME to be a bit more descriptive and filed crbug.com/347435

https://codereview.chromium.org/180843003/diff/1/public/web/WebServiceWorkerF...
File public/web/WebServiceWorkerFetchResponse.h (right):

https://codereview.chromium.org/180843003/diff/1/public/web/WebServiceWorkerF...
public/web/WebServiceWorkerFetchResponse.h:21: class
WebServiceWorkerFetchResponse {
On 2014/02/26 05:42:49, kinuko wrote:
> It's a bit questionable if we want to share this interface /w other APIs that
> rely on Response object, e.g. Cache and Fetch(). If so probably we'd better
> consider which layer this should be in (public/web or public/platform), if
this
> interface should have 'fetch' in its name, if the type Fallback is appropriate
> here etc.

As you once suggested, let me kick the web/ stuff to a separate patch for easier
reviewing.

[kinuko, 2014-02-27 06:54:15]

Ok, I think this lgtm. Thanks,

[falken, 2014-02-27 06:59:20]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-02-27 06:59:33]

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

[falken, 2014-02-27 07:13:30]

The CQ bit was unchecked by falken@chromium.org

[falken, 2014-02-27 07:16:21]

jochen@: can you give an OWNER review for web/ please?

It's just a skeleton for now. For reference, the full change will look something
like Patch Set 1.

[falken, 2014-02-27 11:49:46]

Kinuko-san, my previous patch was missing ResponseInit.h, sorry. I uploaded a
new patch that includes ResponseInit.h and also fixes Windows build.

[kinuko, 2014-02-27 12:03:03]

On 2014/02/27 11:49:46, falken wrote:
> Kinuko-san, my previous patch was missing ResponseInit.h, sorry. I uploaded a
> new patch that includes ResponseInit.h and also fixes Windows build.

Yup.. still lgtm

[jochen (gone - plz use gerrit), 2014-02-27 14:16:47]

lgtm

[falken, 2014-02-27 14:20:26]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-02-27 14:20:47]

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

[commit-bot: I haz the power, 2014-02-27 14:20:52]

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

[commit-bot: I haz the power, 2014-02-27 14:20:59]

Failed to apply patch for Source/modules/modules.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/modules.gypi
  Hunk #1 FAILED at 110.
  Hunk #2 FAILED at 553.
  Hunk #3 succeeded at 563 (offset 3 lines).
  2 out of 3 hunks FAILED -- saving rejects to file
Source/modules/modules.gypi.rej

Patch:       Source/modules/modules.gypi
Index: Source/modules/modules.gypi
diff --git a/Source/modules/modules.gypi b/Source/modules/modules.gypi
index
408b0b55635728cadc5360a72998e1ebd1b0d236..714d81926178812dc5e536b276779a18fbc9823c
100644
--- a/Source/modules/modules.gypi
+++ b/Source/modules/modules.gypi
@@ -110,8 +110,10 @@
       'quota/StorageQuota.idl',
       'quota/StorageQuotaCallback.idl',
       'quota/StorageUsageCallback.idl',
+      'serviceworkers/FetchEvent.idl',
       'serviceworkers/InstallEvent.idl',
       'serviceworkers/InstallPhaseEvent.idl',
+      'serviceworkers/Response.idl',
       'serviceworkers/ServiceWorker.idl',
       'serviceworkers/ServiceWorkerContainer.idl',
       'serviceworkers/ServiceWorkerGlobalScope.idl',
@@ -553,6 +555,8 @@
       'screen_orientation/ScreenOrientationController.h',
       'screen_orientation/ScreenOrientationDispatcher.cpp',
       'screen_orientation/ScreenOrientationDispatcher.h',
+      'serviceworkers/FetchEvent.cpp',
+      'serviceworkers/FetchEvent.h',
       'serviceworkers/InstallEvent.cpp',
       'serviceworkers/InstallEvent.h',
       'serviceworkers/InstallPhaseEvent.cpp',
@@ -560,6 +564,11 @@
       'serviceworkers/NavigatorServiceWorker.cpp',
       'serviceworkers/NavigatorServiceWorker.h',
       'serviceworkers/RegistrationOptionList.h',
+      'serviceworkers/RespondWithObserver.cpp',
+      'serviceworkers/RespondWithObserver.h',
+      'serviceworkers/Response.cpp',
+      'serviceworkers/Response.h',
+      'serviceworkers/ResponseInit.h',
       'serviceworkers/ServiceWorker.cpp',
       'serviceworkers/ServiceWorker.h',
       'serviceworkers/ServiceWorkerContainer.cpp',

[falken, 2014-02-27 14:48:20]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-02-27 14:48:39]

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

[commit-bot: I haz the power, 2014-02-27 18:27:24]

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

[commit-bot: I haz the power, 2014-02-27 18:27:25]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[alecflett, 2014-02-27 18:36:28]

The CQ bit was checked by alecflett@chromium.org

[commit-bot: I haz the power, 2014-02-27 18:36:43]

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

[commit-bot: I haz the power, 2014-02-28 00:12:52]

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

[commit-bot: I haz the power, 2014-02-28 00:12:53]

Retried try job too often on linux_blink for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[falken, 2014-02-28 00:27:36]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-02-28 00:28:01]

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

[commit-bot: I haz the power, 2014-02-28 01:12:09]

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

[commit-bot: I haz the power, 2014-02-28 01:42:46]

Change committed as 168073