Bug 234797 – Add PushSubscriptionIdentifier

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

[Ben Nham]

Add PushSubscriptionIdentifier

[Ben Nham]

Created attachment 448179 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/87313393>

[Darin Adler]

Comment on attachment 448179 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:6624
> +    return PushSubscription::create(PushSubscriptionData { PushSubscriptionIdentifier(), WTFMove(myEndpoint), expirationTime, WTFMove(myServerVAPIDPublicKey), WTFMove(myClientECDHPublicKey), WTFMove(myAuth) });

Why is it OK to have no identifier here?

Can this just be { } instead of PushSubscriptionIdentifier()? Eventually the line of code gets so long it’s too hard to read, and so omitting the name could help.

> Source/WebCore/testing/ServiceWorkerInternals.cpp:188
> +    return PushSubscription::create(PushSubscriptionData { PushSubscriptionIdentifier(), WTFMove(myEndpoint), expirationTime, WTFMove(myServerVAPIDPublicKey), WTFMove(myClientECDHPublicKey), WTFMove(myAuth) });

Ditto.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:573
> +void ServiceWorkerContainer::unsubscribeFromPushService(ServiceWorkerRegistrationIdentifier identifier, PushSubscriptionIdentifier pushSubscriptionIdentifier, DOMPromiseDeferred<IDLBoolean>&& promise)

I suggest just "subscriptionIdentifier" or "pushIdentifier" here to fight the readability problems we have if we get too wordy.

> Source/WebCore/workers/service/ServiceWorkerRegistration.cpp:176
> +void ServiceWorkerRegistration::unsubscribeFromPushService(PushSubscriptionIdentifier pushSubscriptionIdentifier, DOMPromiseDeferred<IDLBoolean>&& promise)

Ditto.

> Source/WebCore/workers/service/WorkerSWClientConnection.cpp:248
> +void WorkerSWClientConnection::unsubscribeFromPushService(ServiceWorkerRegistrationIdentifier registrationIdentifier, PushSubscriptionIdentifier pushSubscriptionIdentifier, UnsubscribeFromPushServiceCallback&& callback)

Ditto.

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:458
> +void WebSWServerConnection::unsubscribeFromPushService(WebCore::ServiceWorkerRegistrationIdentifier registrationIdentifier, WebCore::PushSubscriptionIdentifier pushSubscriptionIdentifier, CompletionHandler<void(Expected<bool, ExceptionData>&&)>&& completionHandler)

I suggest omitting the argument names here instead of having argument names and then citing them in UNUSED_PARAM below.

    void WebSWServerConnection::unsubscribeFromPushService(WebCore::ServiceWorkerRegistrationIdentifier, WebCore::PushSubscriptionIdentifier, CompletionHandler<void(Expected<bool, ExceptionData>&&)>&& completionHandler)

> Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:241
> +void WebSWClientConnection::unsubscribeFromPushService(WebCore::ServiceWorkerRegistrationIdentifier registrationIdentifier, WebCore::PushSubscriptionIdentifier pushSubscriptionIdentifier, UnsubscribeFromPushServiceCallback&& callback)

I suggest just "subscriptionIdentifier" or "pushIdentifier" here to fight the readability problems we have if we get too wordy.

[Ben Nham]

(In reply to Darin Adler from comment #3)
> Comment on attachment 448179 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448179&action=review
> 
> > Source/WebCore/testing/Internals.cpp:6624
> > +    return PushSubscription::create(PushSubscriptionData { PushSubscriptionIdentifier(), WTFMove(myEndpoint), expirationTime, WTFMove(myServerVAPIDPublicKey), WTFMove(myClientECDHPublicKey), WTFMove(myAuth) });
> 
> Why is it OK to have no identifier here?

The default identifier of 0 means that the subscription is not persisted in the push subscription database (e.g. because unsubscribe() has been called on it). These internal testing methods are used to create these subscriptions in the unsubscribed and unpersisted state for layout tests.

> Can this just be { } instead of PushSubscriptionIdentifier()? Eventually the
> line of code gets so long it’s too hard to read, and so omitting the name
> could help.

I'll fix this as well as the naming issues you brought up in the patch for landing.

[Ben Nham]

Created attachment 449742 [details]
Patch for landing

[EWS]

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

[Ben Nham]

Created attachment 449743 [details]
Patch for landing

[EWS]

Committed r288416 (246307@main): <https://commits.webkit.org/246307@main>

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