Bug 178171 – Add preliminary support for fetch event

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Add preliminary support for fetch event

[youenn fablet]

Created attachment 323456 [details]
Patch

[youenn fablet]

Created attachment 323477 [details]
Patch

[youenn fablet]

Created attachment 323483 [details]
Patch

[youenn fablet]

Created attachment 323491 [details]
Patch

[Build Bot]

Comment on attachment 323491 [details]
Patch

Attachment 323491 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4830506

New failing tests:
http/wpt/service-workers/fetchEvent.https.html
http/wpt/service-workers/extendableEvent.https.html

[Build Bot]

Created attachment 323501 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 323491 [details]
Patch

Attachment 323491 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4830402

New failing tests:
http/wpt/service-workers/fetchEvent.https.html
http/wpt/service-workers/extendableEvent.https.html

[Build Bot]

Created attachment 323502 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[youenn fablet]

Created attachment 323526 [details]
Patch

[Sam Weinig]

Comment on attachment 323526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323526&action=review

> Source/WebCore/workers/service/ExtendableEvent.h:63
> +    WTF::Function<void()> m_onFinishedWaiting;
> +
> +};

Extra newline.

> Source/WebCore/workers/service/ExtendableEvent.idl:2
> +//FIXME: Should be exposed on ServiceWorker only.
> +[

This needs a license.

> Source/WebCore/workers/service/ExtendableEventInit.idl:2
> +dictionary ExtendableEventInit : EventInit {
> +};

This needs a license.

> Source/WebCore/workers/service/FetchEvent.idl:1
> +//FIXME: Should be exposed on ServiceWorker only.

This needs a license.

> Source/WebCore/workers/service/FetchEventInit.idl:1
> +dictionary FetchEventInit : ExtendableEventInit {

This needs a license.

[Sam Weinig]

Comment on attachment 323526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323526&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:3425
> +		7E4C96DC1AD4483500365A52 /* WebCore/JSFetchEvent.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 7E4C96D81AD4483500365A52 /* WebCore/JSFetchEvent.cpp */; };
> +		7E4C96DC1AD4483500365A53 /* WebCore/JSExtendableEvent.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 7E4C96D81AD4483500365A53 /* WebCore/JSExtendableEvent.cpp */; };

This looks wrong. I don't think there should be a WebCore/ there.

[youenn fablet]

Created attachment 323553 [details]
Patch

[youenn fablet]

Thanks for the feedback.
Fixed the license and newline issues.

(In reply to Sam Weinig from comment #11)
> Comment on attachment 323526 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323526&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:3425
> > +		7E4C96DC1AD4483500365A52 /* WebCore/JSFetchEvent.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 7E4C96D81AD4483500365A52 /* WebCore/JSFetchEvent.cpp */; };
> > +		7E4C96DC1AD4483500365A53 /* WebCore/JSExtendableEvent.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 7E4C96D81AD4483500365A53 /* WebCore/JSExtendableEvent.cpp */; };
> 
> This looks wrong. I don't think there should be a WebCore/ there.

Agreed.
I did it this way since all JSServiceWorker* are done this way.
Changing that for the JSEvent* would mean changing all of them.
I can do that as a follow-up.

[youenn fablet]

Created attachment 323555 [details]
Patch

[Chris Dumez]

Comment on attachment 323555 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323555&action=review

> Source/WebCore/bindings/js/JSDOMPromise.cpp:70
> +void DOMPromise::whenSettled(std::function<void()>&& callback)

Why isn't this using WTF::Function?

> Source/WebCore/testing/Internals.cpp:4201
> +void Internals::fetchEventFinishedWaiting(FetchEvent& event, DOMPromiseDeferred<IDLInterface<FetchResponse>>&& promise)

I do not like the naming. I think this should start with "set" prefix. Maybe setFetchEventResponseHandler?

> Source/WebCore/testing/Internals.cpp:4211
> +void Internals::extendableEventFinishedWaiting(ExtendableEvent& event, DOMPromiseDeferred<void>&& promise)

I do not like the naming. I think this should start with "set" prefix.

> Source/WebCore/testing/Internals.cpp:4218
> +Ref<ExtendableEvent> Internals::trustedExtendableEvent()

createTrustedExtendableEvent() ?

> Source/WebCore/workers/service/ExtendableEvent.cpp:38
> +ExceptionOr<void> ExtendableEvent::waitUntil(JSC::ExecState& execState, JSC::JSValue promise)

We call this "state" usually I believe, not execState.

> Source/WebCore/workers/service/ExtendableEvent.cpp:43
> +    if (!m_pendingPromises.size() && dispatched())

m_pendingPromises.isEmpty()

Any reason we cannot use Event::isBeingDispatched() ?

> Source/WebCore/workers/service/ExtendableEvent.cpp:67
> +        if (m_pendingPromises.size())

!m_pendingPromises.isEmpty()

