597244 - re-work PopupNotifications event callback API

Status: VERIFIED FIXED

(Toolkit :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Bug 590794 added a "dismissalHandler", and bug 595253 has a patch adding a "shown" handler. A "removed" handler is potentially useful as well. Rather than have the caller specify separate callbacks for all of these events, we should consolidate and have a single "eventHandler" callback that gets called for each of the events, with a parameter. Bug 590794 doesn't have any users yet (was just added recently), so I think we can make this change without affecting any users (but we need to do it soon).

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 2

[Justin Dolske [:Dolske]]

blocking+ since this is an API change.

Comment 3

[Sylvain Pasche]

If I understand correctly, the "removed" event won't be called when you close a tab or the window.

In bug 588266 a cleanup function (cancel add-on installation) needs to be called if the user didn't click on the main action of the notification.
As of now, the cleanup function in called from the dismissal callback introduced in bug 590794, but there's a drawback: the notification is removed when dismissed, so you can't dismiss it and install the add-on later.

Using the "removed" event would fix that issue, but it would need to be fired in all situations where the user didn't accept the notification (tab or window is closed, location change, ...). I could open a followup bug if that's reasonable to have such semantic for the "removed" event.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Yeah, please do file a followup. We may be able to fix that by having browsers remove() their notifications in their destructor, or something along those lines.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/c69173773282

Comment 6

[Sylvain Pasche]

(In reply to comment #4)
> Yeah, please do file a followup. We may be able to fix that by having browsers
> remove() their notifications in their destructor, or something along those
> lines.

I filed bug 609804 for that.