522511 - Update bundled Growl to 1.2 (for nsIAlertsService on OS X)

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

Some files from the Growl source distro are bundled in
toolkit/components/alerts/src/mac/growl, to provide support for
nsIAlertsService on OS X.  These files are from various places in the
Growl distro, including the following directories -- Framework/Source,
Common/Source and Core/Source.  A few contain some very minor
modifications from the original Growl code.

On the 1.9.1 and 1.9.2 branches and the trunk, these files
(originally) came from the Growl 1.1.4 distro (see bug 421551) On the
1.9.0 branch they came (as best I can tell) from the Growl 0.7 distro
(see bug 362685).  Growl source has a browseable hg repository at
http://code.google.com/p/growl/source/browse/.

The bundled Growl files need to be upgraded to version 1.2 to fix bug
510740 and bug 515396.  I'll post a patch in my next comment.

Comment 1

[Shawn Wilsher :sdwilsh]

I don't recall us making any modifications to the source - what are those?

Comment 2

[Steven Michaud [:smichaud] (Retired)]

I had to add the following line to GrowlPathUtilities.mm.  So did
dtownsend when he upgraded to 1.1.4.

  #import <Cocoa/Cocoa.h>

Growl 0.7's CFGrowlAdditions.c also needed a small change to compile
on OS X 10.5 (see bug 384162).

There may be other changes that I missed, but I doubt they're many.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Attachment: Patch (upgrade Growl to version 1.2)

Here's my patch.  I just replaced all existing Growl files in
toolkit/components/alerts/src/mac/growl with newer versions that I
found in the Growl 1.2 source distro's Framework/Source, Common/Source
and Core/Source directories.  I also (as I mentioned above) made one
small edit to GrowlPathUtilities.mm.

For the record, I got the Growl 1.2 source distro from
http://growl.googlecode.com/files/Growl-1.2-src.tbz.

A tryserver build should follow in an hour or so.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my patch:
https://build.mozilla.org/tryserver-builds/[e-mail]-bugzilla522511/bugzilla522511-macosx.dmg

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Growl hides the source archives for versions earlier than the current
one.  Very annoying.  But here are some URLs I've found, which are
still current:

http://growl.info/files/source/Growl-0.7.4-src.tar.bz2
http://growl.info/files/source/Growl-0.7.5-src.tar.bz2
http://growl.info/files/source/Growl-0.7.6-src.tar.bz2
http://growl.info/files/source/Growl-1.1.3-src.tar.bz2
http://growl.info/files/source/Growl-1.1.4-src.tar.bz2
http://growl.info/files/source/Growl-1.1.5-src.tar.bz2
http://growl.info/files/source/Growl-1.1.6-src.tar.bz2

Comment 6

[Steven Michaud [:smichaud] (Retired)]

> Growl hides the source archives for versions earlier than the
> current one.

Actually this isn't quite true.  Older versions aren't accessible
following links from http://www.growl.info.  But the following URL
works (the contents of the directory aren't hidden):

http://growl.info/files/source/

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Older Growl versions' binaries are available at http://growl.info/files/.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

I just realized that we can only do this upgrade on trunk.

On the branches we need to preserve compatibility with OS X 10.4, and Growl claims that Growl 1.1.6 is needed for that.

Comment 9

[Shawn Wilsher :sdwilsh]

I guess that means that on branch we'll have to backport their fix?

Comment 10

[Steven Michaud [:smichaud] (Retired)]

I suppose so.  I'll play with that tomorrow.

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 406518 [details] [diff] [review]
Patch (upgrade Growl to version 1.2)

r=sdwilsh

Think we can get the minor changes upstream by chance to make future upgrades easier?

Comment 12

[Steven Michaud [:smichaud] (Retired)]

> I just realized that we can only do this upgrade on trunk.

I've found out this is wrong.

> On the branches we need to preserve compatibility with OS X 10.4,
> and Growl claims that Growl 1.1.6 is needed for that.

What we bundle of Growl 1.2 works just fine on OS X 10.4.11.  All
that's needed is one additional edit, in GrowlPreferencesController.h:

 @interface GrowlPreferencesController : GrowlAbstractSingletonObject {
-       LSSharedFileListRef loginItems;
+       void *loginItems;
 }

LSSharedFileListRef is new with OS X 10.5 (along with other stuff
defined in LSSharedFileList.h).  But none of this is used in the files
we bundle, which contain no other references to the loginItems member
variable of the GrowlPreferencesController class.

So all that's needed is to use 'void *' in place of the undefined
'LSSharedFileListRef'.

I'll post a new patch in my next comment, and ask you (Shawn) to
review.  I suppose we should use my new patch everywhere -- on trunk
and all the branches.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

I should note that I tested my previous comment's changes in Namoroka, on OS X 10.6.1, 10.5.8 and 10.4.11, using my STR from bug 515396 comment #12.  I didn't see any problems.

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
>  @interface GrowlPreferencesController : GrowlAbstractSingletonObject {
> -       LSSharedFileListRef loginItems;
> +       void *loginItems;
>  }
If that's all your are changing, I don't need to review it and my previous review stands.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

OK, thanks.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Attachment: Patch rev1 (make upgrade to Growl 1.2 compatible with OS X 10.4)

Comment 17

[Steven Michaud [:smichaud] (Retired)]

Landed on trunk (mozilla-central):
http://hg.mozilla.org/mozilla-central/rev/012ea897d108

I'd like to let this bake for a few days before landing on any
branches.  Since this bug blocks at least one 1.9.2-branch blocker, I
should already have approval to land there.  When I land on the 1.9.2
branch I'll request approval for the other branches.

Comment 18

[Johnathan Nightingale [:johnath]]

Steven - any fallout from this bug on central? As you say, it blocks a blocker (bug 510740), so it would be nice to get it landed on 1.9.2.

Also flagging this bug blocking1.9.2+ - bugs which block blockers, block.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

> any fallout from this bug on central?

Not that I've heard.

I'll land this on the 1.9.2 branch today.  That should give us more testers, and more time before the RC.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

Landed on the 1.9.2 branch:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7bf3c6506037

Comment 21

[Reed Loden [:reed]]

This patch modified the license for Growl, but no such update was made to the license for this code under about:license. In the future, please make sure you either update about:license or file a blocker bug to have it updated.

Comment 22

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #21)

Sorry.  I didn't know about this ... but now I do.

Comment 23

[Gervase Markham [:gerv]]

Steven: don't stress about it :-) Bug 523720 has it covered.

Gerv

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Can we get this patch on 1.9.1 too? I know that is no security or stability issue but with the current behavior under 10.6 the browser window will always open in the background and our Mozmill tests fail because we need the window in the foreground.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given by bug 0 we needed this upgrade to fix bug 510740 and bug 515396. Both issues have been verified fixed on trunk and 1.9.2. That means the update of the Growl bundle was successful. Thanks Steven.