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

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: raccettura, Assigned: sdwilsh)

Tracking

Trunk
PowerPC
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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
(Assignee)

Comment 1

12 years ago
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...
(Reporter)

Comment 2

12 years ago
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.
(Reporter)

Updated

12 years ago
Version: unspecified → Trunk
(Assignee)

Comment 3

12 years ago
I think I prefer option one myself.  I think option two would be difficult to get approval to land.
(Assignee)

Comment 4

12 years ago
Created attachment 265839 [details] [diff] [review]
v1.0
Attachment #265839 - Flags: review?(mano)
(Assignee)

Updated

12 years ago
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.
(Reporter)

Comment 6

12 years ago
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?
(Assignee)

Comment 7

12 years ago
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)
(Assignee)

Comment 9

12 years ago
(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.
(Assignee)

Comment 10

12 years ago
Created attachment 265975 [details] [diff] [review]
v1.1
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+
(Assignee)

Comment 12

12 years ago
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
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 13

6 years ago
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.