[webkit-reviews] review granted: [Bug 231922] Add NetworkProcess stubs for push subscriptions : [Attachment 441668] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Ben Nham <nham at apple.com>'s
request for review:
Bug 231922: Add NetworkProcess stubs for push subscriptions
https://bugs.webkit.org/show_bug.cgi?id=231922

Attachment 441668: Patch

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




--- Comment #6 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 441668
  --> https://bugs.webkit.org/attachment.cgi?id=441668
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(ServiceWorkerRegistrationIdent
ifier 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&&.