[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