[webkit-reviews] review granted: [Bug 231008] Add support for ServiceWorkerGlobalScope push event handler : [Attachment 439833] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 231008: Add support for ServiceWorkerGlobalScope push event handler
https://bugs.webkit.org/show_bug.cgi?id=231008

Attachment 439833: Patch

https://bugs.webkit.org/attachment.cgi?id=439833&action=review




--- Comment #6 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 439833
  --> https://bugs.webkit.org/attachment.cgi?id=439833
Patch

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

r=me with fixes.

> Source/WebCore/testing/ServiceWorkerInternals.cpp:60
> +void ServiceWorkerInternals::schedulePushEvent(const std::optional<String>&
message, RefPtr<DeferredPromise>&& promise)

We never use std::optional<String> because String already has a null state
(different than empty string).

The generated bindings code will pass you a null string if the parameter was
omitted in JS, not std::nullopt, so this is wrong.

> Source/WebCore/testing/ServiceWorkerInternals.cpp:66
> +    if (message) {

if (!message.isNull())

> Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:220
> +	   RELEASE_LOG(ServiceWorker,
"ServiceWorkerThread::queueTaskToFirePushEvent firing event for worker %llu",
serviceWorkerGlobalScope->thread().identifier().toUInt64());

PRIu64 not %llu

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:311
> +	       callOnMainRunLoop([this, protectedThis = WTFMove(protectedThis),
identifier, result]() mutable {

We're in WebCore so we should use callOnMainThread() or postTaskToLoader().
ServiceWorkers are WebKit2 only but I remember we used to get tasks out of
order by mixing callOnMainThread and callOnMainRunLoop (not sure if this is
still the case nowadays).