[webkit-reviews] review denied: [Bug 178171] Add preliminary support for fetch event : [Attachment 323555] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 16 12:20:59 PDT 2017
Chris Dumez <cdumez at apple.com> has denied youenn fablet <youennf at gmail.com>'s
request for review:
Bug 178171: Add preliminary support for fetch event
https://bugs.webkit.org/show_bug.cgi?id=178171
Attachment 323555: Patch
https://bugs.webkit.org/attachment.cgi?id=323555&action=review
--- Comment #15 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 323555
--> https://bugs.webkit.org/attachment.cgi?id=323555
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.
More information about the webkit-reviews
mailing list