Closed
Bug 1256635
Opened 8 years ago
Closed 8 years ago
Implement browser.notifications.onClicked
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox48 fixed)
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: bsilverberg, Assigned: bsilverberg)
References
Details
Attachments
(1 file)
This is documented at https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/notifications/onClicked and should be possible by listening for the "alertclickcallback" topic.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.1 - Mar 21
Assignee | ||
Comment 1•8 years ago
|
||
Note that we cannot add a real test for onClicked without mocking out the AlertService. I did test it manually and it seems to work fine. Review commit: https://reviewboard.mozilla.org/r/40133/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40133/
Attachment #8730737 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 2•8 years ago
|
||
Comment on attachment 8730737 [details] MozReview Request: Bug 1256635 - Implement browser.notifications.onClicked, r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40133/diff/1-2/
Comment 3•8 years ago
|
||
Comment on attachment 8730737 [details] MozReview Request: Bug 1256635 - Implement browser.notifications.onClicked, r?kmag https://reviewboard.mozilla.org/r/40133/#review36929 ::: toolkit/components/extensions/ext-notifications.js:55 (Diff revision 2) > } > notificationsMap.get(this.extension).delete(this.id); > }, > > observe(subject, topic, data) { > - if (topic != "alertfinished") { > + if (topic != "alertfinished" && topic != "alertclickcallback") { This shouldn't be necessary. Just add a second `if` for "alertfinished" ::: toolkit/components/extensions/ext-notifications.js:77 (Diff revision 2) > > /* eslint-disable mozilla/balanced-listeners */ > extensions.on("startup", (type, extension) => { > notificationsMap.set(extension, new Map()); > - notificationCallbacksMap.set(extension, new Set()); > + onClickedCallbacksMap.set(extension, new Set()); > + onClosedCallbacksMap.set(extension, new Set()); Typically we'd handle this by creating an EventEmitter for the extension, and then having each event listener register to receive the kind of event that it wants to know about, rather than manually dispatching callbacks for each type of event.
Attachment #8730737 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Updated•8 years ago
|
Iteration: 48.1 - Mar 21 → 48.2 - Apr 4
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8730737 [details] MozReview Request: Bug 1256635 - Implement browser.notifications.onClicked, r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40133/diff/2-3/
Attachment #8730737 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c833fe88afc4
Comment 6•8 years ago
|
||
Comment on attachment 8730737 [details] MozReview Request: Bug 1256635 - Implement browser.notifications.onClicked, r?kmag https://reviewboard.mozilla.org/r/40133/#review39899 This is going to wind up dispatching events for notifcations that belong to other extensions. The event emitters need to be per-extension, or otherwise the listeners need to filter by extension ID (I'd prefer the former).
Attachment #8730737 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8730737 [details] MozReview Request: Bug 1256635 - Implement browser.notifications.onClicked, r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40133/diff/3-4/
Attachment #8730737 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 8•8 years ago
|
||
https://reviewboard.mozilla.org/r/40133/#review39899 Ok, I believe I have fixed that.
Comment 9•8 years ago
|
||
Comment on attachment 8730737 [details] MozReview Request: Bug 1256635 - Implement browser.notifications.onClicked, r?kmag https://reviewboard.mozilla.org/r/40133/#review39911 ::: toolkit/components/extensions/ext-notifications.js:69 (Diff revisions 3 - 4) > }; > > /* eslint-disable mozilla/balanced-listeners */ > extensions.on("startup", (type, extension) => { > notificationsMap.set(extension, new Map()); > + EventEmitter.decorate(notificationsMap.get(extension)); I'd rather have something like: let map = new Map(); EventEmitter.decorate(map); notificationsMap.set(extension, map);
Attachment #8730737 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8730737 [details] MozReview Request: Bug 1256635 - Implement browser.notifications.onClicked, r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/40133/diff/4-5/
Assignee | ||
Comment 11•8 years ago
|
||
https://reviewboard.mozilla.org/r/40133/#review39911 > I'd rather have something like: > > let map = new Map(); > EventEmitter.decorate(map); > notificationsMap.set(extension, map); Heh, I thought about doing that, and anticipated a comment from you to the opposite. I guess I won't try to predict anymore and just go with my gut in the future. ;)
Assignee | ||
Comment 12•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bff8d2c117c8
Assignee | ||
Comment 13•8 years ago
|
||
Please land *after* bug 1254359
Depends on: 1254359
Keywords: checkin-needed
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/f91054a848d6
Keywords: checkin-needed
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f91054a848d6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•