Bug 231008 - Add support for ServiceWorkerGlobalScope push event handler
Summary: Add support for ServiceWorkerGlobalScope push event handler
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 231007
Blocks: 182566 231070
  Show dependency treegraph
 
Reported: 2021-09-30 00:46 PDT by youenn fablet
Modified: 2021-10-02 16:47 PDT (History)
31 users (show)

See Also:


Attachments
Patch (26.76 KB, patch)
2021-09-30 02:46 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (30.34 KB, patch)
2021-10-01 01:59 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (30.31 KB, patch)
2021-10-02 03:45 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2021-09-30 00:46:26 PDT
Add support for ServiceWorkerGlobalScope push event handler
Comment 1 Thomas Steiner 2021-09-30 02:12:29 PDT
*** Bug 182566 has been marked as a duplicate of this bug. ***
Comment 2 youenn fablet 2021-09-30 02:46:13 PDT
Created attachment 439712 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2021-09-30 02:46:57 PDT
<rdar://problem/83710760>
Comment 4 Alex Christensen 2021-09-30 08:35:41 PDT
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.
Comment 5 youenn fablet 2021-10-01 01:59:38 PDT
Created attachment 439833 [details]
Patch
Comment 6 Chris Dumez 2021-10-01 14:33:54 PDT
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).
Comment 7 youenn fablet 2021-10-02 03:45:00 PDT
Created attachment 439961 [details]
Patch for landing
Comment 8 EWS 2021-10-02 04:57:55 PDT
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].
Comment 9 collimarco91 2021-10-02 05:30:24 PDT
"Status: RESOLVED FIXED" ... Does this mean that Web Push Notifications will be finally available in Safari?