add Growl support back

VERIFIED FIXED in Firefox 19

Status

()

Toolkit
General
VERIFIED FIXED
5 years ago
2 months ago

People

(Reporter: MattN, Assigned: Josh Aas)

Tracking

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

Firefox Tracking Flags

(firefox19+ verified, firefox20+ verified, firefox21+ verified)

Details

Attachments

(2 attachments)

+++ This bug was initially created as a clone of Bug #805837 +++

My understanding is that XUL notifications will come from bug 782211 but that has still not been completed. I think we should add growl back to 19 and 20 for the same reason as we did for 18.

(Quoting Josh Aas (Mozilla Corporation) from bug 805837)
Our XUL notification support, and native Notification Center support, should be getting much better in Firefox 19. I removed Growl in Firefox 18, leaving OS X users with only a basic Notification Center implementation for alerts. I think we should add Growl back to Firefox 18 so there is no gap between when we remove Growl and significantly improved alert support.
We should probably actually re-add it on trunk too, and file a bug that depends on bug 782211 to remove it again, so we stop having to go through this dance after merges.

Comment 3

5 years ago
Josh - can we get this fixed Monday in preparation for FF19.0b2?
tracking-firefox19: ? → +
tracking-firefox20: ? → +
(Assignee)

Comment 4

5 years ago
I am planning to work on this today but I doubt I can finish today. The backout is much more complicated than for previous releases due to other code changes.

Comment 5

5 years ago
Spoke with Josh - this isn't going to make b2 but is targeted for b3 (unless conversations around deprecation of the feature pan out). We discussed risk - it should be limited to notifications.
(Assignee)

Comment 6

