Closed Bug 1256635 Opened 5 years ago Closed 5 years ago

Implement browser.notifications.onClicked

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Iteration:
48.2 - Apr 4
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: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.1 - Mar 21
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)
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 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)
Iteration: 48.1 - Mar 21 → 48.2 - Apr 4
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)
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)
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)
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+
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/
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. ;)
Please land *after* bug 1254359
Depends on: 1254359
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f91054a848d6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Blocks: 1392591
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.