Bug 231278 – Add PushSubscription stubs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ben Nham]

Add PushSubscription stubs

[Ben Nham]

Created attachment 440330 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/83918867>

[Alex Christensen]

Comment on attachment 440330 [details]
Patch

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

> Source/WebCore/Modules/push-api/PushEncryptionKeyName.h:32
> +enum class PushEncryptionKeyName {

: bool

> Source/WebCore/Modules/push-api/PushSubscription.cpp:103
> +            { "p256dh", base64URLEncodeToString(m_clientECDHPublicKey) },
> +            { "auth", base64URLEncodeToString(m_auth) }

_s

> Source/WebCore/Modules/push-api/PushSubscription.h:48
> +    WEBCORE_EXPORT static Ref<PushSubscription> create(String&& endpoint, std::optional<DOMTimeStamp> expirationTime, Ref<PushSubscriptionOptions>&&, Vector<uint8_t>&& clientECDHPublicKey, Vector<uint8_t>&& auth);

This might be a good candidate for something like this:
template<typename... Args> static Ref<PushSubscription> create(Args&&... args) { return adoptRef(*new PushSubscription(std::forward<Args>(args)...)); }

> Source/WebCore/Modules/push-api/PushSubscription.h:66
> +    Vector<uint8_t> m_auth;

Could you use a whole word to describe what this is instead of an abbreviation?

> Source/WebCore/Modules/push-api/PushSubscriptionOptions.cpp:40
> +

extra space.

[Ben Nham]

Created attachment 440335 [details]
Patch

[youenn fablet]

Comment on attachment 440335 [details]
Patch

Some nits below

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

> Source/WebCore/Modules/push-api/PushSubscription.cpp:65
> +Ref<PushSubscriptionOptions> PushSubscription::options() const

We can probably return a PushSubscriptionOptions&

> Source/WebCore/Modules/push-api/PushSubscription.h:37
> +#include <array>

Why do we need this one?

> Source/WebCore/Modules/push-api/PushSubscription.h:40
> +#include <wtf/RefCounted.h>

PushSubscriptionOptions.h probably has some of these.

> Source/WebCore/Modules/push-api/PushSubscriptionOptions.cpp:69
> +    return protectedApplicationServerKey;

return m_applicationServerKey.copyRef()?

> LayoutTests/ChangeLog:14
> +        * http/wpt/push-api/pushSubscription.https.any.html: Added.

Maybe not needed right now, but it will be good in the future to cover this in service worker context as well.
In case this is needed ServiceWorkerInternals can help.

[Ben Nham]

Created attachment 440391 [details]
Patch

[Ben Nham]

(In reply to youenn fablet from comment #5)
> > LayoutTests/ChangeLog:14
> > +        * http/wpt/push-api/pushSubscription.https.any.html: Added.
> 
> Maybe not needed right now, but it will be good in the future to cover this
> in service worker context as well.
> In case this is needed ServiceWorkerInternals can help.

For some reason I was having trouble with this--globalThis.internals was null in the service worker context in this test. I plan to change this test soon anyway to not use internals and just create a fake subscription via PushManager.subscribe. Once I do that I'll modify the test to run in the window and worker context.

[Ben Nham]

Created attachment 440414 [details]
Patch

[EWS]

Committed r283700 (242626@main): <https://commits.webkit.org/242626@main>

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