Add a "showing" notification to PopupNotifications

RESOLVED FIXED in mozilla24



5 years ago
5 years ago


(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)



Windows 7
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(1 attachment)



5 years ago
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

5 years ago
Created attachment 762251 [details] [diff] [review]
Add a "beforeshown" notification to PopupNotifications, r?jaws
Attachment #762251 - Flags: review?(jaws)
Attachment #762251 - Flags: review?(jaws) → review+
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.
Attachment #762251 - Flags: review?(mak77)
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 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).
Attachment #762251 - Flags: review?(mak77) → review+
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

5 years ago
No, I just didn't have any context that one name was better than another. Do you want the other name?
Flags: needinfo?(mak77)
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.
Flags: needinfo?(mak77)
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".
Summary: Add a "beforeshown" notification to PopupNotifications → Add a "showing" notification to PopupNotifications

Comment 10

5 years ago
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.