Closed
Bug 1219064
Opened 9 years ago
Closed 8 years ago
Add test for extendable `pushsubscriptionchange` event
Categories
(Core :: DOM: Push Subscriptions, defect)
Core
DOM: Push Subscriptions
Tracking
()
RESOLVED
FIXED
mozilla47
People
(Reporter: lina, Assigned: lina)
References
Details
Attachments
(1 file)
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.
Assignee | ||
Comment 1•9 years ago
|
||
Bug 1219064 - Add test for extendable `pushsubscriptionchange` event. r?mt
Attachment #8680339 -
Flags: review?(martin.thomson)
Comment 2•9 years ago
|
||
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+
Assignee | ||
Comment 3•9 years ago
|
||
> 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.
Assignee | ||
Comment 4•9 years ago
|
||
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.
Updated•9 years ago
|
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eaa49fbb1bc2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 7•8 years ago
|
||
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•8 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(kcambridge)
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d64e928b75ac
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Comment 11•8 years ago
|
||
bugherder uplift |
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.
Description
•