Summary: | Add preliminary support for fetch event | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, aestes, beidson, buildbot, cdumez, commit-queue, rniwa, sam, webkit-bug-importer | ||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
youenn fablet
2017-10-11 09:39:54 PDT
Created attachment 323456 [details]
Patch
Created attachment 323477 [details]
Patch
Created attachment 323483 [details]
Patch
Created attachment 323491 [details]
Patch
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 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
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 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
Created attachment 323526 [details]
Patch
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. 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. Created attachment 323553 [details]
Patch
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. Created attachment 323555 [details]
Patch
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. 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 Created attachment 323954 [details]
Patch
Created attachment 323955 [details]
Patch
Created attachment 323964 [details]
Patch
Created attachment 323978 [details]
Patch
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") ? Created attachment 324029 [details]
Patch for landing
Comment on attachment 324029 [details] Patch for landing Clearing flags on attachment: 324029 Committed r223562: <https://trac.webkit.org/changeset/223562> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 324054 [details]
build fix
Comment on attachment 324054 [details] build fix Clearing flags on attachment: 324054 Committed r223577: <https://trac.webkit.org/changeset/223577> All reviewed patches have been landed. Closing bug. |