Closed Bug 381520 Opened 17 years ago Closed 17 years ago

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

Categories

(Firefox :: Shell Integration, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: raccettura, Assigned: sdwilsh)

Details

Attachments

(1 file, 1 obsolete file)

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
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...
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.
Version: unspecified → Trunk
I think I prefer option one myself.  I think option two would be difficult to get approval to land.
Attached patch v1.0 (obsolete) — Splinter Review
Attachment #265839 - Flags: review?(mano)
Summary: nsIAlertsService doesn't fail on Mac if Growl isn't installed → nsIAlertsService doesn't fail on Mac if Growl isn't running
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.
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?
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 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.
Attachment #265839 - Flags: review?(mano)
(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.
Attached patch v1.1Splinter Review
Attachment #265839 - Attachment is obsolete: true
Attachment #265975 - Flags: review?(mano)
Comment on attachment 265975 [details] [diff] [review]
v1.1

r=mano (re where: mailnews/ code, which doesn't seem to check th rv either).
Attachment #265975 - Flags: review?(mano) → review+
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
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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.)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: