Bug 234797

Summary: Add PushSubscriptionIdentifier
Product: WebKit Reporter: Ben Nham <nham>
Component: New BugsAssignee: Ben Nham <nham>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, beidson, darin, ews-watchlist, gyuyoung.kim, ryuan.choi, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing none

Description Ben Nham 2022-01-02 10:54:18 PST
Add PushSubscriptionIdentifier
Comment 1 Ben Nham 2022-01-02 11:10:39 PST
Created attachment 448179 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2022-01-09 10:55:44 PST
<rdar://problem/87313393>
Comment 3 Darin Adler 2022-01-16 12:16:35 PST
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.
Comment 4 Ben Nham 2022-01-22 15:52:52 PST
(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.
Comment 5 Ben Nham 2022-01-22 17:07:43 PST
Created attachment 449742 [details]
Patch for landing
Comment 6 EWS 2022-01-22 17:08:43 PST
ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.
Comment 7 Ben Nham 2022-01-22 20:30:22 PST
Created attachment 449743 [details]
Patch for landing
Comment 8 EWS 2022-01-22 21:22:05 PST
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].