[webkit-reviews] review granted: [Bug 234797] Add PushSubscriptionIdentifier : [Attachment 448179] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ben Nham <nham at apple.com>'s request
for review:
Bug 234797: Add PushSubscriptionIdentifier
https://bugs.webkit.org/show_bug.cgi?id=234797

Attachment 448179: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 448179
  --> https://bugs.webkit.org/attachment.cgi?id=448179
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(ServiceWorkerRegistrationIde
ntifier 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(PushSubscriptionIdentifie
r pushSubscriptionIdentifier, DOMPromiseDeferred<IDLBoolean>&& promise)

Ditto.

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

Ditto.

> Source/WebKit/NetworkProcess/ServiceWorker/WebSWServerConnection.cpp:458
> +void
WebSWServerConnection::unsubscribeFromPushService(WebCore::ServiceWorkerRegistr
ationIdentifier 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::ServiceWorkerRegistr
ationIdentifier, WebCore::PushSubscriptionIdentifier,
CompletionHandler<void(Expected<bool, ExceptionData>&&)>&& completionHandler)

> Source/WebKit/WebProcess/Storage/WebSWClientConnection.cpp:241
> +void
WebSWClientConnection::unsubscribeFromPushService(WebCore::ServiceWorkerRegistr
ationIdentifier 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.