394346 - Allow for named notifications with nsIAlertsService

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Shawn Wilsher :sdwilsh]

Bug 382694 didn't quite do a good job of giving an API for named notification.  After talking with mano on irc, I think we can do an optional name parameter to the existing call of showAlertNofication.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 2

[Dave Townsend [:mossop]]

I would suggest that the idl comment should not say which platforms it is currently implemented in, and should include just what a name for a notification actually means. Can only one notification with a given name display at once? can extension authors do custom styling based on the name or something?

I wonder if in the event of empty cookie you should pass through the notification? Just a thought.

Comment 3

[Asaf Romano (gone)]

As long as it's mac-only, I would include both comments in the IDL.

Comment 4

[Asaf Romano (gone)]

Comment on attachment 279346 [details] [diff] [review]
v1.0

either way, r=mano.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 279346 [details] [diff] [review]
v1.0

I'll fix the comments (and post a patch) before checkin

Comment 6

[Phil Ringnalda (:philor)]

Getting this out of the approval triage hole, and into Toolkit: General ;)

Comment 7

[Shawn Wilsher :sdwilsh]

Fixed comments on check-in.

Checking in toolkit/components/alerts/public/nsIAlertsService.idl;
new revision: 1.8; previous revision: 1.7
Checking in toolkit/components/alerts/src/nsAlertsService.cpp;
new revision: 1.7; previous revision: 1.6
Checking in toolkit/components/alerts/src/mac/nsAlertsService.h;
new revision: 1.6; previous revision: 1.5
Checking in toolkit/components/alerts/src/mac/nsAlertsService.mm;
new revision: 1.9; previous revision: 1.8
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.122; previous revision: 1.121

Comment 8

[Andrew Schultz]

FYI, camino required additional bustage fix here since it still uses the xpfe alerts service (and the now-updated xpfe download manager).

Comment 9

[Eric Shepherd [:sheppy]]

Documented:

http://developer.mozilla.org/en/docs/nsIAlertsService

Comment 10

[Shawn Wilsher :sdwilsh]

That documentation has some flaws:

1) the yellow box at the top has several bits of wrong information
2) it has changed in 1.9, so the comment about it being unfrozen and not changing since 1.7 is wrong
3) the example should have one for pre 1.9, and one for post 1.9 that takes advantage of the optional arguments (that can be omitted from JS callers).

I'll fix these if I get some time, but I don't expect that to be anytime soon.

Comment 11

[Eric Shepherd [:sheppy]]

Would be helpful if someone could specify what the wrong bits of information are, since I simply updated the text that was already on the page to mention that it works on Mac OS X in Fx3. :)

Comment 12

[Shawn Wilsher :sdwilsh]

Sorry for the delay on this.

Stuff that's wrong:
-All the optional flag stuff is new to 1.9.  Prior to it, all those arguments were required (except for the new one that was added).
-Generally the service will not instantiate on OS X if Growl isn't installed.  It also does a runtime check on the off chance that someone uninstalls Growl while the program is running.
-showAlertNotification will throw NS_ERROR_NOT_AVAILABLE if Growl is not running on OS X as well.

Comment 13

[Peter Van der Beken [:peterv]]

Comment on attachment 279346 [details] [diff] [review]
v1.0

>+  if (!aAlertName.IsEmpty()) {
>+    return DispatchNamedNotification(aAlertTitle, aImageUrl, aAlertTitle,
>+                                     aAlertText, aAlertCookie, aAlertListener);
>+  }

Shouldn't we pass aAlertName as the name here (and not aAlertTitle)?

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #13)
> (From update of attachment 279346 [details] [diff] [review])
> >+  if (!aAlertName.IsEmpty()) {
> >+    return DispatchNamedNotification(aAlertTitle, aImageUrl, aAlertTitle,
> >+                                     aAlertText, aAlertCookie, aAlertListener);
> >+  }
er, yes!  We should probably file a new bug to get that fixed...

Comment 15

[Shawn Wilsher :sdwilsh]

(In reply to comment #14)
> er, yes!  We should probably file a new bug to get that fixed...
Filed bug 448054