Bug 231922 – Add NetworkProcess stubs for push subscriptions

• Component: New Bugs
• Status: RESOLVED FIXED

[Ben Nham]

Add NetworkProcess stubs for push subscriptions

[Ben Nham]

Created attachment 441659 [details]
Patch

[Ben Nham]

Created attachment 441661 [details]
Patch

[Ben Nham]

Created attachment 441668 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/84400696>

[Ben Nham]

Comment on attachment 441668 [details]
Patch

One note is that the IPC parser didn't seem to handle a return type of Expected<enum:uint8_t PushPermissionState, ExceptionData> properly (its codegen tried to emit an #include<enum:uint8_t ...> in the cpp file). For now that IPC message just returns an Expected<uint8_t, ExceptionData> until I can take a look at that codegen issue in a separate patch.

[youenn fablet]

Comment on attachment 441668 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        via ServiceWorkerRegistration => ServiceWorkerContainer => SWClientConnection.

Do the existing tests fully cover this case?
Maybe it is too early to do, but maybe there are some things to test (network process crash).

> Source/WebCore/Modules/push-api/PushSubscription.h:62
> +    WEBCORE_EXPORT PushSubscription(ServiceWorkerRegistration*, String&& endpoint, std::optional<EpochTimeStamp> expirationTime, Ref<PushSubscriptionOptions>&&, Vector<uint8_t>&& clientECDHPublicKey, Vector<uint8_t>&& auth);

It would be better to pass a Ref<ServiceWorkerRegistration> if possible.
If the issue is PushSubscription creation in Internals, we can always change createPushSubscription to add a ServiceWorkerRegistration as parameter.

> Source/WebCore/Modules/push-api/PushSubscription.h:64
> +    RefPtr<ServiceWorkerRegistration> m_serviceWorkerRegistration;

Would be better as a Ref.

> Source/WebCore/Modules/push-api/PushSubscriptionData.h:60
> +bool PushSubscriptionData::decode(Decoder& decoder, PushSubscriptionData& data)

Would be better to use the modern form:
template<class Decoder> std::optional<PushSubscriptionData> PushSubscriptionData::decode(Decoder& decoder)

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:557
> +    auto options = PushSubscriptionOptions::create(true, WTFMove(data.serverVAPIDPublicKey));

true is a bit mysterious.
I guess bool userVisibleOnly = true would be clearer.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:581
> +        promise.resolve(result.releaseReturnValue());

Can we use promise.settle(WTFMove(result))?

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:599
> +        promise.resolve(createPushSubscriptionFromData(protectedRegistration, WTFMove(*optionalPushSubscriptionData)).ptr());

This triggers some count churn.
I would write it as 
RefPtr<PushSubscription> subscription;
if (optionalPushSubscriptionData)
     subscription = createPushSubscriptionFromData(protectedRegistration, WTFMove(*optionalPushSubscriptionData));
promise.resolve(WTFMove(subscription));

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:611
> +        promise.resolve(result.releaseReturnValue());

promise.settle as well?

> Source/WebCore/workers/service/WorkerSWClientConnection.cpp:224
> +void WorkerSWClientConnection::subscribeToPushService(ServiceWorkerRegistrationIdentifier registrationIdentifier, const Vector<uint8_t>& applicationServerKey, SubscribeToPushServiceCallback&& callback)

Could we pass a Vector<uint8_t>&& to not copy it here? It seems at the push manager layer, we have a Vector&&.

[Ben Nham]

Created attachment 441788 [details]
Patch

[Alex Christensen]

Comment on attachment 441788 [details]
Patch

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

> Source/WebCore/Modules/push-api/PushSubscriptionData.cpp:2
> + * Copyright (C) 2017 Apple Inc. All rights reserved.

nit: 2021

[Ben Nham]

Created attachment 441824 [details]
Patch

[EWS]

Committed r284518 (243261@main): <https://commits.webkit.org/243261@main>

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