Closed Bug 1392591 Opened 2 years ago Closed 2 months ago

browser.notifications.onClicked callback get erroneous second parameter

Categories

(WebExtensions :: Compatibility, enhancement, P5, minor)

enhancement

Tracking

(firefox70 fixed)

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: nmaier, Assigned: myeongjun.ko, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file)

A browser.notifications.onClicked callback is supposed to get called with a single parameter, (notificationId) while in fact it gets called with two parameters (notificationId, true)[0].
This seems to be a copypaste mistake (copying onClosed).

I ran into this when registering onClicked/onButtonClicked to the same listener function, expecting the second parameter to be either a button index (well, not in Firefox, no buttons yet) or undefined .


[0] https://hg.mozilla.org/mozilla-central/annotate/f91054a848d6/toolkit/components/extensions/ext-notifications.js#l135
Flags: needinfo?(bob.silverberg)
Priority: -- → P5
Yes, this looks like a bug and a simple one to fix. I'll take it.
Assignee: nobody → bob.silverberg
Flags: needinfo?(bob.silverberg)
Assignee: bob.silverberg → mstriemer
Product: Toolkit → WebExtensions
Assignee: mstriemer → nobody
Mentor: rob
Keywords: good-first-bug

If this is your first contribution, please refer to https://wiki.mozilla.org/WebExtensions/Contribution_Onramp for how to get started.

Assignee: nobody → nobody

When I see this bug, I happen one question.
I think that "onShown" parameter[0] be removed as well as "onClicked".
I checked onShown that is a single parameter. [1][2]
If you have a time, Please Check this things again :)
Thank you.

[0] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/parent/ext-notifications.js#172
[1] https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/notifications/onShown#Parameters
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/schemas/notifications.json#419

Flags: needinfo?(rob)

The second parameter to onShown can be removed too, thanks for asking.

Note: When you link to source code, try to generate a permalink. Otherwise, if the code changes, the link will point to a different location in the code, and it will be more difficult to understand the context of your comment.

Example:

Flags: needinfo?(rob)

Thank you for teaching me :)
When I make link, I gonna be careful to make it.
Can I try to work on this issue?

Flags: needinfo?(rob)

Sure, patches are welcome.

Flags: needinfo?(rob)
Assignee: nobody → myeongjun.ko
Status: NEW → ASSIGNED
Keywords: checkin-needed

Pushed by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f97cc8c93227
browser.notifications.onClicked callback get erroneous second parameter r=robwu

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

Hello,

Could you please provide a test extension and STR if this fix requires manual validation? Otherwise, could the "qe-verify-" flag be added? Thanks!

Flags: needinfo?(myeongjun.ko)
Flags: needinfo?(myeongjun.ko) → qe-verify-
You need to log in before you can comment on or make changes to this bug.