> Source/WebCore/workers/service/ExtendableEvent.h:49
> +    WEBCORE_EXPORT void onFinishedWaiting(WTF::Function<void()>&&);

I think the name should indicate this is only used for testing purposes (e.g. ForTesting suffix).

> Source/WebCore/workers/service/ExtendableEvent.h:61
> +    WTF::Function<void()> m_onFinishedWaiting;

Name should indicate this is only used for testing.

> Source/WebCore/workers/service/ExtendableEvent.idl:28
> +    Constructor(DOMString type, optional EventInit eventInitDict),

I think we should have a ExtendableEventInit like in the spec, even if empty for now.

> Source/WebCore/workers/service/FetchEvent.cpp:37
> +    , m_request(WTFMove(initializer.request))

initializer.request.releaseNonNull()

> Source/WebCore/workers/service/FetchEvent.cpp:46
> +    if (dispatched())

isBeingDispatched() is the getter that indicates if an event is being dispatched.

> Source/WebCore/workers/service/FetchEvent.h:56
> +    FetchRequest* request() { return m_request.get(); }

This should return a reference. m_request can never be null.

> Source/WebCore/workers/service/FetchEvent.h:70
> +    RefPtr<FetchRequest> m_request;

Should use Ref<>

> Source/WebCore/workers/service/FetchEvent.h:78
> +    RefPtr<DOMPromise> m_respondPromise { nullptr };

{ nullptr } is useless and should be omitted.

> Source/WebCore/workers/service/FetchEvent.idl:36
> +    [SameObject] readonly attribute FetchRequest request;

I believe you need a:
void JSFetchEvent::visitAdditionalChildren(SlotVisitor& visitor)
{
    visitor.addOpaqueRoot(&wrapped().request());
}

in JSFetchEventCustom.cpp, as [SameObject] is currently unsupported. This is needed so that the request wrapper stays alive at least as long as its FetchEvent.

This is observable like so:
fetchEvent.request.myTestExpando = 2;
gc();
console.log(fetchEvent.request.myTestExpando); // Should be 2, not undefined.

We should add a test for this.

> Source/WebCore/workers/service/FetchEvent.idl:45
> +dictionary FetchEventInit : EventInit {

Should inherit ExtendableEventInit.

[youenn fablet]

Thanks, I'll upload a new patch fixing most issues.

> > Source/WebCore/bindings/js/JSDOMPromise.cpp:70
> > +void DOMPromise::whenSettled(std::function<void()>&& callback)
> 
> Why isn't this using WTF::Function?

JSFunction does not take a WTF::Function right now.
We should fix that and then use a WTF::Function here.

> > Source/WebCore/testing/Internals.cpp:4201
> > +void Internals::fetchEventFinishedWaiting(FetchEvent& event, DOMPromiseDeferred<IDLInterface<FetchResponse>>&& promise)
> 
> I do not like the naming. I think this should start with "set" prefix. Maybe
> setFetchEventResponseHandler?

Changed to whenFetch>..


> This is observable like so:
> fetchEvent.request.myTestExpando = 2;
> gc();
> console.log(fetchEvent.request.myTestExpando); // Should be 2, not undefined.
> 
> We should add a test for this.

Will add the test

[youenn fablet]

Created attachment 323954 [details]
Patch

[youenn fablet]

Created attachment 323955 [details]
Patch

[youenn fablet]

Created attachment 323964 [details]
Patch

[youenn fablet]

Created attachment 323978 [details]
Patch

[Chris Dumez]

Comment on attachment 323978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=323978&action=review

r=me with comments.

> Source/WebCore/bindings/js/JSDOMPromise.cpp:105
> +    WTFCrash();

RELEASE_ASSERT_NOT_REACHED() or ASSERT_NOT_REACHED();

> Source/WebCore/testing/Internals.cpp:4201
> +void Internals::waitFetchEventToFinish(FetchEvent& event, DOMPromiseDeferred<IDLInterface<FetchResponse>>&& promise)

waitForFetchEventToFinish()

> Source/WebCore/testing/Internals.cpp:4211
> +void Internals::waitExtendableEventToFinish(ExtendableEvent& event, DOMPromiseDeferred<void>&& promise)

waitForExtendableEventToFinish()

> Source/WebCore/testing/Internals.cpp:4220
> +    return ExtendableEvent::create("ExtendableEvent", { }, Event::IsTrusted::Yes);

ASCIILiteral("ExtendableEvent") ?

[youenn fablet]

Created attachment 324029 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 324029 [details]
Patch for landing

Clearing flags on attachment: 324029

Committed r223562: <https://trac.webkit.org/changeset/223562>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35033934>

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 324054 [details]
build fix

[WebKit Commit Bot]

Comment on attachment 324054 [details]
build fix

Clearing flags on attachment: 324054

Committed r223577: <https://trac.webkit.org/changeset/223577>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.