882858 - Add a "showing" notification to PopupNotifications

Status: RESOLVED FIXED

(Toolkit :: General, defect, P2)

Description

[Benjamin Smedberg]

Currently when showing a popup notification, you have to construct the entire notification when you create it, even if it's going to start out dismissed. For the plugins notification, I would like to be able to create it and populate it only when the user opens it, since populating it is a slightly involved process which walks all the plugins in a content tree and sorts/classifies them.

I can implement this by adding a "beforeshown" notification event which modifies the notification object as necessary (in this case, setting up its "centerActions" item).

Comment 1

[Benjamin Smedberg]

Attachment: Add a "beforeshown" notification to PopupNotifications, r?jaws

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 762251 [details] [diff] [review]
Add a "beforeshown" notification to PopupNotifications, r?jaws

We should get a toolkit peer to see this too though.

Comment 3

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

Not really your fault (you're reusing the existing test setup), but the test is a bit hard to follow. Adding a "// checkPopup ensures that the set of this.mainAction.label in the beforeshown handler is reflected in the shown popup" before the call to checkPopup would help.

It's also worth noting that the test only works because the mainAction object passed to show() by showNotification() is the same object referenced by this.notifyObj.mainAction (i.e. what checkPopup uses to compare), so the set of this.mainAction.label in the beforeshown handler affects both what the popup displays and what checkPopup checks for. I don't think any of these tests relied on that behavior before, but not sure the best way to point that out in a comment. I guess you could reference this.notifyObj via a closure in the beforeshown handler and also set its .mainAction.label, for clarity.

Comment 4

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

Comment on attachment 762251 [details] [diff] [review]
Add a "beforeshown" notification to PopupNotifications, r?jaws

>diff --git a/browser/base/content/test/browser_popupNotification.js b/browser/base/content/test/browser_popupNotification.js

> function checkPopup(popup, notificationObj) {
>   info("[Test #" + gTestIndex + "] checking popup");
> 
>+  ok(notificationObj.beforeShownCallbackTriggered, "beforeshown callback was triggered");
>   ok(notificationObj.shownCallbackTriggered, "shown callback was triggered");

It might be nice for the test to ensure ordering of these events (using a counter or something rather than just boolean flags), but I guess that's not a big deal (and not likely something we'd screw up).

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

is there a reason this is called beforeShown instead of showing, that is a far more common name for this kind of notification?

Comment 6

[Benjamin Smedberg]

No, I just didn't have any context that one name was better than another. Do you want the other name?

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

well, I don't *want* it at any cost, I just think, since the most common naming for popup things is "hidden", "showing", "shown", using "showing" here will be easier to memorize for consumers.

Comment 8

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

PopupNotifications event callbacks are somewhat different from normal panel events (there is no "hidden", there are other events that don't map to panels, etc.), so I don't think consistency is that important, but I don't have any objection to calling it "showing" instead of "beforeshown".

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d970c9c1fad

Comment 10

[Benjamin Smedberg]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Benjamin Smedberg]

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/PopupNotifications.jsm