Bug 231008 – Add support for ServiceWorkerGlobalScope push event handler

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Add support for ServiceWorkerGlobalScope push event handler

[Thomas Steiner]

*** Bug 182566 has been marked as a duplicate of this bug. ***

[youenn fablet]

Created attachment 439712 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/83710760>

[Alex Christensen]

Comment on attachment 439712 [details]
Patch

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

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:305
> +    if (m_ongoingPushTasks.isEmpty())

It seems like we might be able to get away with just storing the number of ongoing tasks in a size_t instead of a HashMap of the CompletionHandlers.

[youenn fablet]

Created attachment 439833 [details]
Patch

[Chris Dumez]

Comment on attachment 439833 [details]
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).

[youenn fablet]

Created attachment 439961 [details]
Patch for landing

[EWS]

Committed r283438 (242425@main): <https://commits.webkit.org/242425@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439961 [details].

"Status: RESOLVED FIXED" ... Does this mean that Web Push Notifications will be finally available in Safari?