5 years ago
I added Growl support back to Beta (I'll post a patch later), and in order to test it I bought the latest version of Growl from the Mac App Store, for OS X 10.8. I couldn't get notifications to show up for my patched copy of Beta or the currently released Firefox 18. Is our Growl support not compatible with Growl 2+, in general?
(Assignee)

Comment 7

5 years ago
Created attachment 704070 [details] [diff] [review]
fix v1.0

This compiles on Beta and the resulting browser runs, but I can't get growl to actually work on 10.8 with my Beta build or Firefox 18, with either the old Growl 1.2.2 or the new Growl 2.0.1.
(In reply to Josh Aas (Mozilla Corporation) from comment #6)
> Is our Growl support not compatible with Growl 2+, in general?

It works for me with Growl 1.3 and Growl 2.0.1 on Firefox 18 (release) on OS X 10.7.  I can test on 10.8 on my personal machine on the weekend.

Have you double-checked that you haven't disable Growl notifications for Firefox in Growl preferences?
Status: NEW → ASSIGNED
(Assignee)

Comment 9

5 years ago
(In reply to Matthew N. [:MattN] from comment #8)

> Have you double-checked that you haven't disable Growl notifications for
> Firefox in Growl preferences?

I checked, Firefox notifications were not disabled.
(In reply to Josh Aas (Mozilla Corporation) from comment #7)
> Created attachment 704070 [details] [diff] [review]
> fix v1.0
> 
> This compiles on Beta and the resulting browser runs, but I can't get growl
> to actually work on 10.8 with my Beta build or Firefox 18, with either the
> old Growl 1.2.2 or the new Growl 2.0.1.

If you're not able to get Growl working with FF18/10.8 either, then this wouldn't be a regression and can be uplifted (especially given Matt's positive verification). Can we get this reviewed and uplifted today?

While this is in Growl and Mac alerts code only, we should still get this into a beta asap to get as much regression testing as possible.
Matt - can you verify that this patch tests OK for you?
Flags: needinfo?(mnoorenberghe+bmo)
(Assignee)

Comment 12

5 years ago
(In reply to Alex Keybl [:akeybl] from comment #10)

> If you're not able to get Growl working with FF18/10.8 either, then this
> wouldn't be a regression and can be uplifted (especially given Matt's
> positive verification). Can we get this reviewed and uplifted today?

Yeah, my testing doesn't show a regression but I need to see that my patch does actually work somewhere before landing it, and I can't test that right now. Hopefully MattN can.
The patch WFM applied on m-c with Growl 1.3 & 2.0 on 10.7.  My beta tree is too old at the moment and I'm currently tethering so I can't update it at the moment.
Flags: needinfo?(mnoorenberghe+bmo)
(In reply to Josh Aas (Mozilla Corporation) from comment #12)
> (In reply to Alex Keybl [:akeybl] from comment #10)
> 
> > If you're not able to get Growl working with FF18/10.8 either, then this
> > wouldn't be a regression and can be uplifted (especially given Matt's
> > positive verification). Can we get this reviewed and uplifted today?
> 
> Yeah, my testing doesn't show a regression but I need to see that my patch
> does actually work somewhere before landing it, and I can't test that right
> now. Hopefully MattN can.

In lieu of positive verification today, can we get a try build going to hand off to QA? We'd land on Aurora this week and uplift to FF19b4 for next week in that case.
Patch works with the beta build as well using Growl 1.3 & 2.0 on 10.7.  I will test on 10.8 now.
Note that I have only been testing the download completed notifications.

10.8 results:
* Notification is shown for Growl 1.2
* Notification is shown for Growl 2 (with & without Notification Center forwarding)

Seems like download notifications are working on 10.7 and 10.8 on beta with this patch.
Thanks so much MattN. Josh - can we move forward with landing to central/aurora in preparation for uplift Monday/Tuesday of next week?
status-firefox21: --- → affected
tracking-firefox21: --- → +
(Assignee)

Comment 18

5 years ago
Comment on attachment 704070 [details] [diff] [review]
fix v1.0

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 777409
User impact if declined: no growl
Testing completed (on m-c, etc.): not much
Risk to taking this patch (and alternatives if risky): moderate but contained in alerts
String or UUID changes made by this patch: none

I assume I can take akeybl's comment as a+ but we should get it marked.
Attachment #704070 - Flags: approval-mozilla-beta?
Attachment #704070 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 19

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

Patch for Aurora is slightly different.
Comment on attachment 704070 [details] [diff] [review]
fix v1.0

Hi Josh - I'm approving for branches, but I'm wondering why we're not landing this to mozilla-central as well. I know you'd mentioned that other alerts may take the place of Growl entirely, but unless that's a sure thing that everybody already agrees on (I think Doug disagreed?) we should land to all branches.
Attachment #704070 - Flags: approval-mozilla-beta?
Attachment #704070 - Flags: approval-mozilla-beta+
Attachment #704070 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #706097 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 21

5 years ago
pushed to mozilla-aurora

http://hg.mozilla.org/releases/mozilla-aurora/rev/95a748d5cf96
status-firefox20: affected → fixed
(Assignee)

Comment 22

5 years ago
I'm going to assume I have binary approval for the fact that this adds a new idl file to the beta tree. Let me know if that's a bad assumption.
(Assignee)

Comment 23

5 years ago
pushed to beta:

http://hg.mozilla.org/releases/mozilla-beta/rev/e11e8be9a609
status-firefox19: affected → fixed

Updated

5 years ago
Blocks: 835002
(Assignee)

Comment 24

5 years ago
pushed to mozilla-inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/d9bc099c9486
https://hg.mozilla.org/mozilla-central/rev/d9bc099c9486
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox21: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Summary: add Growl support back for Firefox 19 (Beta) and Firefox 20 (Aurora) → add Growl support back
Keywords: verifyme
Notifications are shown on Growl 1.2.2.
Verified on MAC OS X 10.8 using Firefox 19 RC
Build ID: 20130215130331
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/20100101 Firefox/19.0
status-firefox19: fixed → verified
Notifications are also shown for Firefox 20 Beta 1 using Growl 1.2.2.

Build ID: 20130220104816
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
status-firefox20: fixed → verified
Verified as fixed on Firefox 21 beta 2 using Growl 1.2.2

Build ID:20130401192816
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20100101 Firefox/21.0
Status: RESOLVED → VERIFIED
status-firefox21: fixed → verified

Comment 29

5 years ago
This seems to be broken again in Firefox 22? I'm not getting Growl notifications for downloads being complete.
(In reply to Brian L. Matthews from comment #29)
> This seems to be broken again in Firefox 22? I'm not getting Growl
> notifications for downloads being complete.

Can you please open a new bug? If Growl was updated recently I suppose it's possible a regression was introduced, but the code which landed here does exist in Firefox 22. This should be investigated further in a new bug report.

Thank you.
Keywords: verifyme
No need to file a new bug, the change was intentional as part of bug 782211. See bug 777409 comment 45.
Thanks Gavin.

Brian, as Gavin has pointed out we dropped Growl support starting in Firefox 22 via bug 777409.
You need to log in before you can comment on or make changes to this bug.