Bug 232455 – Add support for PushSubscriptionChangeEvent

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[Ben Nham]

We should import the PushSubscriptionChangeEvent IDL and add relevant tests.

[Radar WebKit Bug Importer]

<rdar://problem/84778954>

[Ben Nham]

Created attachment 442743 [details]
Patch

[Ben Nham]

Created attachment 442747 [details]
Patch

[youenn fablet]

Comment on attachment 442747 [details]
Patch

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

> Source/WebCore/ChangeLog:20
> +        ServiceWorkerRegistration::ref/deref to be exported.

Can you give more info on why is it bad to export ref/deref?

> Source/WebCore/Modules/push-api/PushSubscription.h:65
> +    RefPtr<ServiceWorkerRegistration> m_serviceWorkerRegistration;

I do not think this is progress here.
We now have to deal with the fact that m_serviceWorkerRegistration can be null, while, as I understand it, it should never be null in practice.

If the issue is testing, I guess we could add something like a ServiceWorkerInternals::schedulePushSubscriptionChangeEvent that would take data necessary to call SWContextManager::singleton().firePushSubscriptionChangeEvent.
We would no longer need to construct PushSubscription from internals.
Instead, we would get these objects using the normal code path of receiving a PushSubscriptionChangeEvent.
Once we have them, we can create synthetic PushSubscriptionChangeEvent based on them.

> Source/WebCore/Modules/push-api/PushSubscriptionChangeEventInit.idl:26
> +typedef (BufferSource or USVString) PushMessageDataInit;

Not needed

[Ben Nham]

(In reply to youenn fablet from comment #4)
> Comment on attachment 442747 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442747&action=review
> 
> > Source/WebCore/ChangeLog:20
> > +        ServiceWorkerRegistration::ref/deref to be exported.
> 
> Can you give more info on why is it bad to export ref/deref?

I was trying to cut down on the number of symbols exported purely for testing.

> > Source/WebCore/Modules/push-api/PushSubscription.h:65
> > +    RefPtr<ServiceWorkerRegistration> m_serviceWorkerRegistration;
> 
> I do not think this is progress here.
> We now have to deal with the fact that m_serviceWorkerRegistration can be
> null, while, as I understand it, it should never be null in practice.

When we fire a PushSubscriptionChangeEvent, it contains two PushSubscriptions: the old one and possibly the new one. For instance, we would fire it when you revisit an origin and we've already unregistered the PushSubscription in the background, e.g. if you've sent too many silent pushes. (This behavior is similar to Firefox's.)

In this case, the it seems like the old PushSubscription object should not be associated with a ServiceWorkerRegistration, since it's already been unsubscribed. This is the case where ServiceWorkerRegistration would be null.

[youenn fablet]

Comment on attachment 442747 [details]
Patch

OK, sounds good to use a RefPtr.

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

> Source/WebCore/Modules/push-api/PushSubscription.cpp:101
> +        promise.resolve(false);

According spec, we should most probably queue a task to resolve the promise with false.
This is probably testable with using callback ordering of unsubscribe promise, Promise.resolve() and Promise(resolve => setTimeout(resolve, 0)).

[Ben Nham]

(In reply to youenn fablet from comment #6)
> Comment on attachment 442747 [details]
> Patch
> 
> OK, sounds good to use a RefPtr.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=442747&action=review
> 
> > Source/WebCore/Modules/push-api/PushSubscription.cpp:101
> > +        promise.resolve(false);
> 
> According spec, we should most probably queue a task to resolve the promise
> with false.
> This is probably testable with using callback ordering of unsubscribe
> promise, Promise.resolve() and Promise(resolve => setTimeout(resolve, 0)).

I talked to Chris and Antoine about this and they said the Promise handler should already run in a separate microtask (see resolvePromise in PromiseOperations.js). So I don't think we additionally need to call `promise.resolve` in its own microtask. I also wrote quick test case to verify with PushSubscription.unsubscribe and it passed.

[Chris Dumez]

Comment on attachment 442747 [details]
Patch

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

>>> Source/WebCore/Modules/push-api/PushSubscription.cpp:101
>>> +        promise.resolve(false);
>> 
>> According spec, we should most probably queue a task to resolve the promise with false.
>> This is probably testable with using callback ordering of unsubscribe promise, Promise.resolve() and Promise(resolve => setTimeout(resolve, 0)).
> 
> I talked to Chris and Antoine about this and they said the Promise handler should already run in a separate microtask (see resolvePromise in PromiseOperations.js). So I don't think we additionally need to call `promise.resolve` in its own microtask. I also wrote quick test case to verify with PushSubscription.unsubscribe and it passed.

@Youenn: The spec seems to say:
"If the push subscription has already been deactivated, resolve promise with false and terminate these steps."

Where did you see that we should queue a task to resolve the promise? Your comment does not seem to match my reading of the spec text.

[Chris Dumez]

Comment on attachment 442747 [details]
Patch

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

>>>> Source/WebCore/Modules/push-api/PushSubscription.cpp:101
>>>> +        promise.resolve(false);
>>> 
>>> According spec, we should most probably queue a task to resolve the promise with false.
>>> This is probably testable with using callback ordering of unsubscribe promise, Promise.resolve() and Promise(resolve => setTimeout(resolve, 0)).
>> 
>> I talked to Chris and Antoine about this and they said the Promise handler should already run in a separate microtask (see resolvePromise in PromiseOperations.js). So I don't think we additionally need to call `promise.resolve` in its own microtask. I also wrote quick test case to verify with PushSubscription.unsubscribe and it passed.
> 
> @Youenn: The spec seems to say:
> "If the push subscription has already been deactivated, resolve promise with false and terminate these steps."
> 
> Where did you see that we should queue a task to resolve the promise? Your comment does not seem to match my reading of the spec text.

Never mind, I think I get it now. The spec says:
- Return promise and continue the following steps asynchronously.
- If the push subscription has already been deactivated, resolve promise with false and terminate these steps.

Because of the previous step, the promise resolution should indeed happen asynchronously

[Ben Nham]

Created attachment 443728 [details]
Patch

[Ben Nham]

Created attachment 443729 [details]
Patch

[EWS]

Committed r285564 (244072@main): <https://commits.webkit.org/244072@main>

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