[webkit-reviews] review granted: [Bug 233088] Add support for onpushsubscriptionchange event handler : [Attachment 444615] 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 233088: Add support for onpushsubscriptionchange event handler
https://bugs.webkit.org/show_bug.cgi?id=233088

Attachment 444615: Patch

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




--- Comment #13 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 444615
  --> https://bugs.webkit.org/attachment.cgi?id=444615
Patch

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

> Source/WebCore/Modules/push-api/PushSubscription.cpp:50
> +    , m_serviceWorkerRegistration(WTFMove(registration))

Let's have just one constructor, taking a RefPtr<>

> Source/WebCore/Modules/push-api/PushSubscription.h:70
> +    PushSubscription(PushSubscriptionData&&,
Ref<ServiceWorkerRegistration>&&);

I am not sure why we want to limit exporting.
If we want that, I would tend to have a dedicated create method:
WEBCORE_EXPORT static Ref<PushSubscription> create(PushSubscriptionData&&)
instead of two constructors.

> Source/WebCore/Modules/push-api/PushSubscriptionData.cpp:31
> +#include "ServiceWorkerRegistration.h"

No longer needed?

> Source/WebCore/Modules/push-api/PushSubscriptionData.cpp:-41
> -

No longer needed?

> Source/WebCore/Modules/push-api/PushSubscriptionData.h:32
> +#include <wtf/Forward.h>

No longer needed?

> Source/WebCore/Modules/push-api/PushSubscriptionData.h:39
> +

No longer needed?

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:327
> +    thread().runLoop().postTask([this, protectedThis = Ref { *this },
newSubscriptionData = crossThreadCopy(newSubscriptionData), oldSubscriptionData
= crossThreadCopy(oldSubscriptionData)](auto&) mutable {

We would write it as newSubscriptionData =
crossThreadCopy(WTFMove(newSubscriptionData)), in case we want to have an
optimised isolatedCopy() && on PushSubscriptionData in the future.