remove growl support

RESOLVED FIXED in mozilla22

Status

()

Toolkit
General
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

Trunk
mozilla22
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox18 disabled)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

5 years ago
Diffstat for a removal patch: 35 files changed, 1 insertion(+), 5242 deletions(-)
(Assignee)

Comment 2

5 years ago
Created attachment 645810 [details] [diff] [review]
fix v1.0
(Assignee)

Comment 3

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=738d52a9a571
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.
(Assignee)

Comment 5

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

Comment 6

5 years ago
Created attachment 645847 [details] [diff] [review]
fix v1.1

Fixes a packaging error.
Attachment #645810 - Attachment is obsolete: true
(Assignee)

Comment 7

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

Comment 8

5 years ago
45067k unpatched, 45016k patched. Saves ~51k.
(Assignee)

Comment 9

5 years ago
(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.

Comment 10

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

Comment 11

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

Comment 14

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

Comment 16

5 years ago
(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.

Comment 17

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

Comment 18

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

Comment 19

5 years ago
Created attachment 654659 [details] [diff] [review]
fix v1.2

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
(Assignee)

Comment 20

5 years ago
Created attachment 658199 [details] [diff] [review]
fix v1.3
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.

Updated

5 years ago
Attachment #658199 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 22

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

Comment 24

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

Comment 27

5 years ago
try run for latest patch:

https://tbpl.mozilla.org/?tree=Try&rev=f38bcab57abf
(Assignee)

Comment 28

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

Comment 29

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

Comment 31

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

Comment 32

5 years ago
Created attachment 660468 [details] [diff] [review]
fix v1.4

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

Comment 35

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

Updated

5 years ago
Attachment #658199 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #660468 - Flags: review?(doug.turner)

Updated

5 years ago
Attachment #660468 - Flags: review?(doug.turner) → review+
(Assignee)

Comment 36

5 years ago
pushed to mozilla-inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/9d0128d60f9a

Updated

5 years ago
Blocks: 759780
https://hg.mozilla.org/mozilla-central/rev/9d0128d60f9a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 792793

Updated

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

Comment 39

5 years ago
(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.

Comment 42

5 years ago
I am in the camp to add it back, I miss it already!
(Assignee)

Comment 43

5 years ago
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.
status-firefox18: --- → disabled
Blocks: 852461
No longer blocks: 852461

Comment 44

4 years ago
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

Comment 46

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

Comment 48

4 years ago
(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.

Comment 49

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

Comment 50

4 years ago
(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.

Comment 51

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