Bug 231064 – Add push registration stubs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ben Nham]

Add push registration stubs

[Ben Nham]

Created attachment 439823 [details]
Patch

[youenn fablet]

Comment on attachment 439823 [details]
Patch

A couple of nits below.
I like your idea to use a specific folder in modules, I'll update my patch accordingly.
I think I prefer Source/WebCore/Modules/push-api to better match the name of the spec and name of the related WPT test folder.

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

> Source/WebCore/ChangeLog:8
> +        Description to follow.

We should be able to rebase some WPT push-api tests within this patch

> Source/WTF/wtf/PlatformEnableCocoa.h:473
> +#if !defined(ENABLE_PUSH_API) && (PLATFORM(MAC) || PLATFORM(IOS))

I would hope SERVICE_WORKER to be sufficient here.

> Source/WebCore/Modules/push/PushPermissionState.h:32
> +enum class PushPermissionState {

It is a bit sad that there is a push specific enum for that, but the spec is like that so seems ok.

> Source/WebCore/Modules/push/PushSubscription.cpp:39
> +String PushSubscription::endpoint() const

const String&

> Source/WebCore/Modules/push/PushSubscription.h:46
> +    WTF_MAKE_ISO_ALLOCATED(PushSubscription);

It seems we might want a static create method to create those objects and a private constructor.
If we cannot exercise them right now, we could add an Internals API to create one of these objects to get some early testing.

> Source/WebCore/Modules/push/PushSubscriptionOptions.h:40
> +    WTF_MAKE_ISO_ALLOCATED(PushSubscriptionOptions);

Probably want a create/private constructor as well here.

[Ben Nham]

Created attachment 440078 [details]
Patch

[youenn fablet]

Comment on attachment 440078 [details]
Patch

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

> Source/WebCore/Modules/push-api/PushManager.cpp:55
> +    return PushService::singleton().supportedContentEncodings();

Is that thread safe?

> Source/WebCore/Modules/push-api/PushManager.cpp:60
> +    PushService::singleton().subscribe(m_serviceWorkerRegistrationKey, WTFMove(options), [promise = WTFMove(promise)](auto result) mutable {

Ditto, it seems we should have a push service connection specific for workers and for main threads, especially if we capture objects like promises.
I would tend to reuse ServiceWorkerRegistration infrastructure.

> Source/WebCore/Modules/push-api/PushManager.h:53
> +    PushManager(const ServiceWorkerRegistrationKey&);

explicit.
Maybe the PushManager should have a WeakPtr<ServiceWorkerRegistration> or a Ref to its ServiceWorkerContainer, or a Ref to a SWClientConnection, that will do the worker/main thread handling.

[Radar WebKit Bug Importer]

<rdar://problem/84014097>

[Ben Nham]

Created attachment 440700 [details]
Patch

[Ben Nham]

Created attachment 440722 [details]
Patch

[Ben Nham]

Created attachment 440724 [details]
Patch

[youenn fablet]

Comment on attachment 440724 [details]
Patch

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

> Source/WebCore/Modules/push-api/PushManager.cpp:55
> +    static const auto encodings = makeNeverDestroyed(Vector<String> { "aesgcm"_s, "aes128gcm"_s });

I don't think this is threadsafe, we could ref and deref these strings from various threads.

> Source/WebCore/Modules/push-api/PushManager.cpp:61
> +    if (!scriptExecutionContext.isSecureContext()) {

I am not sure this is needed, PushManager is probably not exposed in non secret context.
An ASSERT might be fine.

> Source/WebCore/Modules/push-api/PushManager.cpp:66
> +    if (!options || !options->userVisibleOnly) {

This makes sense to me but is not as per standard. Is it compatible with existing sites, given default is (sadly) false?

> Source/WebCore/Modules/push-api/PushManager.cpp:77
> +    KeyDataResult keyDataResult = WTF::switchOn(*options->applicationServerKey, [](RefPtr<JSC::ArrayBuffer>& value) -> KeyDataResult {

auto

> Source/WebCore/Modules/push-api/PushManager.cpp:109
> +    if (!m_serviceWorkerRegistration || !m_serviceWorkerRegistration->active()) {

I would think m_serciceWorkerRegistration should always be available if PushManager.
For instance, even if the serviceWorkerRegistration gets garbage collected, push manager should still be able to work fine.

One way to avoid a ref cycle would be the following:
ServiceWorkerRegistration owns a unique_ptr<PushManager>, lazily constructed.
PushManager has a ServiceWorkerRegistration&.
PushManager implements ref/deref methods by calling its ServiceWorkerRegistration ref/deref methods.

> Source/WebCore/Modules/push-api/ServiceWorkerRegistrationPushAPI.cpp:49
> +RefPtr<PushManager> ServiceWorkerRegistrationPushAPI::pushManager()

I guess it should either be a PushManager& or a Ref<PushManager>, since attribute value is not nullable.

> LayoutTests/ChangeLog:12
> +        * http/wpt/push-api/pushManager-serviceworker.js: Added.

You might be able to simplify a bit the tests by doing the following:
- Rename pushManager-test.js in pushManager.any.js (plus remove testSubscribe(), directly call test/promise_test)
- Add an empty file named http/wpt/push-api/pushManager.any.serviceworker.html
- Remove http/wpt/push-api/pushManager-serviceworker.https.htm and http/wpt/push-api/pushManager-serviceworker.js
WPT server will provide templated test files that will make use of your test.
You can see pushEvent.any.worker.js as an example.

> LayoutTests/http/wpt/push-api/pushManager-test.js:38
> +    promise_test(async (test) => {

It would be nice if we could run these tests in window environment as well.

[Ben Nham]

Created attachment 440742 [details]
Patch

[Ben Nham]

The latest patch should address your comments.

Although the standard doesn't require userVisibleOnly to be true, Chrome requires that (it throws otherwise) and we plan to do so as well.

[youenn fablet]

Comment on attachment 440742 [details]
Patch

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

> Source/WebCore/Modules/push-api/PushManager.cpp:78
> +        promise.reject(Exception { NotSupportedError, "Subscribing for push requires userVisibleOnly to be true"_s });

Chrome and Firefox seem to use NotAllowedError, can we align?

[youenn fablet]

(In reply to Ben Nham from comment #11)
> The latest patch should address your comments.
> 
> Although the standard doesn't require userVisibleOnly to be true, Chrome
> requires that (it throws otherwise) and we plan to do so as well.

Sounds good (although I wonder why we even have the userVisibleOnly=false option).
I filed https://github.com/w3c/push-api/issues/339 as I think it would be nice to mention this in the spec (and where/how to do this check).

[youenn fablet]

Comment on attachment 440742 [details]
Patch

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

> LayoutTests/http/wpt/push-api/pushManager.any.js:37
> +        registration = await navigator.serviceWorker.register("pushManager-worker.js");

It is probably worth unregistering in window environment as the last test as some sort of clean-up.

[Ben Nham]

Created attachment 440801 [details]
Patch

[EWS]

Committed r283916 (242785@main): <https://commits.webkit.org/242785@main>

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