381520 - nsIAlertsService doesn't fail on Mac if Growl isn't running

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Robert Accettura [:raccettura]]

nsIAlertsService should be reliable.  If I can use it, I should be confident that the user will *always* see the alerts it fires off.

Currently, if I have Growl turned off in the control panel, I can still fire an alert, yet nothing is shown.  This shouldn't happen.  nsIAlertsService shouldn't work if Growl isn't:

1.  Installed
2.  On

Comment 1

[Shawn Wilsher :sdwilsh]

Ok, so talking to the growl folks - we, as an app, can't start growl.  So, if a user stops it, we are helpless.

Now, the question is this:  Do we want to fail registration if Growl isn't running?  I don't think this is a good idea because they could restart Growl, and it wouldn't dispatch.

We could fail in showAlertNotification, but I bet most people don't try to wrap that in a try catch block.  I'm not sure what the best course of action is here...

Comment 2

[Robert Accettura [:raccettura]]

I'd say it can go two ways:

1.  Fail showAlertNotification  Before this, there was never a situation where it could fail now there is.  Anyone/think concerned should check for it.

2.  API to check if growl is running.  If false, then code can go to plan B.

Comment 3

[Shawn Wilsher :sdwilsh]

I think I prefer option one myself.  I think option two would be difficult to get approval to land.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 5

[Dave Townsend [:mossop]]

Might be worth a comment in nsIAlertsService to say that it fails in the event that an alert couldn't be displayed. At the same time you could fix the comment that claims it only works on Windows and Linux.

Comment 6

[Robert Accettura [:raccettura]]

One testcase for this is if Firefox is running, Growl is running... and alert sucessfully fires it's successful.  Then growl is turned off and another alert is sent... does it fail on showAlertNotification now with the same instance of nsIAlertsService used for both?

Comment 7

[Shawn Wilsher :sdwilsh]

It's a service, so it darn well better.  Without testing it, I see no way that it would fail (based on the code).

Comment 8

[Asaf Romano (gone)]

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

please note the posibility for this method to fail in nsIAlertService.idl; I was quite surprised to see no in-tree callers bailing out on failure.

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> (From update of attachment 265839 [details] [diff] [review])
> please note the posibility for this method to fail in nsIAlertService.idl; I
> was quite surprised to see no in-tree callers bailing out on failure.

Where did you find us using it.  The only place I know of is in nsDownloadManager.cpp, and that checks if we have the service, and will just call the function.  If it fails, it doesn't really matter.

Comment 10

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Comment 11

[Asaf Romano (gone)]

Comment on attachment 265975 [details] [diff] [review]
v1.1

r=mano (re where: mailnews/ code, which doesn't seem to check th rv either).

Comment 12

[Shawn Wilsher :sdwilsh]

Checking in toolkit/components/alerts/public/nsIAlertsService.idl;
new revision: 1.7; previous revision: 1.6
Checking in toolkit/components/alerts/src/mac/nsAlertsService.mm;
new revision: 1.7; previous revision: 1.6

Comment 13

[Ben Bucksch (:BenB)]

FWIW:

> Do we want to fail registration if Growl isn't running?

That was my assumption. (I didn't have a Mac until recently)

> We could fail in showAlertNotification, but I bet most people don't try to wrap that in a try catch block.

Indeed. I did catch the registration failure, but not showAlertNotification().
(Of course, we will fix that now.)