Closed Bug 777409 Opened 12 years ago Closed 12 years ago

remove growl support

Categories

(Toolkit :: General, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox18 --- disabled

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(1 file, 4 obsolete files)

I think it's time to seriously consider removing Growl support. It has a history of performance, security, and compatibility problems. It's also a non-trivial addition to our code size.

Mac OS X 10.8 was just released with support for notifications and we just remove support for Mac OS X 10.5 in Firefox 17. The former gives us a replacement for growl. The latter reduces the number of growl supporters impacted by this move.

This functionality can probably be put into an extension. I don't think Chrome or Safari support growl.

I'll post more data about what this change would mean as I look into it.
Diffstat for a removal patch: 35 files changed, 1 insertion(+), 5242 deletions(-)
Attached patch fix v1.0 (obsolete) — Splinter Review
What about OS X 10.6 and 10.7? What are we planning to do there? And how is the alerts service affected?

I know the web apps implementation is currently using the alerts service to fire notifications when apps are successfully installed. For Mac, it's currently using Growl I believe.
(In reply to Jason Smith [:jsmith] from comment #4)

> I know the web apps implementation is currently using the alerts service to
> fire notifications when apps are successfully installed. For Mac, it's
> currently using Growl I believe.

That would only matter for the small percentage of people who have growl installed. It's not a default feature of Firefox, you have to install a third-party product. We can use telemetry to figure out how many people that is.
Attached patch fix v1.1 (obsolete) — Splinter Review
Fixes a packaging error.
Attachment #645810 - Attachment is obsolete: true
Try server run:

https://tbpl.mozilla.org/?tree=Try&rev=7d666845163e

Equivalent build without the patch for binary size comparison:

https://tbpl.mozilla.org/?tree=Try&rev=5f13916ef56d
45067k unpatched, 45016k patched. Saves ~51k.
(In reply to Josh Aas (Mozilla Corporation) from comment #8)
> 45067k unpatched, 45016k patched. Saves ~51k.

I should have mentioned that is the download dmg sizes. On disk the unpatched app is 149,008,301 bytes (149.3 MB on disk), the patched app is 148,810,149 bytes (149.1 MB on disk).

I'm not sure this is a huge argument for doing this but it is a pretty good sized chunk of code and I was curious to see what removing ~5k lines of code per binary would do.
I am unconvinced that removing a working feature from the product that matters to some percentage of our 10.6 and 10.7 users was necessary.  Some third-party apps not only depend on Growl, but automatically install it.  Adobe, Dropbox, etc.
I'm not convinced yet either, but I do think we should consider it. We probably wouldn't add this feature if we didn't already have it. Also, the number of people who have growl installed != number of people who have it and care that Firefox integrates.
Presumably we'd switch to the toaster notification that we use on other platforms?
Outside of the issue in bug 759780, I'm not a fan of this. I agree that we should probably use NC if possible (bug 728106) but I think Growl is a far better option than nothing for < 10.8. If there are problems with our Growl implementation beyond LOC, we should fix that (and/or use a newer version of Growl).

I especially don't think we should consider taking this before we add support for Notification Center.
Can we please synchronize this effort with providing an extension which implements Growl (via ctypes?)? This way, nobody should be able to complain.
(In reply to Josh Aas (Mozilla Corporation) from comment #11)
> We probably wouldn't add this feature if we didn't already have it.

While on the other hand you are working on adding new code for Notification Center in bug 728106?

I would rather see better support for notifications via Mist (or at least the XUL notifications) for < 10.8 instead of killing it.  We will need a way to show notifications to implement bug 629280.
(In reply to Matthew N. [:MattN] from comment #15)
> (In reply to Josh Aas (Mozilla Corporation) from comment #11)
> > We probably wouldn't add this feature if we didn't already have it.
> 
> While on the other hand you are working on adding new code for Notification
> Center in bug 728106?

They aren't the same. One has Apple backing it, the other is an unknown quantity so far as I know (e.g. security, stability). Also, one is a minimal amount of code, the other is 5000+ lines of third-party code with little oversight and a history of problems.
You need to consider that Thunderbird is also using the Growl support, for new mail notifications and for the new chat feature. So if you remove Growl completely users of 10.6 and 10.7 won't get any notifications anymore. Some alternative for this would be highly desirable than.
(In reply to Matthew N. [:MattN] from comment #15)

> I would rather see better support for notifications via Mist (or at least
> the XUL notifications) for < 10.8 instead of killing it.  We will need a way
> to show notifications to implement bug 629280.

W3C Notifications is a more compelling reason for me to re-consider my position. I'll think it over when I've finished notification center integration and had time to look at the spec and our support plans.
Attached patch fix v1.2 (obsolete) — Splinter Review
Updated to current trunk, which includes Notification Center support.

We've decided not to use Growl for W3C Web Notifications and that being the case, we don't need Growl any more. We'll be investing in improved XUL-based notifications.

I'm going to hold off on removing Growl until Firefox 18.
Attachment #645847 - Attachment is obsolete: true
Attached patch fix v1.3 (obsolete) — Splinter Review
Attachment #654659 - Attachment is obsolete: true
Attachment #658199 - Flags: review?(doug.turner)
> We've decided not to use Growl for W3C Web Notifications and that being the case, we don't need Growl any more.

I do not think that we have made any decision wrt the Notification Center.
Attachment #658199 - Flags: review?(doug.turner) → review+
(In reply to Doug Turner (:dougt) from comment #21)
> > We've decided not to use Growl for W3C Web Notifications and that being the case, we don't need Growl any more.
> 
> I do not think that we have made any decision wrt the Notification Center.

Did you mean to say that we haven't made a decision wrt Web Notifications?
Yes.  Your statement was about using Growl.  I don't think we have had any conversation about using the Notification Center to back w3c web notifications.
(In reply to Doug Turner (:dougt) from comment #23)

My understanding per another discussion was that we are not planning to use Notification Center or Growl for W3C Web Notifications - we are going to use XUL. All that really matters for this bug is that we're not going to use Growl, which everyone I talked to is in agreement about. If we choose to try to implement with Notification Center instead that's fine, and it isn't impacted by this removal of Growl.

That being the case, I'm planning to land this tomorrow.
sounds good!
Still seems like a step backwards to remove this and the click events especially since the Growl 2.0 SDK is now released with Notification Center support and allows growl-like notifications to work via Mist even if a user hasn't installed Growl. https://growl.posterous.com/developers-growl-20-sdk-released
try run for latest patch:

https://tbpl.mozilla.org/?tree=Try&rev=f38bcab57abf
The alert service tests fail on OS X < 10.8, so they'll need to be disabled. Also, "test_bug_765063.xul" fails, probably because it'll fail if there is no alert service.
test_bug_765063.xul:

Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash address: 0x0

Thread 0 (crashed)
 0  libalerts.dylib!nsMacAlertsService::ShowAlertNotification [nsMacAlertsService.mm : 153 + 0x4]
    rbx = 0x5fbf7e48   r12 = 0x5fbf7e60   r13 = 0x5fbf7e60   r14 = 0x1e904940
    r15 = 0x1e904920   rip = 0x053be12b   rsp = 0x5fbf72a0   rbp = 0x5fbf72d0
    Found by: given as instruction pointer in context
 1  XUL!nsAlertsService::ShowAlertNotification [nsAlertsService.cpp : 99 + 0x23]
    rip = 0x01b6d671   rsp = 0x5fbf72e0
    Found by: stack scanning
wait - don't we fall back to the XUL dialog?
My guess based on these test failures is that we do not, but I haven't tested yet. I know that I never saw any alert when I used 10.7, and I never had Growl installed, so my guess is that we don't fall back to XUL on Mac.

With this patch we'd likely just have no alert service on 10.6 and 10.7, which is what happened if you didn't have Growl installed on your machine.
Attached patch fix v1.4Splinter Review
I think I found the bug causing the non-obvious test failure. We were returning an OK status code even when we failed to initialize any Mac alert service.

https://tbpl.mozilla.org/?tree=Try&rev=c149363495a5
(In reply to Doug Turner (:dougt) from comment #30)
> wait - don't we fall back to the XUL dialog?

Nope. IIRC the XUL dialog didn't even work on OSX in the way back when since we didn't have panels or something. I imagine things are probably better now.
so, that means for the people that use growl today (or use Hiss+Notification Center), this patch will basically make them lose functionality.

I am not sure I am okay with that.  What is the rush to remove this now?
I think the reasons listed in the bug summary here are good enough to remove the feature. Growl is a lot of code to support for a feature that isn't turned on for the vast majority of our users, and we don't maintain it well.

Check out bug 759780 for a more specific example of a major problems with Growl. It has a chromehang profile showing Growl doing main thread i/o, blocking the main thread for two seconds.
Attachment #658199 - Attachment is obsolete: true
Attachment #660468 - Flags: review?(doug.turner)
Attachment #660468 - Flags: review?(doug.turner) → review+
Blocks: 759780
https://hg.mozilla.org/mozilla-central/rev/9d0128d60f9a
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 792793
Blocks: 793432
(In reply to Josh Aas (Mozilla Corporation) from comment #35)
> Growl is a lot of code to support for a feature that isn't
> turned on for the vast majority of our users, and we don't maintain it well.

Do we have data for the "vast majority" claim, or is that just speculation?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #38)

> Do we have data for the "vast majority" claim, or is that just speculation?

We don't have data, but I'm quite confident in my speculation. Even if we had that data, we can't assume that the people who have growl installed care about what Firefox does with it. I am running with notifications now and I hardly ever see them, when I do it isn't really adding much to the experience (download finished).

Work has started on XUL-based notifications so we should have that for 10.6 and 10.7 users soon anyway.
(In reply to Josh Aas (Mozilla Corporation) from comment #39)
> (In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment
> #38)
> 
> > Do we have data for the "vast majority" claim, or is that just speculation?
> 
> We don't have data, but I'm quite confident in my speculation. Even if we
> had that data, we can't assume that the people who have growl installed care
> about what Firefox does with it. I am running with notifications now and I
> hardly ever see them, when I do it isn't really adding much to the
> experience (download finished).

Remember this is toolkit code, so getting notifications in other apps is a bit more important. If I had an email app that suddenly stopped getting new mail notifications, I'd be pissed. Or even my browser that used to notify me of new gmail (we have millions of users of mail notification addons). I know it's only 10.5/10.6/10.7 users, but (and I'm speculating quite confidently) that's a non-trivial portion of OS X users across toolkit apps.

> Work has started on XUL-based notifications so we should have that for 10.6
> and 10.7 users soon anyway.

Honestly, I think that work should have been finished before we removed growl support. If that will make it into 18 along with this... fine.
> Honestly, I think that work should have been finished before we removed growl support.

I'm in that camp as well (mulling over adding it back to TenFourFox). However, I cannot find the XUL Notifications bug.
I am in the camp to add it back, I miss it already!
I think we should add Growl back for one more release, Firefox 18. We'll leave it out for Firefox 19, where we'll have much-improved XUL and native alert support. This will eliminate a gap with lesser alert support. Filed bug 805837.
I just downloaded Firefox 22 on my 10.6.8 system and was quite shocked to learn that Growl support is gone. First I thought it's probably some Prefs settings, then probably an about:config setting. But none exist. So assumed it's a bug, which landed me here.

By XUL-notifications I assume everyone is talking about those unaesthetic white boxes that pop up now? Is there theming support for those?

The OP talks about moving Growl support to an extension... I have never written an extension so can I just use the removed Growl code as a starting point?

(PS: as one of the major reasons for removing a perfectly fine working feature was code size... was that intended as serious point or mocking? I can't tell because 5k LOC in a product such as this is nothing?)
(this ended up being delayed by bug 805837 and bug 827979 to Firefox 22, where it was removed as part of bug 782211)
Target Milestone: mozilla18 → mozilla22
Thanks for the insight.

I totally get that native notification centers should be used if an OS has them. But why disable them for an inferior replacement on 10.6 and 10.7?

So it's gone now. 

Well, if anyone has some knowledge if this can be added back in with an add-on and could just drop a line about it I might have a look and see if can sink some time in this. No point in spending a couple days learning the interna of writing FF add-ons just to find out nope can't be done.

Thanks.
the path forward is to move to notification center.  Josh said he was close.
(In reply to Doug Turner (:dougt) from comment #47)
> the path forward is to move to notification center.  Josh said he was close.

10.6 and 10.7 don't HAVE notification centre. Growl was indispensable for being able to see which mail account was receiving mail from from whom. Now if I'm working on another virtual desktop and mail comes in, I have no choice but to go and scan the inboxes of a whole whack of accounts to see what came in where. Why? Because the current notifications are no longer system wide and do not display when SeaMonkey does not have focus.

What a monumental step backwards for those of us who are still on 10.6.8 with Growl 1.2.2. Not happy. A better solution would have been to use notification centre if available and fall back to Growl if installed. That would have maintained existing features while moving forward.

Bah.
Yeah, not too happy either. 

You should get those white notification boxes though like they do pop up on a Windows system... XUL-Notifcations as they were called?
For me they do pop up on desktop without focus when the program is minimized so they provide the bare minimum to make it work, but yeah... not too happy. That's Firefox though. I don't know how it is for SeaMonkey.

No idea why there wasn't a backwards compatible solution.
(In reply to Andre B from comment #49)
> Yeah, not too happy either. 
> 
> You should get those white notification boxes though like they do pop up on
> a Windows system... XUL-Notifcations as they were called?
> For me they do pop up on desktop without focus when the program is minimized
> so they provide the bare minimum to make it work, but yeah... not too happy.
> That's Firefox though. I don't know how it is for SeaMonkey.
> 
> No idea why there wasn't a backwards compatible solution.

I get notifications for sure when SeaMonkey has focus. The XUL-based notifications do work, but with limitations:

* If a Growl notification is on screen, the XUL notification from SeaMonkey will cover it.
* If SeaMonkey is open on Desktop 2 and I'm working on Desktop 1, XUL notifications might very well be displayed, but are not visible on this 'other' desktop.

I haven't checked whether notifications are displayed on the same desktop when SeaMonkey doesn't have focus. And it's a conversation for a different bug, so I suppose I should stop ranting here. I'll simply end by saying that removing features from the code base that has been in place for years makes financial contributors REALLY grumpy. I'm on legacy hardware here that EOLs at 10.7, so unless I roll back my SeaMonkey install and freeze my updates -- not an intelligent security decision -- I'm now forever doomed to enjoying a SeaMonkey that feels more separate from OS X than it has since the 1.x days.

Way to go, guys.
Addendum 1: The XUL notifications do a most excellent job of covering up Growl notifications. Due to opacity of the XUL notification, the Growl bubble is lost completely.

Addendum 2: When dragging mail to a folder, a XUL notification causes the drag to lose focus and fail.

Addendum 3: Confirmed that XUL notifications are only visible on the desktop on which SeaMonkey (or other Gecko app) is running. So, when I'm working on any of the other 5 of my 6 desktops, I don't see any notifications at all.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: