1392591 - browser.notifications.onClicked callback get erroneous second parameter

Status: RESOLVED FIXED

(WebExtensions :: Compatibility, enhancement, P5)

Description

[Nils Maier [:nmaier]]

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

Comment 1

[Bob Silverberg [:bsilverberg]]

Yes, this looks like a bug and a simple one to fix. I'll take it.

Comment 2

[Rob Wu [:robwu]]

This is still open, and quite simple to fix. Just remove the parameter at: https://searchfox.org/mozilla-central/rev/11712bd3ce7454923e5931fa92eaf9c01ef35a0a/toolkit/components/extensions/parent/ext-notifications.js#141

Comment 3

[Caitlin Neiman [:caitmuenster]]

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

Comment 4

[Myeongjun Go]

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

Comment 5

[Rob Wu [:robwu]]

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:

• https://searchfox.org/mozilla-central/source/toolkit/components/extensions/parent/ext-notifications.js#172 (no specific revision)
• https://searchfox.org/mozilla-central/rev/597a69c70a5cce6f42f159eb54ad1ef6745f5432/toolkit/components/extensions/parent/ext-notifications.js#172 (permalink, Searchfox)
• https://dxr.mozilla.org/mozilla-central/rev/85b4e005b39ce061e796d65ce1b2fc4e83c96a41/toolkit/components/extensions/parent/ext-notifications.js#172 (permalink, DXR)

Comment 6

[Myeongjun Go]

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

Comment 7

[Rob Wu [:robwu]]

Sure, patches are welcome.

Comment 8

[Myeongjun Go]

Attachment: Bug 1392591 - browser.notifications.onClicked callback get erroneous second parameter r=robwu

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f97cc8c93227
browser.notifications.onClicked callback get erroneous second parameter r=robwu

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/f97cc8c93227

Comment 11

[Alex Cornestean]

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!