Add test for extendable `pushsubscriptionchange` event

RESOLVED FIXED in Firefox 46

Status

()

Core
DOM: Push Notifications
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kitcambridge, Assigned: kitcambridge)

Tracking

(Depends on: 1 bug)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 wontfix, firefox46 fixed, firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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.
Created attachment 8680339 [details]
MozReview Request: Bug 1219064 - Add test for extendable `pushsubscriptionchange` event. r?mt

Bug 1219064 - Add test for extendable `pushsubscriptionchange` event. r?mt
Attachment #8680339 - Flags: review?(martin.thomson)
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.
Attachment #8680339 - Flags: review?(martin.thomson) → review+
> 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.
Depends on: 1159641
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.
Depends on: 1223202

Updated

2 years ago
status-firefox44: affected → wontfix

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eaa49fbb1bc2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
sorry had to back this out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=20808764&repo=mozilla-inbound
Flags: needinfo?(kcambridge)

Updated

2 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(kcambridge)

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d64e928b75ac
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED

Comment 11

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/21b3c24170a7
status-firefox46: --- → fixed
You need to log in before you can comment on or make changes to this bug.