1219064 - Add test for extendable `pushsubscriptionchange` event

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler (ex-Mozilla)]

The test in bug 1205109 fails intermittently. I'm not sure if it's caused by `nsFrameMessageManager` or the other failures we've been seeing, but I need the `ExtendableEvent` change for bug 1219063. Breaking the test out into a separate bug.

Comment 1

[Lina Butler (ex-Mozilla)]

Attachment: MozReview Request: Bug 1219064 - Add test for extendable `pushsubscriptionchange` event. r?mt

Bug 1219064 - Add test for extendable `pushsubscriptionchange` event. r?mt

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8680339 [details]
MozReview Request: Bug 1219064 - Add test for extendable `pushsubscriptionchange` event. r?mt

https://reviewboard.mozilla.org/r/23619/#review21115

This looks good.  I'd like an answer to how you plan to deal with the issue; or, if you think you need help, what help you might need (like the conclusion to the push-api issue).

::: dom/push/test/test_subscription_change.html:40
(Diff revision 1)
> +    registration = yield navigator.serviceWorker.register(url, {scope: "."});

Scope of "/" instead?

::: dom/push/test/test_subscription_change.html:65
(Diff revision 1)
> +    try {
> +      yield registration.pushManager.getSubscription();
> +      ok(false, "Should not return subscription when permission is revoked");
> +    } catch (e) {
> +      ok(true, "Should throw for subscription if push permission is revoked");
> +    }

Though it wasn't decided, my view was that this was not a desirable behaviour.  `getSubscription` should just resolve to null if there is no subscription (which is always the case if there is no permission).

::: dom/push/test/worker.js:50
(Diff revision 1)
> +    if (clients.length) {
> +      clients[0].postMessage({type: "changed", okay: "yes"});
> +    }

Nit: I suggest that you just iterate over the clients and send them all the message.  That will be more robust and also less code.

Comment 3

[Lina Butler (ex-Mozilla)]

> Though it wasn't decided, my view was that this was not a desirable behaviour.  `getSubscription` should just resolve to null if there is no subscription (which is always the case if there is no permission).

Ah, my apologies; I missed your comment in https://github.com/w3c/push-api/issues/150. I'll take care of that first, in a separate ticket.

Comment 4

[Lina Butler (ex-Mozilla)]

https://reviewboard.mozilla.org/r/23619/#review21207

::: dom/push/test/test_subscription_change.html:40
(Diff revision 1)
> +    registration = yield navigator.serviceWorker.register(url, {scope: "."});

Unfortunately, that fails with a `SecurityError`.

::: dom/push/test/worker.js:50
(Diff revision 1)
> +    if (clients.length) {
> +      clients[0].postMessage({type: "changed", okay: "yes"});
> +    }

Good call.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa49fbb1bc2

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eaa49fbb1bc2

Comment 7

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=20808764&repo=mozilla-inbound

Comment 8

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/941033a51983

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d64e928b75ac

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d64e928b75ac

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/21b3c24170a7