283100 - Growl global notifications in Camino for completed downloads

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, enhancement)

Description

[Brian Schack]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.2) Gecko/20040825 Camino/0.8.1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.2) Gecko/20040825 Camino/0.8.1

When a download is completed, Camino should post a Growl
notification. If you register the notifications,
then the UI for turning it on and off and customizing it is in Growl.

1) This can be done without any new UI in Camino.
2) It's not slow.

Bug #282185 applies to Firefox.

- Growl (http://growl.info/)
(http://www.drunkenblog.com/drunkenblog-archives/000341.html)
- Growl is licensed under a BSD-style license
(http://growl.info/documentation/Growl-Main-License.txt).
- Documentation (http://growl.info/documentation/)
- Implementing Growl in Carbon applications
(http://growl.info/documentation/implementing-growl.php?lang=carbon)
- Self-proclaimed "hack" that does this in Firefox
(http://www.mrchucho.net/index.php/archives/2004/12/27/firefox-and-growl/)

Thank you.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Samuel Sidler (old account; do not CC)]

While this may be useful to a few users, it doesn't seem like it would benefit
*most*. If someone wants to take it, great. Otherwise, it's targeted for FUTURE
and will get looked at later.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

*** Bug 329013 has been marked as a duplicate of this bug. ***

Comment 3

[Nick Kreeger]

(In reply to comment #0)
> User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
> rv:1.7.2) Gecko/20040825 Camino/0.8.1
> Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
> rv:1.7.2) Gecko/20040825 Camino/0.8.1
> 
> When a download is completed, Camino should post a Growl
> notification. If you register the notifications,
> then the UI for turning it on and off and customizing it is in Growl.
> 
> 1) This can be done without any new UI in Camino.
> 2) It's not slow.
> 
> Bug #282185 applies to Firefox.
> 
> - Growl (http://growl.info/)
> (http://www.drunkenblog.com/drunkenblog-archives/000341.html)
> - Growl is licensed under a BSD-style license
> (http://growl.info/documentation/Growl-Main-License.txt).
> - Documentation (http://growl.info/documentation/)
> - Implementing Growl in Carbon applications
> (http://growl.info/documentation/implementing-growl.php?lang=carbon)
> - Self-proclaimed "hack" that does this in Firefox
> (http://www.mrchucho.net/index.php/archives/2004/12/27/firefox-and-growl/)
> 
> Thank you.
> 
> Reproducible: Always
> 
> Steps to Reproduce:
> 

We would defiantly have to add a pref so that users could turn this off (if it ever gets approved to land)

Comment 4

[froodian (Ian Leue)]

> We would defiantly have to add a pref so that users could turn this off (if it
> ever gets approved to land)

Part of the whole point of Growl is that it requires zero configuration from the app's point of view (when done properly.  Please don't use Colloquy as a frame of reference).  All configuration (including turning off notifications) can be done from the Growl side - we assume that if the user has Growl installed, they want growl notifications.

I know most of you guys don't feel that way, though, so maybe a better solution would just be to ship with all notifications off and let users turn them *on* from the Growl side.

Comment 5

[Ben Willmore]

Since it doesn't look like this is going to happen soon, I made a Growl plugin for Camino, using SIMBL.  It notifies on completed downloads.  It may well be buggy.


To install:

Download and install SIMBL, see:
http://culater.net/software/SIMBL/SIMBL.php

Download GrowlCamino:
http://socrates.berkeley.edu/~benwill/GrowlCamino.tar.gz

Install GrowlCamino:
mkdir -p ~/Library/Application\ Support/SIMBL/Plugins
tar xfz GrowlCamino.tar.gz
cp GrowlCamino.bundle ~/Library/Application\ Support/SIMBL/Plugins/

Next time you run Camino, it will register with Growl.

Bug reports and suggestions to ben at opendarwin dot org.

Comment 6

[Ben Willmore]

Update:

 To install, download:

 http://socrates.berkeley.edu/~benwill/GrowlCamino-0.1.dmg

 Mount the disk image and follow the instructions.

 Next time you run Camino, it will register with Growl.

Comment 7

[Håkan Waara]

We discussed this on the Camino meeting today.

The concensus is that we could take a patch implementing a Growl download-completed notification, iff we can weak-link the framework, instead of bundling the 244K version.

AFAICS, that should technically be no problem. We weakline with the weak-framework linker flag, and then just check on runtime if the symbols are available.

Comment 8

[Chris Forsythe]

(In reply to comment #7)
> We discussed this on the Camino meeting today.
> 
> The concensus is that we could take a patch implementing a Growl
> download-completed notification, iff we can weak-link the framework, instead of
> bundling the 244K version.
> 
> AFAICS, that should technically be no problem. We weakline with the
> weak-framework linker flag, and then just check on runtime if the symbols are
> available.
> 


And how do you expect the symbols to be there if you don't include the framework?

Comment 9

[Chris Forsythe]

You guys really need to read my comments here:

https://bugzilla.mozilla.org/show_bug.cgi?id=308552#c10

Comment 10

[froodian (Ian Leue)]

ok, so we can't weak-link.  Therefore, we should use AppleScript calls instead.  a=pinkerton per status meeting since nothing gets bundled.

Comment 11

[Chris Forsythe]

(In reply to comment #10)
> ok, so we can't weak-link.  Therefore, we should use AppleScript calls instead.
>  a=pinkerton per status meeting since nothing gets bundled.
> 

So long as you guys understand that the Applescript api can change at any time and the ramifications of the changes, it should be acceptable.

Comment 12

[Simon Fraser [no longer active]]

Why doesn't growl ship a small static lib that dynamically loads the Growl.framework symbols at runtime? Then, if growl isn't installed on a user's machine, it fails gracefully.

Comment 13

[Chris Forsythe]

(In reply to comment #12)
> Why doesn't growl ship a small static lib that dynamically loads the
> Growl.framework symbols at runtime? Then, if growl isn't installed on a user's
> machine, it fails gracefully.
> 

What we do is the mac way.

Comment 14

[Stuart Morgan]

(In reply to comment #13)
> What we do is the mac way.

I'm not clear on what that means--there's more than one way to link things on the Mac, and weak linking is supported by the linker that's part of Mac OS X (and if there is only One True Way, I personally like to think it's not a solution that results in n copies of the same code all resident in non-shared memory at the same time).

Leaving that aside though: I've read your comments in bug 308552, and I want to make sure I'm understanding the situation with AppleScript correctly.  As I read it, all of the following is true:

- If Camino uses AppleScript, the Growl support could completely break as a result of a user upgrading Growl.  (I'm not clear on whether that just means explicitly downloading a new version, or if downloading any app that bundles a newer Growl would do it.)

- If the above happens, it could in addition result in Camino crashing a core Growl service.  (What are the implications for other apps that are currently running and try to use Growl if that happens?)

- Because the AppleScript API is subject to change without notice, it is possible to end up in a situation where Camino upgrades would have to be lockstepped with Growl upgrades on all user's machines to prevent breakage, because having a new Camino and an old Growl would be as bad as an old Camino and a new Growl. (This would have the effect of not only being a big pain for users, but would mean Camino would be essentially forced to release a new stable release any time Growl changed the AppleScript API.)

Please let me know if that's not accurate; if it is it seems like AppleScript is completely unworkable as a solution for Camino.

Comment 15

[Chris Forsythe]

(In reply to comment #14)
> (In reply to comment #13)
> > What we do is the mac way.
> 
> I'm not clear on what that means--there's more than one way to link things on
> the Mac, and weak linking is supported by the linker that's part of Mac OS X
> (and if there is only One True Way, I personally like to think it's not a
> solution that results in n copies of the same code all resident in non-shared
> memory at the same time).
> 
> Leaving that aside though: I've read your comments in bug 308552, and I want to
> make sure I'm understanding the situation with AppleScript correctly.  As I
> read it, all of the following is true:
> 
> - If Camino uses AppleScript, the Growl support could completely break as a
> result of a user upgrading Growl.  (I'm not clear on whether that just means
> explicitly downloading a new version, or if downloading any app that bundles a
> newer Growl would do it.)
> 
> - If the above happens, it could in addition result in Camino crashing a core
> Growl service.  (What are the implications for other apps that are currently
> running and try to use Growl if that happens?)
> 
> - Because the AppleScript API is subject to change without notice, it is
> possible to end up in a situation where Camino upgrades would have to be
> lockstepped with Growl upgrades on all user's machines to prevent breakage,
> because having a new Camino and an old Growl would be as bad as an old Camino
> and a new Growl. (This would have the effect of not only being a big pain for
> users, but would mean Camino would be essentially forced to release a new
> stable release any time Growl changed the AppleScript API.)
> 
> Please let me know if that's not accurate; if it is it seems like AppleScript
> is completely unworkable as a solution for Camino.
> 

Comment 16

[Chris Forsythe]

Ok, let's try this again:

(In reply to comment #14)
> (In reply to comment #13)
> > What we do is the mac way.
> 
> I'm not clear on what that means--there's more than one way to link things on
> the Mac, and weak linking is supported by the linker that's part of Mac OS X
> (and if there is only One True Way, I personally like to think it's not a
> solution that results in n copies of the same code all resident in non-shared
> memory at the same time).

A framework gives you separation of code much in the same way that a shared library or other things would give you it. Frameworks are generally the "one true way" to do this within Cocoa.

Reference: http://developer.apple.com/documentation/MacOSX/Conceptual/BPFrameworks/index.html

"A framework is a hierarchical directory that encapsulates shared resources, such as a dynamic shared library, nib files, image files, localized strings, header files, and reference documentation in a single package"

Weak-linking is an option we tried with some successes and a lot of failures. It's not something that I'd like to revisit.

This is why the framework ships within the app bundle of every app using it and not installed with Growl. One cannot simply assume the framework is on the system, but a lot of people had problems with weak-linking. Shipping the framework in the app bundle solves this, and generates a new problem of the framework being on the system multiple times.

It's a decision really, and we preferred the choice of eliminating a crasher, retaining separation of code that a framework provides, and general ease of use for the developer implementing Growl rather than choosing to save negligible amounts of disk space in order to uphold an ideal that a shared library/framework should be used system wide.

If you would like to discuss this point further, I am on #growl on freenode most days. We will not be changing how the framework is shipped in apps though, and I believe this is probably counter productive for this ticket/bug (or whatever you guys call these things).

> Leaving that aside though: I've read your comments in bug 308552, and I want to
> make sure I'm understanding the situation with AppleScript correctly.  As I
> read it, all of the following is true:
> 
> - If Camino uses AppleScript, the Growl support could completely break as a
> result of a user upgrading Growl.  (I'm not clear on whether that just means
> explicitly downloading a new version, or if downloading any app that bundles a
> newer Growl would do it.)

Correct, in a way. I'll give you a specific example though.

The applescript api in .5 was designed so that it allowed notifications without registration. Basically, registration is what generates an appticket in the Growl Apps tab. No registration = no preferences for that applescript.

There was an application which had decided not to use the shared framework at the time (remember, Growl .5 was a pkg installer and did install the framework into /Library/Frameworks). When we shipped Growl .6, it completely stopped his application from being able to talk to Growl.

Since the Applescript api is meant just for scripters, our policy has always been that applications should use the framework, but that they could use the applescript api if they really wanted to. 

> 
> - If the above happens, it could in addition result in Camino crashing a core
> Growl service.  (What are the implications for other apps that are currently
> running and try to use Growl if that happens?)
> 

Honestly not sure in what it would result in. I don't see us changing it much if at all, but the point still remains that the applescript api is meant for scripters and not application devs.

> - Because the AppleScript API is subject to change without notice, it is
> possible to end up in a situation where Camino upgrades would have to be
> lockstepped with Growl upgrades on all user's machines to prevent breakage,
> because having a new Camino and an old Growl would be as bad as an old Camino
> and a new Growl. (This would have the effect of not only being a big pain for
> users, but would mean Camino would be essentially forced to release a new
> stable release any time Growl changed the AppleScript API.)
> 

If it broke Growl integration or caused crashing of Camino or GrowlHelperApp, then yes. If it caused crashing in GrowlHelperApp especially because I'd probably become rather annoying ;)

> Please let me know if that's not accurate; if it is it seems like AppleScript
> is completely unworkable as a solution for Camino.
> 

I hope I've explained it adequately. Please feel free to contact me via my email address (in the cc list) or via irc if you need further explanation. If irc it could be logged and posted here.

I've also branched from our .7.4 tag in the hopes to create the smallest framework possible for you guys. I don't think it's going to shrink down much, but eh worth a shot right? :)

Comment 17

[Chris Forsythe]

FYI, from the work done tonight, the framework looks to have gotten smaller.

With headers: 184k
Without headers: 136k

These numbers should fluctuate, but so far so good with this branch.

If any camino devs are interested in working on this branch to get the framework down to what you folks need, let me know.

Comment 18

[Mike Pinkerton (not reading bugmail)]

i finally nailed gerv down. He's fine with us checking in the binary as long as we also check in accompanying source. He seemed fine with the existing BSD-esque license.

So we can either check in the src and build the binaries ourselves, or just check in the source and binaries together. i don't have a preference.

Comment 19

[Evan Schoenberg]

(In reply to comment #12)
> Why doesn't growl ship a small static lib that dynamically loads the
> Growl.framework symbols at runtime? Then, if growl isn't installed on a user's
> machine, it fails gracefully.
> 

I think this point bears clarification even though the root of the problem ("Can Camino include the Growl framework?") has been answered.

Growl being installed does not put Growl.framework anywhere on the system; this framework is, instead, shipped only with applications. That's why it couldn't be dynamically loaded -- it doesn't exist except within apps.  If it *were* installed, applications would weak link against it and that'd be that, but as Chris mentioned there were a number of problems with that approach; such problems have led it to be a very rare occurrence for OS X developers to install custom frameworks systemwide for use by other applications.

Comment 20

[Ben Willmore]

Attachment: Patch providing Growl notifications for downloads

Comment 21

[Ben Willmore]

The attached patch provides basic Growl support for downloads.  The Growl framework is required as well, and could be incorporated in a number of ways, as discussed by those who understand the constraints better than me.

Note that, if Growl is not installed, this patch does nothing and is invisible to the user.

Comment 22

[Stuart Morgan]

Comment on attachment 244060 [details] [diff] [review]
Patch providing Growl notifications for downloads

>+    mGrowlController = [[GrowlController alloc] init];


This should be released in the dealloc

>+- (void) growlNotificationWasClicked:(id)clickContext;

No space between the type and the method name, so:
- (void)growl...
(the current source is inconsistent about that, but we are slowly cleaning it up)

Also, any methods which aren't intended to be called from outside the class, which in this case is all of them, should be declared in a Private category at the top of the implementation file rather than in the header

>Index: src/browser/GrowlController.mm

Is this file actually bringing in any cpp code that requires this to be a .mm file?

>+static NSString* const kShortDownloadCompleteGrowlName  = @"Short Download Complete";
>+static NSString* const kDownloadCompleteGrowlName  = @"Download Complete";

Why the arbitrary distinction of short and not short?

> +  NSArray *allNotifications = [[NSArray alloc] initWithObjects:


We tend to prefer the use of autoreleased objects in non-performance-critical code, so use arrayWithObjects: here.

>+  NSDictionary* notificationDictionary = [NSDictionary dictionaryWithObjectsAndKeys:
>+			allNotifications, GROWL_NOTIFICATIONS_DEFAULT,
>+			allNotifications, GROWL_NOTIFICATIONS_ALL,
>+            nil];

There are tabs here, and several other places in this patch.  There is also trailing whitespace on a bunch of lines that needs to be removed.

>+- (void)notify:(NSNotification*)notification

This could use a more descriptive name; perhaps something along the lines of growlForNotification: ?

>+  NSString* description = nil;
>+  NSDictionary* context = [NSDictionary dictionaryWithObjectsAndKeys:notificationName, @"name", objectAsNumber, @"object", userInfo, @"userInfo", nil];

In general, declare variables as they are needed.

>+  if ([notificationName isEqual:kDownloadStartedNotificationName])
>+    [[NSNotificationCenter defaultCenter] postNotificationName:kDownloadStartedGrowlClickedNotificationName
>+                    object:object userInfo:[clickContext objectForKey:@"userInfo"]];
>+  else if ([notificationName isEqual:kDownloadFailedNotificationName])
>+    [[NSNotificationCenter defaultCenter] postNotificationName:kDownloadFailedGrowlClickedNotificationName
>+                    object:object userInfo:[clickContext objectForKey:@"userInfo"]];
>+  else if ([notificationName isEqual:kShortDownloadCompleteNotificationName] || [notificationName isEqual:kDownloadCompleteNotificationName])
>+    [[NSNotificationCenter defaultCenter] postNotificationName:kDownloadCompleteGrowlClickedNotificationName
>+                    object:object userInfo:[clickContext objectForKey:@"userInfo"]];

The download controller shouldn't have to know about the Growl notification system; that's the wrong way for the dependency to go. You already have the objects, so it seems like the better way to go is to make calls directly to them.

>+// select a single download, regardless of modifiers etc
>+-(void)singleDLInstanceSelected:(NSNotification*)notification
>+{
>+  ...

This seems convoluted and backwards, but looking into the download view stuff it appears that it all works this way.  For the second round kreeger should probably take a look too, since he's the one familiar with this code.  From what I see, though, it seems like you might be able to have your new select: method just set |lastModifier| to nothing and use DLInstanceSelected:, rather than create a new method.

>+  // if we are going to terminate, cancel all active downloads
>+  if (shouldTerminate) {
>+    NSEnumerator* dlEnumerator = [mProgressViewControllers objectEnumerator];
>+    ProgressViewController* theDL = nil;
>+    while ((theDL=[dlEnumerator nextObject])) {
>+      if ([theDL isActive])
>+        [theDL cancel:self];
>+	}
>   }

applicationWillTerminate is the right place to do this sort of thing. Why is it needed at all though? And will it interfere with restarting the downloads later?

>+-(void)select:(id)sender
>+{
>+  [self setSelected:YES];
>+  [[NSNotificationCenter defaultCenter] postNotificationName:kSingleDownloadInstanceSelectedNotificationName object:self userInfo:nil];
>+}

It seems ugly to do this here when the rest of these kind of notifications are posted in the view, but again the whole view system the downloads use seems very strange, so I'll defer to kreeger here.


Once the patch is finished, pink or mento is going to need to decide where and how the framework will fit into the project and build system.

Comment 23

[Chris Lawson (gone)]

Ben, have you had a chance to look at this at all vis a vis Stuart's comments on your patch?

Comment 24

[Markus S.]

In case you didn't notice, Growl is now enabled in Firefox trunk builds: Bug 362685

Comment 25

[Stuart Morgan]

That's not really relevant here; we don't share download UI implementation with Firefox, and this patch is still unfinished.

Comment 26

[Ben Willmore]

I no longer have time to work on this, sorry.

Comment 27

[Ilya Sherman]

Attachment: update of Ben's patch, w/ lots of questions

Not sure if this is appropriate, but I have left in a bunch of // TODOs that are mostly design or style questions (but a few actual code issues).  Feel free to ignore any that aren't obvious --- I'll look into them later.

Comment 28

[Stuart Morgan]

Comment on attachment 348314 [details] [diff] [review]
update of Ben's patch, w/ lots of questions

Very rough pass, mostly just answering TODO questions:

>+// TODO: these strings and the below should be marked const, but this gives warnings...

What warnings?

>+// TODO: Short download is here so that users can disable notifications for
>+//       "short" downloads from within Growl.  Not clear if this is worth keeping, but
>+//       leaving for now

I don't know anything about Growl prefs; is a "short" download some concept intrinsic to Growl?

>+extern NSString* kShortDownloadCompleteNotificationName;
>+extern NSString* kDownloadCompleteNotificationName;

If this concept only matters to Growl, then make this code figure it out based on elapsed time since the start notification for that download. That arbitrary concept should not be pushed into the actual download logic.

>+// TODO: any reason to keep these?

Nope. Growl logic shouldn't be sprinkled through other parts the code base.

>+// TODO the commented-out method signatures are actually inherited --- should they
>+//      be redeclared?

Nope.

>+    // TODO: Should the localized strings have comments?

New Camino code should use |nil| for the second argument.

>+    // TODO: this is no longer necessary, right?
>+    //[allNotifications release];

Right, this line would cause crashes now.

>+    // TODO why does clickContext have to be plist-able?

I'm not really the best person to answer this question, but presumably GrowlApplicationBride is going to push your context back and forth over DO or the equivalent, and needs serializable values.

>+    NSNumber* objectAsNumber = [NSNumber numberWithInt:(long) [notification object]];

Why cast it to a long, but then use numberWithInt?

More importantly though, this code assumes that you can store off a pointer to an object that you don't own and then use it again later. That's incredibly dangerous--for example, you'd probably find that with the 'remove downloads on completion' pref set, clicking on a Growl notification would crash.

You need to do something more along the lines of storing a pointer and then asking the main download controller to select the view for that pointer if it still exists. (Alternately you could in theory retain the download, but we unfortunately have some very fragile code in the download manager that would make changing the lifetimes inadvisable.)

>+    // TODO what should the name & title be if the notificationName is something
>+    // other than one of the above cases?

How would that happen?

>+    [GrowlApplicationBridge notifyWithTitle:title description:description
>+                            notificationName:name iconData:nil priority:0
>+                            isSticky:0 clickContext:context];

One argument per line; this style is hard to read.

>+    // TODO: is this good style for getting the controller?

See above.

>+        // TODO: How do I deselect other downloads?  Not really sure how the controller system works...

You need to be talking to the overall download UI controller, not the view controller for the individual download.

>+    // TODO: style question: where does the line-break go for the ||?

We tend to prefer leaving the operator as the end of the line.

>+        // TODO am I the "sender"?

Or |nil|; we don't have much code that cares about senders, and aren't consistent when it doesn't matter.

>+const NSString* kDownloadStartedNotificationName = [...]

NSString* const. The data is already const since it's not an NSMutableString.

>+	GrowlController*        mGrowlController;

Indent with spaces, not tabs.

Comment 29

[Ilya Sherman]

Attachment: Updated patch, with far fewer TODOs

Comment 30

[Ilya Sherman]

(In reply to comment #28)
> (From update of attachment 348314 [details] [diff] [review]JG [details]JH)
> Very rough pass, mostly just answering TODO questions:
> 
> >+// TODO: these strings and the below should be marked const, but this gives warnings...
> 
> What warnings?

I think I was doing it the wrong way 'round, as const NSString* rather than NSString* const

> 
> >+// TODO: Short download is here so that users can disable notifications for
> >+//       "short" downloads from within Growl.  Not clear if this is worth keeping, but
> >+//       leaving for now
> 
> I don't know anything about Growl prefs; is a "short" download some concept
> intrinsic to Growl?

Users can customize preferences (like whether to show a notification) for each type of notification registered with Growl from within the Growl system preference pane.  So, the intended use case is that somebody might not want to see a "Download Completed" notification for a download that takes < 15 seconds (number is arbitrary; not sure if it's a good number), but want to see such a notification for longer downloads.  I'm not sure if this is a realistic use-case though...

Comment 31

[Ilya Sherman]

Attachment: Quick update, remove |self| as observer on |dealloc|

Comment 32

[Ilya Sherman]

Quick update responding to concerns about the framework (per IRC):

Q: Are there any l10n issues with the framework?
A: Nope.

Q: What happens if Growl is not installed?  Does it annoy the user?
A: If Growl is not installed, nothing really happens.  The user is left in blissful peace :)

Q: How big is the framework?
A: 232k with headers, 140k without.

Does it sound fine to bundle the framework?  Are there any concerns that I missed?

Comment 33

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Ilya's patch itself adds some strings that must be localized, so it must be finished before the l10n freeze if it will make 2.0.

Comment 34

[Stuart Morgan]

Comment on attachment 349546 [details] [diff] [review]
Quick update, remove |self| as observer on |dealloc|

>+// TODO what's the syntax to mark these const?  I get linker errors if I try |NSString* const|

NSString* const is the correct syntax. Without knowing what the linker error is, I can't really help.

>+  // TODO is it better style to define constants for literal strings?

Yes.

>+// Names of NSNotifications we will register for
>+extern NSString* const kDownloadStartedNotificationName;
>+extern NSString* const kDownloadFailedNotificationName;
>+extern NSString* const kDownloadCompleteNotificationName;

These belong in the header of the class that defines them.

>+  [[NSNotificationCenter defaultCenter] removeObserver:self];
>+	[super dealloc];

No tabs.

>+  // TODO else log an exception?

I still don't understand how you believe this case is possible. Who do you think is going to call this method other than the three callbacks you set up for specific notifications?

>+      [progressWindowController updateSelectionOfDownload:downloadInstance

You still haven't addressed the safety issue; passing a potentially dead object as an argument is not okay unless it's to a method that explicitly expects that. The *only* thing you can do with that pointer at this stage is do pointer-only comparisons to find out if it's still a live object.

>+  // TODO else log unknown notification?

Why? And again, how would that happen?

Comment 35

[Stuart Morgan]

I'm fine with doing a strings-only patch right now, but this bug as a whole isn't going to block b1.

Comment 36

[Ilya Sherman]

Stuart, thanks for the comments :)

(In reply to comment #34)
> (From update of attachment 349546 [details] [diff] [review])
> >+// TODO what's the syntax to mark these const?  I get linker errors if I try |NSString* const|
> 
> NSString* const is the correct syntax. Without knowing what the linker error
> is, I can't really help.
> 
> >+// Names of NSNotifications we will register for
> >+extern NSString* const kDownloadStartedNotificationName;
> >+extern NSString* const kDownloadFailedNotificationName;
> >+extern NSString* const kDownloadCompleteNotificationName;
> 

I was getting a linker error that these symbols weren't found when they were both extern and marked as const in ProgressViewController.mm.  Moving them to the header resolves the linker error though.

> >+  // TODO else log an exception?
> 
> I still don't understand how you believe this case is possible. Who do you
> think is going to call this method other than the three callbacks you set up
> for specific notifications?

Well, it's logging an exception because it's not expected.  It's maybe a useful way to future-proof the code, so that when the Growl code is being updated and new notifications are added, it's immediately clear if you forget to update part of the code.  Is it better to just give the method a very clear comment that notes what notifications it should expect?

> >+      [progressWindowController updateSelectionOfDownload:downloadInstance
> 
> You still haven't addressed the safety issue; passing a potentially dead object
> as an argument is not okay unless it's to a method that explicitly expects
> that. The *only* thing you can do with that pointer at this stage is do
> pointer-only comparisons to find out if it's still a live object.

I marked this bug as depending on bug 465242 for precisely this reason; guess I should probably have also left a comment in this bug.  Part of that patch updates [progressWindowController updateSelectionOfDownload] to check for dead downloads.

Comment 37

[Ilya Sherman]

Attachment: Patch v2.5, addressing (most of) Stuart's comments

Questions remaining on my end:

1) Do we want to maintain the distinction between "downloads" and "short downloads" so that users can configure different behavior for each?

2) Should we log exceptions for unknown (unexpected) notifications, or just silently ignore them?  With the current code, there *shouldn't* be a way to hit these exceptions; the exceptions would be purely for the sake of protecting against bit rot a bit.

3) Is the code I have for opening a file given its filename (at the very end of GrowlController.mm) fine, or is there a better way to write it?  I've seen similar code look different in other parts of the Camino source; but since I'm pretty new to Cocoa, I'm not sure if the differences are purely for the sake of finer-grained behavior, or because there's something non-optimal about this way of opening a file.

Comment 38

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #35)
> I'm fine with doing a strings-only patch right now, but this bug as a whole
> isn't going to block b1.

Moving this to the b2 flag; it'll either need be done before b2 or have to wait for Camino Next.

Comment 39

[Stuart Morgan]

(In reply to comment #36)
> I marked this bug as depending on bug 465242 for precisely this reason; guess I
> should probably have also left a comment in this bug.  Part of that patch
> updates [progressWindowController updateSelectionOfDownload] to check for dead
> downloads.

I really don't like the idea of some random method in the controller having to expect to be dealing with a bad object. It's updateSelectionOfDownload:, not updateSelectionOfDownloadThatWasPointedToByPontentiallyDeadPointer:. There needs to be a special-purpose method that does nothing but give back either a (valid) download object or nil. Although at that point, it would be better to use some identifier rather than overloading the pointer for that purpose. You could have each download synthesize an ID, but since performance isn't an issue I'd say just store the path and make a method that does path-based lookup.

(In reply to comment #37)
> 1) Do we want to maintain the distinction between "downloads" and "short
> downloads" so that users can configure different behavior for each?

Since none of us use Growl, I don't think we can give useful opinions here. Pick whatever makes sense to you, and go with it.

> 2) Should we log exceptions for unknown (unexpected) notifications, or just
> silently ignore them?

You should ignore them. I find it very unlikely that someone adding new code in this class would go to the trouble of creating a new notification and registering as a listener for it, then forget to write code that actually handled the notification--and then never test that it worked correctly.

> 3) Is the code I have for opening a file given its filename (at the very end of
> GrowlController.mm) fine, or is there a better way to write it?

That's fine, but you should check that it exists first. Calling open on the download would take care of that for you, so that may be easier--although if someone has their download manager set to remove downloads when done, doing it the way you have it now would make clicking the notification do what people would presumably want.

Comment 40

[Stuart Morgan]

(In reply to comment #39)
> I really don't like the idea of some random method in the controller having to
> expect to be dealing with a bad object. It's updateSelectionOfDownload:, not
> updateSelectionOfDownloadThatWasPointedToByPontentiallyDeadPointer:.

And I do realize I suggested pretty much exactly what I'm objecting to now; I'm
not sure what I was thinking at the time. Sorry about that :(

Comment 41

[Christopher Henderson]

As a frequent user of Growl, I know I'd appreciate the distinction between short and long downloads. Without the ability to turn off notifications for short downloads, I probably wouldn't use this feature at all.

Comment 42

[Ilya Sherman]

Attachment: Patch v2.6

The patch includes the code for bug 465242 as well, because I couldn't figure out an easy way to separate out the differences for this patch; but the only relevant change in |ProgressDlgController| is the new method |downloadWithIdentifier|.  I'll clean up this patch once the other one lands (unless there's an easy way to split out this change, in which case I'll do it earlier).

This patch adds unique identifiers to |ProgressViewController|s.  I opted for new identifiers rather than just the filepath because the filepath is not guaranteed to be unique across downloads (e.g. if the user downloads two files named 01.pdf, and deletes the old one while the new one is downloading).  Two things I'm not sure about though:

1) Will these identifiers be saved to disk when Camino quits?  If so, is there a way to mark them not to be, and then to generate new ones upon reloading the document?  It seems much easier to ensure the identifiers are really unique that way, and the expected use case for the identifiers doesn't need them to be consistent across runs of Camino.

2) Is there a need to worry about thread-safety with the shared identifier pool -- i.e., can multiple threads create |ProgressViewController|s?  If so, what's the best way to make the relevant code ([ProgressViewController getNextIdentifier]) thread-safe?  I found [http://developer.apple.com/documentation/Cocoa/Conceptual/Multithreading/ThreadSafety/chapter_5_section_6.html], but I'm not sure if that's the best solution -- should I just make the identifier be of type |int32_t|?

Comment 43

[Stuart Morgan]

Comment on attachment 351829 [details] [diff] [review]
Patch v2.6

(In reply to comment #42)
> 1) Will these identifiers be saved to disk when Camino quits?

Only if you add them to downloadInfoDictionary.

> can multiple threads create |ProgressViewController|s?

I'm not sure offhand. If you want to make it threadsafe, just wrap it in a @synchronized(self) block; it's not performance-critical code.


Massive whitespace cleanup needs happen in totally separate patches; it messes with cvs blame, and it makes reviewing a pain. Please don't include all the whitespace-only changes in the next version.

Comment 44

[Ilya Sherman]

Attachment: Patch v2.7, minus purely whitespace changes

Comment 45

[Ilya Sherman]

Attachment: Patch v2.7, sans bug 465242 code

Comment 46

[Stuart Morgan]

Comment on attachment 352145 [details] [diff] [review]
Patch v2.7, sans bug 465242 code

>+++ src/download/ProgressViewController.h	9 Dec 2008 19:06:38 [...]
>+// Names of notifications we will post on download-related events
>+NSString* const kDownloadStartedNotificationName = @"DownloadStartedNotificationName";
>+NSString* const kDownloadFailedNotificationName = @"DownloadFailedNotificationName";
>+NSString* const kDownloadCompleteNotificationName = @"DownloadCompleteNotificationName";
>+
>+// Names of keys for objects passed in these notifications
>+NSString* const kDownloadNotificationFilenameKey = @"DownloadNotificationFilenameKey";
>+NSString* const kDownloadNotificationTimeElapsedKey = @"DownloadNotificationTimeElapsedKey";

String values should not be in headers. The header should declare these as extern, with no definition, and they should be defined in the implementation file.

>++(unsigned)getNextIdentifier;

(unsigned int)

>+    toReturn = identifier;
>+    identifier++;

You may as well make this one line, since you are post-incrementing anyway.

>+  while ((curController = [downloadsEnum nextObject]))
>+  {

{ should be on the same line as the |while|.

>+    if ([curController uniqueIdentifier] == identifier) return curController;

... but this should be two lines.

>+- (id)init
>+{
>+  if ((self = [super init])) {
>+    [GrowlApplicationBridge setGrowlDelegate:self];
>[...]
>+- (void)dealloc
>+{
>+  [[NSNotificationCenter defaultCenter] removeObserver:self];
>+  [super dealloc];
>+}

Doesn't there need to be a [GrowlApplicationBridge setGrowlDelegate:nil] or similar in the dealloc?

>+  NSNumber* downloadIdentifier = [NSNumber numberWithUnsignedInt: [download uniqueIdentifier]];

No space after :

>+  if ([notificationName isEqual:kDownloadStartedNotificationName] ||
>+      [notificationName isEqual:kDownloadFailedNotificationName]) {

{ on a new line here, since it's a multi-line conditional.


With those changes, r=me for the code. I don't use Growl though, so I can't test; tagging hendy for a behavioral review since he does.

Comment 47

[Ilya Sherman]

Attachment: Patch v2.8, addressing Stuart's comments

Thanks for the many rounds of review, Stuart :)

Comment 48

[Ilya Sherman]

Attachment: v2.8 refreshed

Had accidentally left in a bit of code from working on another patch...

Comment 49

[Christopher Henderson]

You might want to look at that patch again:

patch: **** malformed patch at line 153: @@ -281,16 +292,27 @@

I can excise the superfluous blocks of code manually on my end, though.

Comment 50

[Ilya Sherman]

Attachment: v2.8 properly refreshed

This time properly generated, not just quickly hand-edited.

Comment 51

[Ilya Sherman]

Attachment: Growl framework

The process for making this patch build is a bit annoying:

1) Add the Growl framework to the project (I added it to Frameworks ~> Linked Frameworks)
2) Add GrowlController.h/mm to the project (I added it to Classes ~> UI Components)

3) Add the Growl framework to Camino App ~> Copy Frameworks
4) Add the Growl framework to Camino App ~> Link Binary with Libraries
5) Add GrowlController.mm to Camino App ~> Compile Sources

Maybe not all of this is needed, but I'm pretty sure that at least this works.

Comment 52

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #51)
> Created an attachment (id=356963) [details]
> Growl framework

We need to handle this just like SharedMenus and Sparkle, with a source directory, README detailing the source revision, and so forth, and then make the project changes based on building that.

Comment 53

[Christopher Henderson]

Instead of the notification click opening the downloaded file, do we want to have it reveal the file in the Finder? This would be safer and more convenient, IMO, and more in line with what other apps (eg: Transmission) do. Often I will want to move the file before opening it, or open it in a non-default app.

Comment 54

[Ilya Sherman]

Attachment: v2.9, reveal file instead of opening

Good idea, hendy :)

Comment 55

[Christopher Henderson]

Comment on attachment 357525 [details] [diff] [review]
v2.9, reveal file instead of opening

Works well, and has plenty of scope to customize notification behaviour.

Comment 56

[Ilya Sherman]

Attachment: v2.91, comments updated

Comment 57

[Ilya Sherman]

Attachment: v2.92, minor fix-up

Per http://growl.info/documentation/developer/implementing-growl.php?lang=cocoa, updates |dealloc| to have [GrowlApplicationBridge setGrowlDelegate:@""]; instead of [GrowlApplicationBridge setGrowlDelegate:nil];

Comment 58

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 357529 [details] [diff] [review]
v2.92, minor fix-up

>Index: src/browser/GrowlController.h
>===================================================================
>RCS file: src/browser/GrowlController.h
>diff -N src/browser/GrowlController.h
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ src/browser/GrowlController.h	12 Jan 2009 09:23:40 -0000

>Index: src/browser/GrowlController.mm
>===================================================================
>RCS file: src/browser/GrowlController.mm
>diff -N src/browser/GrowlController.mm
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ src/browser/GrowlController.mm	12 Jan 2009 09:23:40 -0000

Why do these files live in src/browser?  Since they have absolutely nothing to do with the browser window, that doesn't seem like the right place for them; I'd have expected either src/download or src/application.

Comment 59

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Adds Growl to the project

Here's the project+Makefile parts of the patch.

In conjunction with Ilya's patch as currently available (v2.92) and the Growl source archive (http://www.growl.info/files/source/Growl-1.1.4-src.tar.bz2) expanded into mozilla/camino/growl, this should produce a working Camino-with-Growl.  The Universal framework clocks in at 233 KB after being lipoed together and (I assume) teased; as shipped by the Growl SDK, it's 248 KB.

I've built both my debug and static Universal trees, and the build appears to succeed and the app launches, but I'd appreciate some verification from a) people who have Growl installed, to ensure the Growl integration works and b) someone on 10.4 with Xcode 2.5 installed (Growl claims to require Xcode 2.5 to build, so while we've probably still worked with Xcode 2.4.x for the last year or so, we should now have a hard requirement of Xcode 2.5 or newer).

Stuart, you've done the "add frameworks" dance before, so if you could sanity-check the actual project changes, too, I'd appreciate that.

Comment 60

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #57)
> Created an attachment (id=357529) [details]
> v2.92, minor fix-up

Er, one more question for Ilya here (since when the bug reloaded, I saw the "l10n" tag); didn't you tell us at one point we'd be picking up some Localizable.strings additions to support this patch?

Comment 61

[Ilya Sherman]

(In reply to comment #58)
> Why do these files live in src/browser?  Since they have absolutely nothing to
> do with the browser window, that doesn't seem like the right place for them;
> I'd have expected either src/download or src/application.

I just left them where they were in the original patch, but src/application makes more sense to me, since we might expand Growl to include more than just download notifications.

(In reply to comment #59)
> Created an attachment (id=357618) [details]
> Adds Growl to the project
> 
> Here's the project+Makefile parts of the patch.

Thanks for putting this together :)

Just to check: did you look into using the Growl source that Firefox is using?  I think it's http://mxr.mozilla.org/mozilla/source/toolkit/components/alerts/src/mac/growl/


(In reply to comment #60)
> (In reply to comment #57)
> > Created an attachment (id=357529) [details] [details]
> > v2.92, minor fix-up
> 
> Er, one more question for Ilya here (since when the bug reloaded, I saw the
> "l10n" tag); didn't you tell us at one point we'd be picking up some
> Localizable.strings additions to support this patch?

Yep.  GrowlController.mm lines 46-49, 101-104, 126, 129, 134, 135, 138.  Actually, would it be better to change lines 46-49 to make global |NSLocalizedString|s instead of global |NSString|s?

Comment 62

[Mike Pinkerton (not reading bugmail)]

+      NSString* name = mDownloadFailed? kDownloadFailedNotificationName : kDownloadCompleteNotificationName;

space before |?|

sr=pink

Comment 63

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 357529 [details] [diff] [review]
v2.92, minor fix-up

sr=pink

Comment 64

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 357618 [details] [diff] [review]
Adds Growl to the project

As Ilya discovered, this is missing the Makefile.in hunk (and as I discovered, has some "Xcode-trying-to-be-smarter-than-you" cruft, too).

I'll attach a new patch later that takes care of both and also includes the subset of the Growl tree that we need to build just the framework.

Comment 65

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Adds Growl to the project, adds Growl framework sources to cvs

Per discussions today, we'll just check in the Growl source files needed to build the framework; that's a small enough set to include in a patch, so this patch includes them (in addition to a README and the Makefile/project/xcconfig changes).

This patch also assumes that GrowlController.{h|mm} live in src/application, where Ilya will put them in the next version of his patch.

As before, I've verified build+launch on a Growl-less 10.5, but it'd be good if Growl users and 10.4 users could verify that my project integration works OK.

Comment 66

[Ilya Sherman]

Attachment: v2.93, localized

Comment 67

[Stuart Morgan]

Comment on attachment 358124 [details] [diff] [review]
Adds Growl to the project, adds Growl framework sources to cvs

r=me

Comment 68

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk, and cb-xserve01 and cb-miniosx01 have gone green.

Thanks, Ben, for the start of this patch, and Ilya for bringing it to completion!

Comment 69

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Early indications show boxset has a significant (~10%) Ts regression from this checkin; I'll let the tree run without further checkins overnight to see what happens to the numbers.

Comment 70

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

We earlier filed bug 475571 on comment 69.

Switching to lazy loading to fix issues identified in bug 510740 has brought us a ~13% Ts win on cb-minibinus01-perf, about 200ms, which probably wipes out the regression (though it's hard to say exactly with different boxen, even if their build configs are otherwise the same).