609930 - Stop dispatching the unused AlertClose event

Status: RESOLVED INVALID

(Firefox for Metro Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Fennec implemented an AlertClose event, fired when notification boxes are closed. It would be nice to have it in toolkit instead of mb so that all our apps can use it (they already have an AlertOpen event).

Comment 1

[Tom Doan]

I second this. And instead of stating "New in Mobile 2" on page https://developer.mozilla.org/en/XUL/notificationbox it's more helpful to state "Only available on Mobile 2".

Comment 2

[Mark Capella [:capella]]

I found this bug while researching a patch I'm working on for Bug 752988 - Chrome Scratchpad focus bug ...

For the solution there, I'm having to add an AlertClose to the toolkit/notification.xml button ... how much of this bug does that change satisfy?

Comment 3

[Mark Capella [:capella]]

Attachment: Patch (v1)

I added the new AlertClose event to the notification box à la Fennec, and a quickie mochitest for both AlertActive and AlertClose events. Can you give this a look?

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 647436 [details] [diff] [review]
Patch (v1)

I don't understand why we should add this. The event appears to be unused in Fennec, so it seems that we should just remove it there.

Comment 5

[Mark Capella [:capella]]

Comment on attachment 647436 [details] [diff] [review]
Patch (v1)

Yes I see. The AlertClose event was fired and listened for in Bug 509408 - when getting notification asking to allow popup, it covers the url bar ...

The EventListener was removed somewhere along the line, but the dispatch was left hanging.

I'm asking :wesj in Fennec if I can remove it for code clean up and to complete this bug, as he was involved here back when.

Comment 6

[Wesley Johnston (:wesj)]

We don't use notifications.xml in Native Fennec anymore. I don't think doing code cleanup on XUL Fennec is worth it. We risk breaking something and having no idea we broke it because no one is using/testing it anymore.

Note Metro Firefox is also still using this code though:
https://mxr.mozilla.org/projects-central/source/elm/browser/metro/base/content/bindings/notification.xml#81

Removing it there might be better cleanup. Sorry to keep bumping you around :(

Comment 7

[Mark Capella [:capella]]

Interesting! I've never seen that code area before "This is the home for incomplete mozilla-central projects." ...

Maybe this patch should just be WONTFIX'ed, as Widgets doesn't want to add the AlertClose, and Fennec doesn't want to drop their AlertActive ?

Comment 8

[Mark Capella [:capella]]

Sorry, "Fennec / doesn't want to drop their AlertClose."

Comment 9

[Mark Capella [:capella]]

Dropping off this one until further clarification.

Comment 10

[Wesley Johnston (:wesj)]

Pushing this over to metro firefox and cc'ing bbondy just so you guys know this code is there... doing nothing.

Comment 11

[Wesley Johnston (:wesj)]

Adding Matt since he's back in this code!

Comment 12

[Gregory Szorc [:gps]]

I am interested in AlertClose so I can write mochitests against global-notificationbox for FHR. IMO an explicit event is much more useful than sniffing lower-level DOM events. Although poking around, it does appear there is an undocumented "Removed" event in notification.xml:169. Maybe this is part of some DOM event magic. I don't know DOM that well. I dunno.

Comment 13

[Dão Gottwald [:dao]]

"removed" isn't a DOM event. It's just a call to a function that you may add as appendNotification's last argument.

Comment 14

[Matt Brubeck (:mbrubeck)]

This is no longer unused on Metro.

Comment 15

[Dão Gottwald [:dao]]

I don't understand comment 14. http://mxr.mozilla.org/mozilla-central/search?string=%22AlertClose%22 shows that AlertClose is still around. Even if it wasn't, wouldn't that make this bug FIXED/WORKSFORME?

Comment 16

[Matt Brubeck (:mbrubeck)]

(In reply to Dão Gottwald [:dao] from comment #15)
> I don't understand comment 14.
> http://mxr.mozilla.org/mozilla-central/search?string=%22AlertClose%22 shows
> that AlertClose is still around. Even if it wasn't, wouldn't that make this
> bug FIXED/WORKSFORME?

What I said in comment 14 is that it is "no longer unused" -- i.e. it is still around, but we actually use it now, so we don't want to remove it.  I guess INVALID is a better resolution.