Closed Bug 827979 Opened 7 years ago Closed 7 years ago

add Growl support back

Categories

(Toolkit :: General, defect)

All
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla21
Tracking Status
firefox19 + verified
firefox20 + verified
firefox21 + verified

People

(Reporter: MattN, Assigned: jaas)

References

Details

Attachments

(2 files)

+++ 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.
Josh - can we get this fixed Monday in preparation for FF19.0b2?
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.
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.
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?
Attached patch fix v1.0Splinter Review
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
(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)
(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?
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?
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?
Attachment #706097 - Flags: approval-mozilla-aurora+
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.
Blocks: 835002
https://hg.mozilla.org/mozilla-central/rev/d9bc099c9486
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Summary: add Growl support back for Firefox 19 (Beta) and Firefox 20 (Aurora) → add Growl support back
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
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
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
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.