316232 - Add ability to pass RSS/Atom feeds detected on a page to a helper app

Status: VERIFIED FIXED

(Camino Graveyard :: General, defect)

Description

[Nick Kreeger]

We should have the ability to pass off feeds that get detected in a page to a helper app, rather than just a bug for full blown mangament of RSS/ATOM feeds

Comment 1

[Nick Kreeger]

Attachment: Status bar icon idea 1

This is a screen shot with the rss icon being placed next to the pop-up icon.

Comment 2

[Nick Kreeger]

Attachment: Screen shot where the rss icon is placed in the URL bar

This is a screen shot of the Feed logo placed in the URL bar

Comment 3

[Nick Kreeger]

Attachment: Screen shot where feed icon and security bolt are displayed at the same time

This is a screen shot where both the feed icon and the security bolt are displayed in a site where there is a feed and a ssl connection

Comment 4

[Nick Kreeger]

Attachment: Update when a feed icon is clicked on

This is an update to my work, i don't pull out the title of the feed yet, so it just displays the title of the feed

Comment 5

[Jasper]

Attachment: How I'd say we should do it

Since I'd say the majority of our users have learned to read from the left top to the right bottom, using the left bottom of the UI to show status icons isn't the most optimal thing to do as it's not the place people will expect these things.

So I would sugggest to move the popup and feed detection icons to the right side of the statusbar. I would also like to suggest we find a way in the future to dynamically place the icons depending on what other object are shown.

Another benefit of placing the icon on the right is that our status and link text will no longer be indetend on the left.

Comment 6

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

I agree with Jasper, although I was thinking that the icon should be to the right of the progress bar so it doesn't look like it's stranded in the middle of the status bar when no progress bar is showing. (Dynamic positioning would probably solve bug 166248, too, but we really probably want to make the Feed and Popup Blocking icons fixed targets, Fitt's Law and all that....)

I'm also concerned with overloading the location bar, particularly for those with smaller (1024x768 and 800x600 iBooks) screens and people running the default toolbar layout; it'll look more crowded than in Nick's screenshot, I'm afraid.

Comment 7

[Simon Fraser [no longer active]]

I really don't think that bottom-right is more visible than bottom left. Yes, people scan top-left to bottom-right, but that means that bottom-right is the *last* place they look (and it's also the part of the window with the least real-estate value, so may often get obscured by other windows).

There's a strong precedent for using the bottom-left for "status" icons (e.g. Netscape 4.x and on).

Comment 8

[Jasper]

Either way the status bar or the url field I think they both suck. I still believe in just doing a toolbar item. That's what the freaking things is for.

Comment 9

[Mike Pinkerton (not reading bugmail)]

so here's my buck-fitty on placement....

I think that bottom left is the *last* place one looks. As you scan across the window, your eyes move diagonally and naturally rest at the bottom right. The user isn't used to looking at bottom left because generally no content is there, so when there is some, it's overlooked entirely. It takes extra, conscious, effort to look at content on the bottom left. Also, they're more likely to see things on the right because the scrollbar is there. 

And i'm not certain if NS4 is necessarily a good precedent ;)

Comment 10

[Simon Fraser [no longer active]]

I'm more likely to see things in the bottom left, because a) the status text is there, and b) when I'm reading a web page the text is most often left-aligned.

However, I don't know that the placement is hugely important in this case, because I think the <feed> indicator is something that you'll go looking for once you land on an "interesting" page.

I think it might be odd to have it in a toolbar button. It will be very unclear why the button is disabled for some pages, and enabled for others.

Comment 11

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

I agree with Simon about the toolbar button, however, I don't think that's really a topic for this bug. This bug should just be about a simple implementation which passes off the feed to another app. That implementation should either be in the status bar or location bar (I definitely prefer the status bar). A further implementation can be discussed in bug 287705 which may or may not include a toolbar icon.

Comment 12

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

People are already looking at the bottom right for the progress bar (unless they've got a throbber because they don't like looking down there).  

We used to have the lock down there, so everyone coming from 0.8.x will be used to stuff in the bottom right, too (Firefox still puts the lock down there, plus the site name, and extensions add their bit there, too).  FWIW.

Comment 13

[Nick Kreeger]

Attachment: Updated screen shot, now has the title of the feed found

This is an update to the feed detector, if a feed is found on a page and has a corresponding title, we display the title of the feed instead of the URI. This is very handy on sites like wired.com where there are several feeds with generic URI's.

Comment 14

[Nick Kreeger]

Attachment: First version of the RSS detection patch

This is the first reviewable patch for this bug. It sniffs out feed links on a page, then shoots the commands through the browser UI code to display the feed icon. It also contains the code to draw the popup menu for feeds, which is very simular to the popup blocked button.

Note that this is only the patch, you also need:
- updated BrowserWindow.nib
- updated Localizable.strings
- new feed.tif icon

Comment 15

[Nick Kreeger]

Attachment: Updated BrowserWindow.nib

Comment 16

[Nick Kreeger]

Attachment: Updated Localizable.strings

Comment 17

[Nick Kreeger]

Attachment: New feed.tif icon

This is a temp icon, it is the correct size but will probably need some love before we push a new release. We can file a bug for this if we want

Comment 18

[Nick Kreeger]

Attachment: Oops name change correction

I changed the name of a variable and did not change it in the loop it was being modified in, and forgot to build, oops. This patch builds cleanly!

Comment 19

[Nick Kreeger]

Attachment: Proposed improved UI option

Here is a proposed UI notice so we don't confuse users. We want to try and avoid confusing the users as to what they are doing with the feed. What I have attached is a screen shot (from my working code) of a proposed sheet that runs when a user chooses to "port/subscribe" a feed to an external application. In the sheet is a pop-up button that contains the list of available feeds, and the default application to use with |feed://| is selected by default. If the user changes one of the applications in the pop-up button, and then (we don't want to change their pref if the click Cancel!) if they click Continue, we save the default feed application pref in Launch Services and then open the feed. 

Special thanks goes to Jasper for providing the NIB/mockup!

Comment 20

[Nick Kreeger]

Attachment: Another screenshot from the proposed UI notice

This just shows what apps i get on my 10.4 box

Comment 21

[Nick Kreeger]

Attachment: Another screenshot from the proposed UI notice

This just shows what apps i get on my 10.4 box

Comment 22

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

Comment on attachment 203694 [details]
Another screenshot from the proposed UI notice

So this is going to replace the Necko "External Protocol" sheet  that normally comes up when you hit a feed:// url (e.g., Simon's blog)?

Comment 23

[Nick Kreeger]

Currently, this just slides down when you click on a feed in the popup button from the feed icon that appears when a feed is detected on the page. We *could* add in detection on a feed that is clicked on in the page to show this sheet instead of the launch services (i.e. the sheet gecko sends out).

Comment 24

[Mike Pinkerton (not reading bugmail)]

is directing our users to safari *really* what we want to be doing? they'll just stay there and keep browsing. solution? *shrug*, just something to think about....

Comment 25

[Nick Kreeger]

Attachment: Updated popup button menu

Comment 26

[Chris Casciano]

> is directing our users to safari *really* what we want to be doing?

Is there any other option really? Seems that forcing a block to Safari in the list would cause nothing but headaches (or perhaps some ill will). 

A few other questions while I'm thinking about this stuff this afternoon (apologies for not having a recent build to check myself.

* Does iTunes appear in the list of possible readers (I'm thinking in the case that a "podcast" feed is among the items detected... a user like myself may read most feeds in NNW, but also have MP3 feeds run in itunes)

* This may be out of the scope of this bug, but is there any method available to isntead of subscribing to a feed in a local application instead passing it off to some web location (for example subscribing on bloglines or newsgator)?

Comment 27

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

The real annoyance will be for people on 10.3.9 who discover feeds via Camino.  Safari insists on setting itself as the default handler for feed:// urls even though it is incapable of being a feed reader on 10.3.9 :-(

We're darned if we do and darned if we don't; without this feature, we keep getting bashed by the Mac press-and-pundits as "Camino, the only Mac browser that acts like feeds don't exist", but if we add the feature, we risk Camino users just discovering feeds via Camino defecting to Safari (on 10.4).

Samuel and I were talking about special-casing Safari on 10.3.9 for this dialogue (if user's OS = 10.3.9, don't show Safari in list of feed-supporting apps), which we need for good user experience on 10.3.9, but it makes the code really ugly I'm sure. :-(

Comment 28

[Nick Kreeger]

I agree as well, since safari versions for <= 10.3 for some reason set safari as the default RSS app, we could do some OS version checking, it shouldn't make the code that bad?

Comment 29

[Jasper]

Attachment: new "smaller" feed icon for status bar

Changed the size of the statusbar feed icon to look less cramped

Comment 30

[Simon Fraser [no longer active]]

This icon makes me think of volume, or wireless, but not feed.

Comment 31

[Chris Fonnesbeck]

(In reply to comment #30)
> This icon makes me think of volume, or wireless, but not feed.
> 

It is very similar to the LiveBookmark icon in FireFox.

Comment 32

[Mike Pinkerton (not reading bugmail)]

i never understood why firefox chose that representation, duplicating it seems silly.

Comment 33

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

(In reply to comment #32)
> i never understood why firefox chose that representation, duplicating it seems
> silly.

Particularly since our implementation is entirely different from theirs.  Fx uses that symbol for a bookmark that, when clicked, drops down a list of all the blog entries in a given feed.  Our impl is about noticing a  feed and sending it to an external feed-reader.

Second, the orange airport/volume icon would only be recognizable to Firefox users, whereas the orange/blue lozenges are widely present on the web and should be more readily recognizable to a much larger audience.

I appreciate the desire to make the icon a square like the pop-up blocker, but usability (recognizability/clear differentiation of function) needs to come before pure aesthetics here.

Comment 34

[Jasper]

Option1: I used the firefox like implementation to ensure people understand the icon faster, on the other side I agree that it's totally unclear what it means.

Remember that doing and icon for a phenomena like FEED is almost impossible as it has no actual life like comparison.

Option2: We could use an icon similar to what apple does, but put the text FEED on it. Is the easiest and most readable but we won't be able to localize it.

Option3: I could try and make a small (12/14px) representation of a newspaper. I honestlt thik that would be the most clear icon to do for feed.

Othere then what I descibed above I have no idea how to more clearly visualize FEED.

Comment 35

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

Per pink, this may not make 1.0.

Re-requesting blocking.

Comment 36

[Chris Casciano]

some more comments from the peanut gallery after running this for the afternoon (10.3.9)

* whats being represented with the icon aside, the icon there now feels cramped in the status bar (i'm only seeing 1px of whitespace on the top and bottom of the icon)

* the option to "subscribe to all feeds" seems odd to me. When there are multiple feeds on the same page I've noticed its generally the same content just in different formats (e.g. http://boingboing.net/ ).

* On the following page there some meta data that is being picked up as a feed when it is another fata format entirely (appears as FOAF in the popup menu)

http://michaelraichelson.com/plex/ 

Coded in the following manner:
<link rel="meta" type="application/rdf+xml" title="FOAF" href="http://michaelraichelson.com/foaf.rdf" />

Don't know enough about the conventions being used for linking meta ddata to say if this is a bug in the detection code that needs to be addressed or a symptom of a project picking a silly way to link their data (project at: http://www.foaf-project.org/ ).

* The appearance/indenting of the status information when there is no popup blocked and no feed is still driving me a little nutty, but at this point I'd consider tht another bug.

Overall the combination of NNW + Camino w/ this patch on OS X 10.3.9 is working just fine

Comment 37

[Nick Kreeger]

Looks like I.E. is going to use a simular (or the same?) feed icon as firefox:
http://slashdot.org/article.pl?sid=05/12/19/1522200

Thanks Chris Lawson for the info

Comment 38

[Chris Lawson (gone)]

(In reply to comment #33)
> (In reply to comment #32)
> > i never understood why firefox chose that representation, duplicating it seems
> > silly.
> 
> Particularly since our implementation is entirely different from theirs.

I don't buy this argument at all. Icons represent general concepts, not specific implementations of those concepts. The Dock's Trash icon and the Trash icon that will be added to Camino's Download Manager are entirely different in functionality, but identical in concept. Clicking the Trash icon in the Dock opens the Trash in the Finder. Clicking the Trash icon in the Download Manager will move the selected file(s) to the Trash. Two different implementations, two fairly different functions, but the same general idea and therefore the same (or substantially similar) icon.

Now, I think Fx made a mistake in picking that icon to represent a feed, but now that IE is going to use it as well, I suspect that in six months or so, if we *don't* use this de facto "standard" feed icon, we're going to have a lot of people complaining "Camino doesn't use a standard feed icon! Why not?"

> Second, the orange airport/volume icon would only be recognizable to Firefox
> users, whereas the orange/blue lozenges are widely present on the web and
> should be more readily recognizable to a much larger audience.

See Nick's comment 37 for why this is about to become a complete non-issue. I also think that lozenges with text are a horrible idea from a localisation standpoint, as the text is really Roman-specific at best, and English-specific at worst. "RSS" (which is, IIRC, what Safari uses) has no meaning in non-Roman character sets.

cl

Comment 39

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

(In reply to comment #38)
> I don't buy this argument at all. Icons represent general concepts, not
> specific implementations of those concepts. The Dock's Trash icon and the Trash
> icon that will be added to Camino's Download Manager are entirely different in
> functionality, but identical in concept. Clicking the Trash icon in the Dock
> opens the Trash in the Finder. Clicking the Trash icon in the Download Manager
> will move the selected file(s) to the Trash. Two different implementations, two
> fairly different functions, but the same general idea and therefore the same
> (or substantially similar) icon.
> 

For the record, the "trash" icon we'll be using isn't a standard trash icon. It has an arrow on it which shows the "send to" part of the button's action.

https://bugzilla.mozilla.org/attachment.cgi?id=197789

Comment 40

[Nick Kreeger]

(In reply to comment #39)
> (In reply to comment #38)
> > I don't buy this argument at all. Icons represent general concepts, not
> > specific implementations of those concepts. The Dock's Trash icon and the Trash
> > icon that will be added to Camino's Download Manager are entirely different in
> > functionality, but identical in concept. Clicking the Trash icon in the Dock
> > opens the Trash in the Finder. Clicking the Trash icon in the Download Manager
> > will move the selected file(s) to the Trash. Two different implementations, two
> > fairly different functions, but the same general idea and therefore the same
> > (or substantially similar) icon.
> > 
> 
> For the record, the "trash" icon we'll be using isn't a standard trash icon. It
> has an arrow on it which shows the "send to" part of the button's action.
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=197789
> 

Well in that case what does the "green arrow" mean? Does it mean that it is going to show me the trash or what? I would first look at that icon and think that the green arrow will open the trash or something (but then again i know from the patch what will happen)

Comment 41

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

(In reply to comment #40)
> (In reply to comment #39)
> > 
> > For the record, the "trash" icon we'll be using isn't a standard trash icon. It
> > has an arrow on it which shows the "send to" part of the button's action.
> > 
> > https://bugzilla.mozilla.org/attachment.cgi?id=197789
> > 
> 
> Well in that case what does the "green arrow" mean? Does it mean that it is
> going to show me the trash or what? I would first look at that icon and think
> that the green arrow will open the trash or something (but then again i know
> from the patch what will happen)
> 

I'm not sure what "green arrow" you mean, but if you're talking about anything related to the download manager, that's not meant for this bug.

The point of my comment was that the icon is different than a standard trash icon because the action is different. (I was not arguing whether the icon for that feature was correct or not.) That was in retort to Chris' comment on how they were the same (or similar) which would cause confusion. If the implementation is different, I think the icon should be different.

Comment 42

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

Although it's not clear from my comments in bug, I wasn't advocating using the unlocalizable text found in current lozenges on the web and in Safari.  Sam and I talked about this a bit on irc a while back; I proposed an icon something akin to tractor-feed paper and Sam mentioned something like a newspaper.

As for the trash can "example", that's a curved arrow and it's very clear to me that it means "move to trash", as a curved arrow + object is the standard iconic metaphor for "move to".  And it's different enough from the standard Mac OS X trash icon to allow the user to know it's a trash-related function but not the same Trash thing that the standard Mac OS X trash icon in the Dock does.

We cannot read or parse feeds; we can only parse HTML and pull urls of feeds out of the HTML and then send them to a helper app.  This is significantly different from Firefox's abilities and implementation (which is able to handle feeds in-app and which will only add more advanced management and reading functionality as time goes on, aiui).

Comment 43

[Nick Kreeger]

http://feedicons.com/

Comment 44

[Håkan Waara]

I would like to add my vote to use the new standard RSS icon. As people have pointed out, I think it will have a negative influence on Camino if it's decided now (when actually a new standard is formed) to use another icon.

The good things about using an icon that people in the future will recognize and understand by far outweights the cons of not being able to (at first) understand the abstraction.

Comment 45

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

Please move all discussion regarding which icon we should use to the forum [1] and keep the topic of this bug on the implementation of the actual feature.

[1] http://forums.mozillazine.org/viewtopic.php?t=358072

Comment 46

[Jasper]

Attachment: new FF like feed icon for camino

This is a new  13/13px feed orange icon based on how the one in Firefox looks.

Comment 47

[Nick Kreeger]

Attachment: Posting some updated work

Nothing that is really review ready yet, I just figured that I would post an updated work in progress.

Comment 48

[Nick Kreeger]

Attachment: Updated FeedServiceController.h

This is a new file that controls the feed service controller

Comment 49

[Nick Kreeger]

Attachment: Updated FeedServiceController.mm

New file FeedServiceController.mm, not done yet, but I am posting some progress here. Note: I caught the tabs in the header

Comment 50

[Nick Kreeger]

Comment on attachment 207393 [details]
Updated FeedServiceController.mm

settting mime manually

Comment 51

[Nick Kreeger]

Attachment: FeedServiceController.mm For Review

This is a review ready version of FeedServiceController.mm. This is a special class to drive the UI for the NIBS contained on OpenFeed.nib, which are used to notify a user when they attempt to open feeds. These can be turned off with a pref. 

The rest of the patch will be posted soon.

Comment 52

[Nick Kreeger]

Attachment: FeedServiceController.h For Review

FeedServiceController.h for review, see comments for the attachment on FeedServiceController.mm

Comment 53

[Nick Kreeger]

Attachment: OpenFeed.nib attachement

This is the new NIB for FeedServiceController. It contains 2 nibs, one if the user does have feed applications, and one if the user doesn't.

Comment 54

[Nick Kreeger]

Attachment: Updated Localizable.Strings

New Localizable.strings used in the RSS patch.

Comment 55

[Nick Kreeger]

Attachment: A Feed Icon

This is a feed icon to use, it doesn't have to be the final one, but this is the one that Firefox and I.E. (at least will) both use. We can open another bug to debate the actual icon to use. This can be used for now.

Comment 56

[Nick Kreeger]

Attachment: Updated BrowserWindow.nib

Updated BrowserWindow.nib to contain the RSS changes

Comment 57

[Nick Kreeger]

Attachment: Updated Navigation.nib

Updated Navigation.nib to include RSS changes. Inside the Navigation pref pane, you can select a default feed viewer and set the warn when opening pref. It also listens to changes to the default feed viewer and warn when open pref from FeedServiceController.

Comment 58

[Simon Fraser [no longer active]]

Comment on attachment 208802 [details]
FeedServiceController.mm For Review


> @implementation FeedServiceController
> 
> static FeedServiceController* sInstance = nil;

> static NSString* sDefaultFeedViewerChanged = @"DefaultFeedViewerChanged";
> static NSString* sWarnWhenOpeningFeedPrefChanged = @"WarnWhenOpenFeedPrefChanged";

Use static NSString* const kFoo for these strings.

> /* Init method, set up flags and load the OpenFeed nib */
> -(id)init
> {
>   if ((self = [super init]))
>   {
>     // Assume that an application exists, check when building the popup menu
>     mFeedAppsExist = YES;
> 
>     [NSBundle loadNibNamed:@"OpenFeed" owner:self];
>     [self populateFeedAppsPopUp];

Might be cleaner to implement -awakeFromNib and call -populateFeedAppsPopUp there.


> /* Method to populate the feed popup menu with applications */
> -(void)populateFeedAppsPopUp
> {
>   NSMutableArray* feedApps = [[NSWorkspace sharedWorkspace] installedFeedViewerIdentifiers];

NSMutableArray? Also, if this method returned an NSSet, it would already have duplicates removed.

>   NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"Feed Viewers"] autorelease];
>   NSString* defaultFeedViewerID = [[NSWorkspace sharedWorkspace] defaultFeedViewerIdentifier];
>   int numAppsAdded = 0;
> 
>   // No feed viewer apps exist
>   if ([feedApps count] == 0)
>   {
>     mFeedAppsExist = NO;
>     return;
>   }

Maybe delay making the NSMenu until here.

>   // add a seperator here, we will add the default above it below
>   [menu addItem:[NSMenuItem separatorItem]];
> 
>   NSMenuItem* selectedFeedAppMenuItem = nil;
>   NSString* bundleID = nil;

Declare this inside the for loop.

>   unsigned int numFeedApps = [feedApps count];
>   for (unsigned int i = 0; i < numFeedApps; i++)
>   {
>     bundleID = [feedApps objectAtIndex:i];
> 
>     // Check to make sure that if the user is running a OS version less than tiger,
>     // that the only feed app is Safari. Safari registers itself in 10.3.9 as an app
>     // that handles "feed://" URLs. If this is the case, pull Safari out of the array.
>     if (![NSWorkspace safariSupportsRSS] && [bundleID isEqualToString:@"com.apple.Safari"])
>     {
>       // if safari is the only application for RSS
>       if ([feedApps count] == 1)
>         mFeedAppsExist = NO;
> 
>       continue;
>     }
> 
>     // check to see if the application has already been added

See above. This algorigthm is potentially slow.


>     NSMenuItem* item = [[NSMenuItem alloc] initWithTitle:[[NSWorkspace sharedWorkspace] displayNameForFile:appURL]
>                                                   action:nil keyEquivalent:@""];

This item is leaked.

>   // allow user to select a feed viewer
>   NSMenuItem* selectItem = [[NSMenuItem alloc] initWithTitle:NSLocalizedString(@"Select...", nil)
>                                     action:@selector(hitSelectMenuItem:) keyEquivalent:@""];

This item is leaked too.

>   if (selectedFeedAppMenuItem == nil)
>   {
>     NSURL* defaultFeedViewerURL = [[NSWorkspace sharedWorkspace] urlOfApplicationWithIdentifier:defaultFeedViewerID];
>     if (!defaultFeedViewerURL)
>       return;

Really return and give up on all this hard work?

>   [mFeedAppsPopUp selectItem:selectedFeedAppMenuItem];

Don't you want to set the menu, and then select the item?
Also, do I recall that NSPopUpButton eats the first item in the menu?

>   [mFeedAppsPopUp setMenu:menu];
> 
>   [selectedFeedAppMenuItem release];
> }

> -(int)runOpenDialog:(id)containerWindow
> {
>   NSOpenPanel* openPanel = [NSOpenPanel openPanel];
>   [openPanel setCanChooseDirectories:NO];
>   [openPanel setAllowsMultipleSelection:NO];
>   [openPanel beginSheetForDirectory:nil
>                                file:nil
>                               types:[NSArray arrayWithObject:@"app"]
>                      modalForWindow:containerWindow
>                       modalDelegate:self
>                      didEndSelector:@selector(openPanelDidEnd:returnCode:contextInfo:)
>                         contextInfo:nil];

Looking for .app extensions won't work for old-school non-bundle Carbon apps. Do we care?

> }

Um, this method doesn't return a value, yet claims to return an int.
Since beginSheetForDirectory is modeless, it should return void.


> -(void)openPanelDidEnd:(NSOpenPanel*)sheet returnCode:(int)returnCode contextInfo:(void *)contextInfo
> {
>   if (returnCode == NSOKButton)
>   {
>     // we are only allowing one item to be selected
>     NSArray* selectedURL = [sheet URLs];
>     NSURL* itemURL = [selectedURL objectAtIndex:0];

Clearer as:

    NSURL* itemURL = [sheet URLs] objectAtIndex:0];

>     if (itemURL)

NSArray with throw an exception rather than give you an item for a non-existent index.

>       // coming from notifyOpenExternalFeedApp dialog, add the item to the pop up list
>       if ([sheet parentWindow] == notifyOpenExternalFeedApp)

I don't like the way the window is used to decide what to do. It would be better to use
2 different "didEnd" selectors on the open panel.

>         NSMenuItem* selectedAppMenuItem = [[NSMenuItem alloc] initWithTitle:[[NSWorkspace sharedWorkspace] displayNameForFile:itemURL]
>                                                                      action:nil keyEquivalent:@""];
>         NSImage* icon = [[NSWorkspace sharedWorkspace] iconForFile:[[itemURL path] stringByStandardizingPath]];
>         [icon setSize:NSMakeSize(16.0, 16.0)];
>         [selectedAppMenuItem setImage:icon];
>         [selectedAppMenuItem setRepresentedObject:appBundleID];
>         [selectedAppMenuItem setEnabled:YES];
> 
>         NSMenu* popupMenu = [mFeedAppsPopUp menu];
>         [popupMenu insertItem:selectedAppMenuItem atIndex:0];
>         [mFeedAppsPopUp selectItem:selectedAppMenuItem];
> 
>         [selectedAppMenuItem release];

So you stuff a menu item into the popup menu here, but just below you call
-populateFeedAppsPopUp. What gives?


> /* Action method that runs the open panel for a user to select a specific RSS viewing application */
> -(IBAction)hitSelectButton:(id)sender
> {
>   int code = [self runOpenDialog:notifyNoFeedAppFound];
> 
>   // if the user selected something and did not cancel, stop the dialog
>   if (code == NSAlertDefaultReturn)
>   {
>     [NSApp stopModalWithCode:code];
>     [self checkWarnWhenOpenCheckboxState];

Some confustion about open panel modality here.

> /* Set the passed in application bundleID as the default feed viewer */
> -(void)setDefaultFeedViewerWithIdentifier:(NSString*)inBundleID
> {
>   NSBundle* defaultAppID = [NSBundle bundleWithIdentifier:[[NSWorkspace sharedWorkspace] defaultFeedViewerIdentifier]];
> 
>   // if the user selected something other than the default application
>   if (![inBundleID isEqualToString:[defaultAppID bundleIdentifier]])
>   {
>     NSURL* appURL = nil;
>     // attempt to get the URL of the selected application, if we get the URL, set it as the default RSS viewer app.
>     if (LSFindApplicationForInfo(kLSUnknownCreator, (CFStringRef)inBundleID, NULL, NULL, (CFURLRef*)&appURL) == noErr)

Is this different from  calling
    [[NSWorkspace sharedWorkspace] urlOfApplicationWithIdentifier:bundleID]?


> /* Run the notification sheet based on if the user has a valid RSS reader application or not */
> -(int)showFeedWillOpenDialog:(NSWindow*)parent
> {
>   // no feeds exist for the user, run the appropriate dialog to notify the user
>   if (!mFeedAppsExist)
>   {
>     int result = [NSApp runModalForWindow:notifyNoFeedAppFound relativeToWindow:parent];
>     [notifyNoFeedAppFound close];

-runModalForWindow:relativeToWindow: is a nasty hack and deprecated. Can we do this
with a normal sheet?

>   // set the mFeedURI variable so that we can be flexible in opening feeds.
>   // if the user selects remember my choice and set reader as default,
>   int result = [NSApp runModalForWindow:notifyOpenExternalFeedApp relativeToWindow:parent];

Ditto.

Comment 59

[Simon Fraser [no longer active]]

Comment on attachment 208803 [details]
FeedServiceController.h For Review

> #import <Cocoa/Cocoa.h>
> 
> @interface FeedServiceController : NSObject 
> {
>   IBOutlet id             notifyOpenExternalFeedApp;  // shown when there are RSS viewing apps
>   IBOutlet id             notifyNoFeedAppFound;       // shown when there is not a RSS viewing app registered

Prefix these with 'm', and use concrete classes please.

>   IBOutlet NSPopUpButton* mFeedAppsPopUp;             // pop-up that contains the registered RSS viewer apps
>   IBOutlet NSButton*      mSelectAppButton;           // button that allows user to select a RSS viewer app
>   
>   BOOL                    mFeedAppsExist;             // true when there is at least one registered feed viewer app
> }
> 
> +(FeedServiceController*)instance;

"sharedFeedServiceController" ?

> -(IBAction)hitButtonCancel:(id)sender;
> -(IBAction)hitButtonOpen:(id)sender;
> -(IBAction)hitContinueButton:(id)sender;
> -(IBAction)hitSelectButton:(id)sender;
> -(IBAction)hitSelectMenuItem:(id)sender;

Add comments to say where these are called from.

Comment 60

[Simon Fraser [no longer active]]

Can you post a couple of screenshots of the nibs in action?

Comment 61

[Nick Kreeger]

>     NSMenuItem* item = [[NSMenuItem alloc] initWithTitle:[[NSWorkspace sharedWorkspace] displayNameForFile:appURL]
>                                                   action:nil keyEquivalent:@""];

On these NSMenuItems, do I need to be autoreleasing them or just plain releasing them after they have been added to the NSMenu?

Comment 62

[Nick Kreeger]

> -(int)runOpenDialog:(id)containerWindow
> {
>   NSOpenPanel* openPanel = [NSOpenPanel openPanel];
>   [openPanel setCanChooseDirectories:NO];
>   [openPanel setAllowsMultipleSelection:NO];
>   [openPanel beginSheetForDirectory:nil
>                                file:nil
>                               types:[NSArray arrayWithObject:@"app"]
>                      modalForWindow:containerWindow
>                       modalDelegate:self
>                      didEndSelector:@selector(openPanelDidEnd:returnCode:contextInfo:)
>                         contextInfo:nil];

Looking for .app extensions won't work for old-school non-bundle Carbon apps.
Do we care?

> }

I haven't been able to find a old-school non-bundle Carbon RSS app that will accepts feed URL's from another application. So, I would think we shouldn't care.

Comment 63

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

Pushing off to 1.1...

Comment 64

[Nick Kreeger]

Since we moved the pop-up dialog out of the status bar, do we still want to keep the RSS notice icon down there or perhaps move it to the toolbar?

Comment 65

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

(In reply to comment #64)
> Since we moved the pop-up dialog out of the status bar, do we still want to
> keep the RSS notice icon down there or perhaps move it to the toolbar?

Please implement it in the status bar first, then file a follow up bug (after the fix for this bug lands), so we can have discussion on if/where it should be moved.

Comment 66

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

Also note bug 287705 comment 36 for information on Fx's implementation.

Comment 67

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

Per pink: "we should be using the feed detection, etc that they've landed for ff"

Info from beng (some of this is overkill for this bug):

* Feed Handling for Firefox 2 checked in and turned on on the trunk and the 1.8 branch.
* Most of the code is not Firefox specific, and may be of interest to any feed handling functionality you wish to add to Camino.

- an IE7 compatible feed sniffer, to detect any feed content regardless of how incorrectly it was served (e.g. as text/html)
- a Feed Processor - a XPCOM js component that parses all different types of feed formats
- a Feed Converter - a XPCOM js component/stream listener that handles loading a preview page in the browser window when a feed is encountered

* Standardizing on this Feed Processor across various feed usages in Firefox and Thunderbird

* It lives in toolkit/components/feeds

* The rest of the code lives in browser/components/feeds

* Aside from the above, there is also the preview page itself and
options UI, which are Firefox specific but might be useful to look at.

* Nice things in a recent Minefield or branch nightly: the ability to have a web service like bloglines etc automatically handle links to feed content so that when you go to say news.com and click on the XML icon you get taken straight to the landing page for that service.

* For reference, here is my post to dev.apps.firefox which talks about the requirements of the Firefox system:
http://groups.google.com/group/mozilla.dev.apps.firefox/msg/f598759e5f5ddf5b

Comment 68

[Nick Kreeger]

Attachment: Updated FeedServiceController.mm

Here is an updated FeedServiceController.mm, Simon when you get a spare moment, could you take a look at it?

Comment 69

[Nick Kreeger]

Attachment: Updated FeedServiceController.h

Here is the new header...

Comment 70

[Nick Kreeger]

Attachment: RSS Detection Patch1

Here is the first go-round of the RSS detection support patch. Simon, Mento, and/or Pink could one of you guys please take a look at this?

Comment 71

[Nick Kreeger]

Attachment: New FeedServiceController.h

Here is an updated FeedServiceController.h

Comment 72

[Nick Kreeger]

Attachment: Updated FeedServiceController.mm

Updated FeedServiceController.mm

Comment 73

[Nick Kreeger]

Attachment: Updated BrowserWindow.nib

This contains the updated BrowserWindow.nib to include the RSS icon.

Comment 74

[Nick Kreeger]

Attachment: Updated Navigation.nib

This is an updated Navigation.nib to include pref's for RSS (default viewing app and warn when opening feeds).

Comment 75

[Nick Kreeger]

Attachment: New OpenFeed.nib

This is the nib that contains the two windows for warning users when a feed URL is opened.

Comment 76

[Nick Kreeger]

Attachment: Updated Localizable.strings

Contains necessary localized strings for feeds.

Comment 77

[Nick Kreeger]

Attachment: The standard feed icon

This contains the RSS feed icon we will use at least for now. This is discussion for another bug.

Comment 78

[Nick Kreeger]

Attachment: Updated RSS patch

This patch fixes a bug when asking for a set of default feed identifiers.

NSSet* apps = nil;

Rather than:

NSet* apps;

Special thanks goes to mento.

Comment 79

[Nick Kreeger]

Attachment: Updated patch

This is just to remove the output from a diff on nibs, sorry for the bug spam

Comment 80

[Nick Kreeger]

Attachment: Updated FeedServiceController.mm

This is to catch the problem of mFeedsDetected not being set correctly in |awakeFromNib|.

Comment 81

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

Comment on attachment 222196 [details]
Updated BrowserWindow.nib

This needs a new nib now that bug 313083 has landed.

Comment 82

[Nick Kreeger]

Attachment: Un-rotted patch

Rather than posting new nibs each time they rot, I will post them after the code looks good (or if they are requested).

This patch is special because it contains FeedServiceController.h/.mm rather than separate attachments.

Oh, here is another screen shot of the spacing of the icon and the status text: www.nkreeger.com/camino/rssshot.png

Comment 83

[Chris Casciano]

using build provided last night I'm seeing some buggy behavior with feed selection.

10.3.9/OS X, yesterdays trunk + patch, handing off to NNW as my default feed reader


http://blogs.msdn.com/webdevtools/default.aspx

2 feeds on that page seem reversed in the popup dialog (e.g. label doesn't match feed url)

I've also noticed a few times where the first popup of the dialog the first item isn't labeled properly (either a duplicate of the 2nd label from the top, or blank in the case of a page with a single feed.

I've seen both above cases with the following url which currently has 4 feeds listed.

http://placenamehere.com/

Comment 84

[Nick Kreeger]

(In reply to comment #83)
> using build provided last night I'm seeing some buggy behavior with feed
> selection.
> 
> 10.3.9/OS X, yesterdays trunk + patch, handing off to NNW as my default feed
> reader
> 
> 
> http://blogs.msdn.com/webdevtools/default.aspx
> 
> 2 feeds on that page seem reversed in the popup dialog (e.g. label doesn't
> match feed url)
> 
> I've also noticed a few times where the first popup of the dialog the first
> item isn't labeled properly (either a duplicate of the 2nd label from the top,
> or blank in the case of a page with a single feed.
> 
> I've seen both above cases with the following url which currently has 4 feeds
> listed.
> 
> http://placenamehere.com/
> 

Chris,

I can't seem to reproduce, can you please take a screen shot (or two?).

Comment 85

[Nick Kreeger]

Attachment: Updated patch

This is an updated patch to correct the backwards-assignment of the feed URI's. The problem was I was indexing the selected item from the popup menu and attempting to get that out of the NSArray, oops.

This makes things simpler by assigning a representedObject to the menu item with the menu items associated feed uri.

Comment 86

[Chris Casciano]

Attachment: sceenshot of behavior from c83

This is a screenshot of the behavior I saw where the first feed item label is duplicated on first open. This is with the previous patch, not the current one. Screenshot shows initial display on opening and then 2nd (and corrected) attempt to open the menu. This is *not* reproducible always, on 10.3.9 though I have seen it on more then one occasion.

Comment 87

[Mike Pinkerton (not reading bugmail)]

i hate coming in at the 11th hour with something like this but...

we should put the feed icon in the url bar like every other modern browser. We shouldn't deviate from where users have been trained to expect it without a really really good reason, whether we like it or not. I don't feel that we have a compelling reason to put it in the status bar (especially as we try to get rid of it), so to do so seems different just to be different.

smfr and my discussion was about bottom left v. bottom right, not really about the merits of having it in the status bar.

Comment 88

[Jasper]

I agree let's put it in the url bar ;)

Comment 89

[Stuart Morgan]

I know this is ongoing point of contention, but in files where the style is already correct (Navigation.mm), please use Mozilla style:

if ()/while ()/else {

rather than

if ()/while ()/else
{

Ideally also for files where it's largely undecided (BWC.mm) or the file is new (FeedServiceController.mm)--it would be nice to minimize the introduction of new code that is against the style guidelines in support of trying to get all the code to match someday.

Comment 90

[Nick Kreeger]

Attachment: URL Bar with RSS

Here is a screen shot with the RSS icon in the URL bar, the icon will need to be resized, but it is a start.

Comment 91

[Nick Kreeger]

Attachment: Patch 1

Here is the reviewable patch for the move to the URL bar. Stuart could you please review this for me?

Comment 92

[Chris Casciano]

2 cosmetic requests after running the posted to irc build from a night or two ago

(1) The order items appear in the popup menu are in the opposite order that they appear in the head of the document. not "wrong" (there is no right here) but its odd to see things like comments feeds first (see http://placenamehere.com/ )

(2) I'd love to see just a little more margin on the left side of the icon so long URLs don't get so close... seems like the  new icon has about half the whitespace around it as the lock icon does.

neither of these concerns should hold the patch up.. .and I'd be happy to move them to new bugs after this landss .

Comment 93

[Nick Kreeger]

One note:

+- (void)mouseDown:(NSEvent *)theEvent
+{
+  // By default a right-click will show the context menu for our image view subclass, 
+  // but a left click will not show a context. Since we want to show the context menus
+  // on a left click, foward this mouse down event as a right mouse down event.
+  [super rightMouseDown:theEvent];
+}
+

The call shouldn't be to the |super|, but rather |self|.

Comment 94

[Stuart Morgan]

Comment on attachment 230197 [details] [diff] [review]
Patch 1

>+// method to be called so that when the default feed viewer is modified
>+// in FeedServiceController, we can rebuild the list here as well.
>+-(void)updateDefaultFeedViewerMenu;


Move this out of the header since it's only meant to be called internally. Also remove the comment, since it's used for cases that have nothing to do with FSC as well.

>+const int kSelectMenuItemTag = -5;

This can be local to the function it's used in.  Unless there's a reason you can't use -1, do that instead--magic numbers are confusing.

+static NSString* const kWarnWhenOpeningFeedPrefChanged = @"WarnWhenOpenFeedPrefChanged";

Per my understanding of your discussion with pink, this pref UI should be removed entirely, as users don't need a repeated warning for doing something that they have to explicitly chose to do. So all that's really needed is a one-time warning, and to allow picking the feed viewer if they haven't already.

>+- (void)openPanelDidEndToSelectBrowser:(NSOpenPanel*)sheet returnCode:(int)returnCode contextInfo:(void*)contextInfo;
>+- (void)openPanelDidEndToSelectFeedViewer:(NSOpenPanel*)sheet returnCode:(int)returnCode contextInfo:(void*)contextInfo;

Having the DidEnd in the middle of the description of the panel is a bit awkward. How about browserSelectionPanelDidEnd:... and feedSelectionPanelDidEnd:...?

>+ if ([self getBooleanPref:"camino.warn_before_open_feed" withSuccess:&gotPref])


Should be camino.warn_before_opening_feed

>+  // its not a feed viewer that was selected, so the user must want to select a feed viewer

Remove this; the name of the method makes it clear what's happening

>+    // attempt to get the URL of the selected application, if we get the URL, set it as the default RSS viewer app.
>+    NSURL* appURL = [[NSWorkspace sharedWorkspace] urlOfApplicationWithIdentifier:inBundleID];
>+    if (appURL) {
>+      NSBundle* appBundleID = [NSBundle bundleWithPath:[appURL path]];
>+      [[NSWorkspace sharedWorkspace] setDefaultFeedViewerWithIdentifier:[appBundleID bundleIdentifier]];
>+    }
>+    // could not find information for the selected app, warn the user
>+    else {
>+      NSRunAlertPanel(NSLocalizedString(@"Application does not exist", nil),
>+                      NSLocalizedString(@"FeedAppDoesNotExistExplanation", nil),
>+                      NSLocalizedString(@"OKButtonText", nil), nil, nil);
>+    }


It seems like this validation should happen when building the popup list, not when they pick the app; why give them the apparent option of picking something that won't work?  Where ever this code ends up, appBundleID needs to either be renamed or be the bundle ID (not the bundle itself)

>+- (void)openPanelDidEndToSelectFeedViewer:(NSOpenPanel *)sheet returnCode:(int)returnCode contextInfo:(void *)contextInfo
>+{
>+  if (returnCode == NSOKButton) {
>+    NSURL* itemURL = [[sheet URLs] objectAtIndex:0];
>+    NSBundle* appBundleID = [NSBundle bundleWithPath:[itemURL path]];


Again, no NSBundles named appBundleID

>+  // Clear the menu to the "Select.." menu item, which has a tag of -5, 
>+  // then move back one position to include the seperator.
>+  int numItemsToDelete = [defaultFeedViewerPopUp indexOfItemWithTag:kSelectMenuItemTag] - 1;

If what you want is the position of that separator, why not tag the separator directly?

>+  for (int i = 0; i < numItemsToDelete; i++) 
>+    [defaultFeedViewerPopUp removeItemAtIndex:0];
>+  
>+  // After removing items in a NSPopUpButton's menu and then re-adding them, the standard
>+  // trick of inserting a blank item that gets swallowed by the popup does not work here. The blank
>+  // item remains in the list.

AFAIK it's not a general property of NSPopUpButtons that the first menu item vanishes.  I think what you are describing is expected behavior, so it doesn't need a comment

>+  // Without inserting the blank item, the menu's items are inserted inconsistently.
>+  // To get around this, create a "dummy" blank menu item and insert it into the list.

This sounds like there is another problem with the insertion logic that should be fixed, instead of worked around

>+  // If we do not find a default application for feed viewing, leave the "dummy" item in the list, and
>+  // select it. This will give the appearance to the user that a default feed viewer has not been
>+  // selected on his/her system.

So then this should be done only later, if needed, rather than up front and then undone later.

>+    // Check to make sure that if the user is running a OS version less than tiger,
>+    // that the only feed app is Safari. Safari registers itself in 10.3.9 as an app
>+    // that handles "feed://" URLs. If this is the case, pull Safari out of the array.

Not counting Safari on 10.3.9 should be moved into the logic for defaultFeedViewerIdentifier and installedFeedViewerIdentifiers--since it's not really a feed viewer, it shouldn't be returned by those methods.

>+    // Check to see if this is the default feed viewer, if it is, insert it above the separator.
>+    if ([bundleID isEqualToString:defaultFeedViewerID]) {
>+      insertionIndex = 0;
>+      insertedDefaultApp = YES;
>+    }
>+    else
>+      insertionIndex = 1;

The only separator that exists right now is the one above "Select...", and everything goes above it, not just the first, so the comment is misleading.  (If the code here was the same before your dummy item, the inconsistency you mentioned would have been due to index 1 being after that lower separator until the default app has been inserted.)

>+  if (!insertedDefaultApp) {
>+    NSURL* defaultFeedViewerURL = [[NSWorkspace sharedWorkspace] urlOfApplicationWithIdentifier:defaultFeedViewerID];

Why the round-trip through the ID, rather than using your +[NSWorkspace defaultFeedViewerURL] directly?

>+    if (defaultFeedViewerURL) {
>+      NSMenuItem* curItem = [[NSMenuItem alloc] initWithTitle:[[NSWorkspace sharedWorkspace] displayNameForFile:defaultFeedViewerURL]
>+                                                       action:@selector(defaultFeedViewerChange:)
>+                                                keyEquivalent:@""];
>+      
>+      NSImage* icon = [[NSWorkspace sharedWorkspace] iconForFile:[[defaultFeedViewerURL path] stringByStandardizingPath]];
>+      [icon setSize:NSMakeSize(16.0, 16.0)];
>+      [curItem setImage:icon];
>+      [curItem setRepresentedObject:[NSBundle bundleWithPath:[defaultFeedViewerURL path]]];
>+      [curItem setTarget:self];
>+      [curItem setEnabled:YES];
>+      
>+      [[defaultFeedViewerPopUp menu] insertItem:curItem atIndex:0];
>+      [curItem release];
>+    }

That's a lot of code duplicated from above; either move it to a helper method, or pre-insert the default app into the looped array above in the case where it's not already there. (In fact, since installedFeedViewerIdentifiers already does insert the default viewer, isn't this code path impossible to hit?)

>+  // If more than one application was added, insert another separator
>+  if (numAppsAdded > 1)
>+    [[defaultFeedViewerPopUp menu] insertItem:[NSMenuItem separatorItem] atIndex:1];
>+  
>+  // If we did add at least one application, remove the dummy item, if not, leave it there.
>+  if (numAppsAdded > 0)
>+    [[defaultFeedViewerPopUp menu] removeItem:dummyItem];


Is it possible for there to be only one item and for it not to be the default?  If so, this won't work quite right, since there should be a blank item shown as selected, then a separator, then the one item.

>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/AutoCompleteTextField.h,v
...
>+@class BrowserWindowController;
...
>+  BrowserWindowController*  mBrowserWindowController;  // weak, shortcut to to our BWC
...
>+- (void)setBrowserWindowController:(BrowserWindowController*)aWindowController;


It seems kind of backwards for the ATCF to have to know about BWC.  Is there any way the information it needs from BWC could be pushed in from outside instead?

>+- (void)validateFeedIconPosition;

positionFeedIcon

>   if (mDisplaySecureIcon)
>     theRect.size.width -= kSecureIconRightOffset;
>+  if (mDisplayFeedIcon)
>+    theRect.size.width -= kFeedIconRightOffest;

Use your new accessors, rather than the member variables.

>+// -isSecureIconDisplayed:
>+//
>+// Accessor method to help the layout the feed icon to the left of the security icon. This
>+// method is used when |
>+//
...
>+// -isFeedIconDisplayed:
>+//
>+// Accessor method to help the layout the feed icon to the left of the security icon. This
>+// method is used when |displaySecureIcon:| is called to determine if the feed icon has been
>+// displayed before the security icon.


I think these methods are self-explanatory. If the comments stay though, comment what the methods do, not who the current clients happen to be.

>-@interface LockImageView : NSImageView
>+@interface TextFieldImageView : NSImageView
> @end
> 
>-@implementation LockImageView
>+@implementation TextFieldImageView

I don't see anything about this class that relates to being in a TextField; it's all just additional menu stuff.

>+- (void)displayFeedIcon:(BOOL)inDetected
>+{
>+  if (inDetected && ![mFeedIcon superview]) {
>+    [mFeedIcon setImage:[BrowserWindowController feedIcon]];

Is there a reason you'd need to set the image more than once, and can't do this when the view is created?

>+    [[self cell] setDisplayFeedIcon:YES];
>+    [self validateFeedIconPosition];
>+    [self addSubview:mFeedIcon];
...
>+    [[self cell] setDisplayFeedIcon:NO];
>+    [mFeedIcon removeFromSuperview];

Shouldn't calling setDisplayFeedIcon do the adding/removing?  That seems like a critical part of making it display/not display.

>+    // if there was no feed title, or the length of the string is 0, append the feed's URI
>+    if (!feedTitle || [feedTitle length] == 0)
>+      itemTitle = [NSString stringWithFormat:title, feedURI];
>+    
>+    // there is a title for the feed
>+    else
>+      itemTitle = [NSString stringWithFormat:title, feedTitle];

Reverse the order here, and remove the comments (the code is self-explanatory).  No blank space separating an else from the associated if, please.

>+    // ensure that the item with the same title hasn't been inserted already

You might want to make clear that this for clarity to the user; there's no hard restriction for an NSMenu.

>+      [mFeedIconContextMenu insertItem:curFeedMenuItem atIndex:insertionIndex];


instertionIndex should be incremented on insert so order is preserved.


>+// -showFeedDetected:
>+//
>+// Called when a feed link tag has been sniffed out,

Isn't that only the :YES calls?

>+// -feedList
...
>+// Called by AutoCompleteTextField.


Again, I'm not generally a fan of documenting callers of methods. Methods shouldn't generally care who calls them, and it's likely to become incorrect over time.

>+-(BOOL)shouldWarnBeforeOpeningFeed
>+{
>+  if ([[PreferenceManager sharedInstance] getBooleanPref:"camino.warn_before_open_feed" withSuccess:NULL])
>+    return true;
>+  
>+  return false;
>+}

return [[PreferenceManager sharedInstance] getBooleanPref:"camino.warn_before_open_feed" withSuccess:NULL];

>+// -openFeedInExternalApp
...
>+  else
>+    [[NSWorkspace sharedWorkspace] openURL:[NSURL URLWithString:[sender representedObject]]];

Doesn't this need to make sure there's a default feed viewer selected, and show the user a dialog if not?

>+    mFeedList = [[NSMutableArray alloc] init];

Why keep an empty array around rather than nil in the no-feed case?

>+- (BOOL)feedsDetected
>+{
>+  if (!mFeedList)
>+    return NO;
>+  
>+  return ([mFeedList count] > 0);
>+}

return (mFeedList && [mFeedList count] > 0);

>+- (void)onFeedDetected:(NSString*)inFeedURI feedTitle:(NSString*)inFeedTitle;
>+{
>+  // add the two in variables to a dictionary, then store in the feed array
>+  NSMutableDictionary* feed = [[NSMutableDictionary alloc] init];
>+  [feed setObject:inFeedURI forKey:@"feeduri"];
>+  [feed setObject:inFeedTitle forKey:@"feedtitle"];
>+  [mFeedList addObject:feed];
>+  // notify the browser UI that a feed was found
>+  [mDelegate showFeedDetected:YES];
>+}

NSDictionary *feed = [NSDictionary dictionaryWithObjectsAndKeys:inFeedURI, @"feeduri", inFeedTitle, @"feedtitle", nil];
That avoids the NSMutableDictionary overhead that you don't need (and as an added bonus it won't leak the feed info dictionaries).


>Index: src/browser/FeedServiceController.mm


Most of this file seems to be duplicate code from the pref pane.  There needs to be one common implementation of as much as possible.

> protected:
> 
>   nsresult HandleBlockedPopupEvent(nsIDOMEvent* inEvent);
>   nsresult HandleLinkAddedEvent(nsIDOMEvent* inEvent);
>   
> private:
>+  PRBool HandleFaviconLink(nsIDOMElement* inElement, nsIDOMDocument* inDocument);
>+  PRBool HandleFeedLink(nsIDOMElement* inElement, nsIDOMDocument* inDocument);


Why private instead of protected?

>-  nsAutoString relAttribute;
>-  linkElement->GetAttribute(NS_LITERAL_STRING("rel"), relAttribute);
>-
>-  if (!relAttribute.EqualsIgnoreCase("shortcut icon") && !relAttribute.EqualsIgnoreCase("icon"))
>-    return NS_OK;

There are still a whole lot of link elements we don't care about.  Don't we still want to short-circuit, and just add "alternate" to the list?

>+  
>+  // now pass off to the two Handle methods
>+  if (!HandleFaviconLink(linkElement, domDoc))
>+    HandleFeedLink(linkElement, domDoc);

Break it out based on the relAttribute, not to both.

>+}

This needs to return NS_OK

>+PRBool
>+CHBrowserListener::HandleFaviconLink(nsIDOMElement* inElement, nsIDOMDocument* inDocument)
...
>+PRBool
>+CHBrowserListener::HandleFeedLink(nsIDOMElement* inElement, nsIDOMDocument* inDocument)


These don't need to return anything, since it doesn't actually matter to anyone what happens inside.

>+  nsAutoString titleAttr;
>+  rv = inElement->GetAttribute(NS_LITERAL_STRING("title"), titleAttr);
>+  if (NS_FAILED(rv))
>+    return PR_FALSE;

Given that there is fallback code in the UI for feeds with no titles, why is this a fatal error?

>+- (NSSet*)installedFeedViewerIdentifiers
>+{
>+  NSArray* apps = nil;
>+  NSMutableSet* feedApps = [[[NSMutableSet alloc] init] autorelease];
>+  BOOL defaultFeedAppAdded = NO;

Since you are using a set anyway, just always add the default app at the beginning rather than doing all the manual tracking of whether you've seen it yet.

Comment 95

[Nick Kreeger]

Attachment: Patch 2

Here is round two, with a lot of changes and re-factorization especially in Navigation.mm. I added a new class that didn't have the extra dependencies that are found in FeedServiceController, so that we the footprint of the Navigation pref pane target remains small. The class currently is dubbed "AppListMenuFactory", (if you have a better name please suggest). It builds the default application menus for web browser and feed viewer selection. It also contains helper methods for setting the default browser/feed viewer from data that could possibly be stale.

Smorgan -> Please take a look when you get some time :-)

Comment 96

[Stuart Morgan]

Comment on attachment 231864 [details] [diff] [review]
Patch 2

Looks pretty good, so it's mostly just smaller stuff remaining:

>+/*
>+ Define a simple class to share some of the building tasks for populating
>+ feed application popups. Probably something else here would be nice as well
>+ */


I think the second sentence can probably go ;)

>+-(void)buildFeedAppsMenuForPopup:(NSPopUpButton*)inPopupButton 
>+                      withAction:(SEL)inAction 
>+                withSelectAction:(SEL)inSelectAction
>+                      withTarget:(id)inTarget;
>+
>+-(void)buildBrowserAppsMenuForPopup:(NSPopUpButton*)inPopupButton
>+                         withAction:(SEL)inAction
>+                   withSelectAction:(SEL)inSelectAction
>+                         withTarget:(id)inTarget;

Stardard naming style is something:withFoo:andBar:andBaz, rather than repeating the 'with'.

>+-(BOOL)attemptDefaultFeedViewerRegistration:(NSString*)inBundleID;
>+-(BOOL)attemptDefaultBrowserRegistration:(NSString*)inBundleID;

It's not very clear from the name what the 'attempt' part of this is.  How about validateAndRegisterDefaultFoo:?  And since they present UI and don't actually have to do with building the menus, could they be put into FeedServiceController?

>+-(NSMenuItem*)createMenuItemForAppURL:(NSURL*)inURL 
>+                withRepresentedObject:(id)inRepresentedObject 
>+                          withAction:(SEL)inAction
>+                          withTarget:(id)inTarget;


make this menuItemForAppURL:... instead of createMenuItemForAppURL:..., and autorelease the return to make it less likely a caller will accidentally leak.  And since it's always going to be a bundle ID here, perhaps withBundleId: instead of withRepresentedObject:?

>+static int CompareBundleIDAppDisplayNames(id a, id b, void *context)
>+{
>+  NSURL* appURLa = nil;
>+  NSURL* appURLb = nil;
>+  
>+  if ((LSFindApplicationForInfo(kLSUnknownCreator, (CFStringRef)a, NULL, NULL, (CFURLRef*)&appURLa) == noErr) &&
>+      (LSFindApplicationForInfo(kLSUnknownCreator, (CFStringRef)b, NULL, NULL, (CFURLRef*)&appURLb) == noErr))
>+  {
>+    NSString *aName = [[NSWorkspace sharedWorkspace] displayNameForFile:appURLa];
>+    NSString *bName = [[NSWorkspace sharedWorkspace] displayNameForFile:appURLb];
>+    return [aName compare:bName];
>+  }
>+  
>+  // this shouldn't ever happen, and if it does we handle it correctly when building the menu.
>+  // there is no way to flag an error condition here and the sort fuctions return void
>+  return NSOrderedSame;
>+}

aName and bName need to be nil checked; aName being nil will cause this to return whatever 0 happens to map to, and bName being nil would throw an exception.  Also, appURLa and appURLb are leaked here.

>+-(void)buildFeedAppsMenuForPopup:(NSPopUpButton*)inPopupButton 
>+                      withAction:(SEL)inAction 
>+                withSelectAction:(SEL)inSelectAction
>+                      withTarget:(id)inTarget
>+{
...
>+    // Since we don't add Safari to the list of available feed apps, skip it.

This comment makes it sound like it's a function of some other code, when in fact this is the implementation of not adding Safari.  The comment should just say something like // Don't add Safari to the list

...
>+    if ([curBundleID isEqualToString:defaultFeedViewerID]) {

defaultFeedViewerID could be nil, in which case this will throw an exception. It needs to be |if (defaultFeedViewerID && [...])|

>+  [selectItem setEnabled:YES];

Isn't that the default state for a new MenuItem?


>+// -buildBrowserAppsMenuForPopup: withAction: withSelectAction: withTarget:

No spaces.

>+// After building a menu for the feeds found on the page, it sets the feed
>+// icons menu for the URL bar.
>+//
>+-(void)buildBrowserAppsMenuForPopup:(NSPopUpButton*)inPopupButton

This comment got the wrong method.

>+    if ([bundleID isEqualToString:currentDefaultBrowserBundleID])

currentDefaultBrowserBundleID should be nil-checked as well

>+  // sort the list by display name
>+  NSMutableArray* browsers = [[browsersSet allObjects] mutableCopy];
>+  [browsers sortUsingFunction:&CompareBundleIDAppDisplayNames context:NULL];
>+  
>+  return [browsers autorelease];

This can all be replaced with |return [[browserSet allObjects] sortedArrayUsingFunction:&CompareBundleIDAppDisplayNames: context:NULL]|

>+  // sort the list by display name


Same here.

>+// -createMenuItemForAppURL: withRepresentedObject: withAction: withTarget:


No spaces

>+// Builds a menu item for the passed in values.
>+//
>+-(NSMenuItem*)createMenuItemForAppURL:(NSURL*)inURL 
>+                withRepresentedObject:(id)inRepresentedObject 
>+                           withAction:(SEL)inAction
>+                           withTarget:(id)inTarget
>+{
>+  NSMenuItem* menuItem = [[NSMenuItem alloc] initWithTitle:[[NSWorkspace sharedWorkspace] displayNameForFile:inURL]
>+                                                    action:inAction 
>+                                             keyEquivalent:@""];

[[NSWorkspace sharedWorkspace] displayNameForFile:inURL] needs to be nil-checked before being passed in to initWithTitle:

>+-(BOOL)attemptDefaultFeedViewerRegistration:(NSString*)inBundleID
>+{
>+  NSBundle* defaultAppID = [NSBundle bundleWithIdentifier:[[NSWorkspace sharedWorkspace] defaultFeedViewerIdentifier]];

Get the ID here, not the bundle.

>+  if (![inBundleID isEqualToString:[defaultAppID bundleIdentifier]]) {

The default app's ID needs to be nil-checked.

>+  NSBundle* defaultAppBundle = [NSBundle bundleWithIdentifier:[[NSWorkspace sharedWorkspace] defaultBrowserIdentifier]];
...
>+  if (![inBundleID isEqualToString:[defaultAppBundle bundleIdentifier]]) {

>-@interface LockImageView : NSImageView
>+@interface ClickMenuImageView : NSImageView
>+{
>+  BOOL mIsFeedIcon;
>+}
>+- (void)setIsFeedIcon:(BOOL)inState;
...
> - (NSMenu *)menuForEvent:(NSEvent *)theEvent
> {
>+  // If we are the a feed icon, post this notification so the menu can get built.
>+  if (mIsFeedIcon) {
>+    [[NSNotificationCenter defaultCenter] postNotificationName:kBuildFeedMenuForURLBarNotification 
>+                                                        object:self];
>+  }
>   return [self menu];
> }


Make this generic; rather than having to know it's the feed icon, just have a setter for an optional notification to be sent before showing the menu, and set that to kBuildFeedMenuForURLBarNotification for the feed icon instance.

>+- (void)mouseDown:(NSEvent *)theEvent
>+{
>+  // By default a right-click will show the context menu for our image view subclass, 
>+  // but a left click will not show a context. Since we want to show the context menus
>+  // on a left click, foward this mouse down event as a right mouse down event.
>+  [super rightMouseDown:theEvent];
>+}

Wasn't this supposed to change to |self|?

>+- (void)displayFeedIcon:(BOOL)inDetected

How about inDisplay instead of inDetected, to be clearer

>+{
>+  if (inDetected && ![mFeedIcon superview]) {
...
>+  }
>+  else if (!inDetected && [mFeedIcon superview]) {
...
>+  }
>+  else if (inDetected && [mFeedIcon superview])

Please rearrange this to group the 'show' cases together (either in one block broken down by another if, or just reordered).

>+- (void)buildFeedsDetectedListMenu:(NSNotification*)notifier
>+{
>+  NSMenu* menu = [[NSMenu alloc] initWithTitle:@"FeedListMenu"];  // retained by the popup button

This is leaked

>+  NSArray* feeds = [[self getBrowserWrapper] feedList];
>+  NSEnumerator* feedsEnum = [feeds objectEnumerator];

Just do this in one line, since you never use |feeds|

>+  NSString* title = NSLocalizedString(@"SubscribeTo", nil);

Call this something like titleFormat, and use the comment param to give an example of what it would look like (so the later use is clear).

>+  NSDictionary* curFeedDict;
>+  while ((curFeedDict = [feedsEnum nextObject])) {
>+    NSString* feedTitle = [curFeedDict objectForKey:@"feedtitle"];
>+    NSString* feedURI = [curFeedDict objectForKey:@"feeduri"];
>+    NSString* itemTitle;
>+    
>+    if (feedTitle || [feedTitle length] > 0)

This should be &&, not ||

...
>+  [feedPrefItem setEnabled:YES];

Again, is this necessary?  If so, why is the feed pref item setEnabled, but not all the items for detected feeds?

>+-(IBAction)hitButtonCancel:(id)sender;            // called from mNotifyOpenExternalFeedApp
>+-(IBAction)hitButtonOpen:(id)sender;              // called from mNotifyOpenExternalFeedApp
>+-(IBAction)hitContinueButton:(id)sender;          // called from mNotifyNoFeedAppFound
>+-(IBAction)hitSelectButton:(id)sender;            // called from mNotifyNoFeedAppFound
>+-(IBAction)hitSelectMenuItem:(id)sender;          // called from mNotifyOpenExternalFeedApp


Name these by the action they take, not the action the user took (e.g., openFeed:, not hitButtonOpen:).

+-(void)runOpenDialog:(NSWindow*)containerWindow;
+-(void)openPanelDidEnd:(NSOpenPanel*)sheet returnCode:(int)returnCode contextInfo:(void *)contextInfo;


These methods appears to be cruft.  runOpenDialog: is never called, and openPanelDidEnd:... isn't implemented.

>+    // Assume that an application exists, check when building the popup menu
>+    mFeedAppsExist = YES;

Why?

>+// Populate the feed popup menu with registered feed applications XXX Update!


What's the |XXX Update|?

>+  // no feeds exist for the user, run the appropriate dialog to notify the user
>+  if (!mFeedAppsExist)
>+    [self showNotifyNoFeedAppFoundModal:inParent];
>+  else {
>+    // make sure the default feed viewer is selected
>+    [self buildFeedAppsPopUp];
>+    [self showNotifyOpenExternalFeedAppModal:inParent];
>+  }

Reverse the order of the clauses (to avoid the |if (!...|)

>+-(BOOL)feedAppsExist
>+{
>+  return mFeedAppsExist;
>+}

I can't find any case where mFeedApps is assigned after the initial "assume YES" in init.  Does it actually do anything?

>+typedef enum 
>+{
>+  eFeedType,
>+  eFavIconType,
>+  eOtherType
>+  
>+} ELinkAttributeType;

Remove the blank line

>   // make sure the load is for the main window
>   nsCOMPtr<nsIDOMDocument> domDoc;
>   linkElement->GetOwnerDocument (getter_AddRefs(domDoc));
> 
>   nsCOMPtr<nsIDOMDocumentView> docView(do_QueryInterface(domDoc));
>   NS_ENSURE_TRUE(docView, NS_ERROR_FAILURE);
> 
>   nsCOMPtr<nsIDOMAbstractView> abstractView;
>@@ -807,46 +810,151 @@
>   nsCOMPtr<nsIDOMWindow> topDomWin;
>   domWin->GetTop(getter_AddRefs(topDomWin));
> 
>   nsCOMPtr<nsISupports> domWinAsISupports(do_QueryInterface(domWin));
>   nsCOMPtr<nsISupports> topDomWinAsISupports(do_QueryInterface(topDomWin));
>   // prevent subframes from setting the favicon
>   if (domWinAsISupports != topDomWinAsISupports)
>     return NS_OK;

Shouldn't all the code after the domDoc be in HandleFaviconLink?  Presumably it's okay for subframes to provide feeds (since unlike the favicon there can be more than one feed)

>+  if (linkAttrType == eFavIconType)
>+    HandleFaviconLink(linkElement, domDoc);
>+  
>+  else if (linkAttrType == eFeedType)
>+    HandleFeedLink(linkElement, domDoc);


No blank lines before else's

>+nsresult
>+CHBrowserListener::GetLinkAttributeType(nsIDOMElement* inElement, ELinkAttributeType& inLinkAttrType)
>+{

Is there any reason this can't return its one interesting value, rather than passing it as an outparam and returning NS_OK every time?

>+nsresult
>+CHBrowserListener::HandleFaviconLink(nsIDOMElement* inElement, nsIDOMDocument* inDocument)
...

>+nsresult
>+CHBrowserListener::HandleFeedLink(nsIDOMElement* inElement, nsIDOMDocument* inDocument)


Again, these functions shouldn't return anything.

>+  [mContainer onFeedDetected:feedSpec feedTitle:titleSpec]; //XXX why the warning

Assuming the warning is a may not respond to selector:, it's because mContainer is an NSView that implements the CHBrowserListener and CHBrowserContainer protocols (not a BrowserWrapper).  You need to add the method to the CHBrowserListener protocol

>+  NSMutableSet* feedApps = [[[NSMutableSet alloc] init] autorelease]; 


Isn't this just [NSMutableSet set] ?

Comment 97

[Nick Kreeger]

Attachment: Patch 3

>+-(BOOL)attemptDefaultFeedViewerRegistration:(NSString*)inBundleID;
>+-(BOOL)attemptDefaultBrowserRegistration:(NSString*)inBundleID;

>It's not very clear from the name what the 'attempt' part of this is.  How
>about validateAndRegisterDefaultFoo:?  And since they present UI and don't
>actually have to do with building the menus, could they be put into
>FeedServiceController?

I don't think we should because then Navigation.mm may have to go through and init FeedServiceController and deal with overhead that we would run into there.

+  // sort the list by display name
+  NSMutableArray* browsers = [[browsersSet allObjects] mutableCopy];
+  [browsers sortUsingFunction:&CompareBundleIDAppDisplayNames context:NULL];
+  
+  return [browsers autorelease];

>This can all be replaced with |return [[browserSet allObjects]
>sortedArrayUsingFunction:&CompareBundleIDAppDisplayNames: context:NULL]|

This needs to stay the same because of a immutable object error. (You can only sort on a mutable array).

+    NSString *aName = [[NSWorkspace sharedWorkspace]
displayNameForFile:appURLa];
+    NSString *bName = [[NSWorkspace sharedWorkspace]
>Also, appURLa and appURLb are leaked here.

Actually |displayNameForFile:| already auto-releases those strings.

Comment 98

[froodian (Ian Leue)]

Comment on attachment 235318 [details] [diff] [review]
Patch 3

Targeting to the right smorgan.

Comment 99

[Stuart Morgan]

(In reply to comment #97)
> +  // sort the list by display name
> +  NSMutableArray* browsers = [[browsersSet allObjects] mutableCopy];
> +  [browsers sortUsingFunction:&CompareBundleIDAppDisplayNames context:NULL];
> +  
> +  return [browsers autorelease];
> 
> >This can all be replaced with |return [[browserSet allObjects]
> >sortedArrayUsingFunction:&CompareBundleIDAppDisplayNames: context:NULL]|
> 
> This needs to stay the same because of a immutable object error. (You can only
> sort on a mutable array).

sortedArrayUsingFunction doesn't sort the array, it returns a sorted copy of the array.  It's an NSArray function.

> +    NSString *aName = [[NSWorkspace sharedWorkspace]
> displayNameForFile:appURLa];
> +    NSString *bName = [[NSWorkspace sharedWorkspace]
> >Also, appURLa and appURLb are leaked here.
> 
> Actually |displayNameForFile:| already auto-releases those strings.

Not aName/bName, appURLa and appURLb. LSFindApplicationForInfo doesn't autorelease the URLs it gives you.

Comment 100

[Nick Kreeger]

Attachment: Patch 4

Addresses issues on comment 99.

Comment 101

[Stuart Morgan]

Comment on attachment 235359 [details] [diff] [review]
Patch 4

From code-reading, the only things I see left are small changes:

+-(NSMenuItem*)menuItemForAppURL:(NSURL*)inURL 
+                   withBundleID:(NSString*)inBundleID 
+                     withAction:(SEL)inAction
+                     withTarget:(id)inTarget;

Still needs to change to menuItemForAppURL:withBundleID:andAction:andTarget


+-(BOOL)validateAndRegisterDefaultFeedViewer:(NSString*)inBundleID
+{
+  NSBundle* defaultAppID = [NSBundle bundleWithIdentifier:[[NSWorkspace sharedWorkspace] defaultFeedViewerIdentifier]];
+  BOOL succeeded = YES;
+  
+  // if the user selected something other than the default application
+  if (![inBundleID isEqualToString:[defaultAppID bundleIdentifier]]) {

You don't use the bundle, just the ID. So it should be
  NSBundle* defaultAppID = [[NSWorkspace sharedWorkspace] defaultFeedViewerIdentifier];

Then nil-checked:
  if (defaultAppID && ![inBundleID isEqualToString:defaultAppID]) {


+  NSString* displayName = [[NSWorkspace sharedWorkspace] displayNameForFile:inURL];
+  
+  if (displayName) {
+    menuItem = [[NSMenuItem alloc] initWithTitle:[[NSWorkspace sharedWorkspace] displayNameForFile:inURL]

use displayName in the NSMenuItem creation, rather than re-fetching it through the NSWorkspace call.


+  if (mShouldSendMenuNotification) {
+    [[NSNotificationCenter defaultCenter] postNotificationName:kWillShowMenuForClickMenuImageView 
+                                                        object:self];
+  }

I was thinking an NSString member variable that, if set, would cause that notification to be sent.  That gives the option for different types of click images to send different notifications.


+  if (inDisplay) {
...
+  else if (!inDisplay && [mFeedIcon superview]) {

!inDisplay is redundant here.


There is also a little bit of respinning needed on a couple of bits of the patch due to the activate -> focusContent change.

I'd r+ with those changes except that I can't find any non-obsolete nibs, so I can't make a test build.  If you can post them, I'll promise to test them same-day so this can land.


If the plan is to land this on the branch too (which I assume is the case) there will need to be a branch spin of the patch and the nibs as well.

Comment 102

[Nick Kreeger]

Attachment: RSS patch v5

Here is the updated version, it is synced with a trunk tree from today.

I am posting a zip file containg the updated resources with directions.

Comment 103

[Nick Kreeger]

Here is the Localizable.strings change:

/* feed detection */
"FeedDetected" = "This page contains xml feed links";
"SubscribeTo" = "Subscribe to “%@”";
"FeedPreferences" = "Feed Preferences…";

Comment 104

[Nick Kreeger]

Attachment: Resources Needed

This contains the updated NIB files, Localizable.strings, feed icon, and notes/instructions.

Please hold off any polish requests until after this lands.

Comment 105

[Nick Kreeger]

Attachment: Patch v5

Oops, I forgot to fake CVS add the two new files.

Comment 106

[Stuart Morgan]

Comment on attachment 237237 [details] [diff] [review]
Patch v5

r=me; let's land this sucker.

I didn't throw any wacky edge cases at it, but we can shake those out later if necessary. For standard usage, it's great.

My only nit:

+  // If we are the a feed icon, post this notification so the menu can get built.
+  if (mMenuNotificationName) {
+    [[NSNotificationCenter defaultCenter] postNotificationName:mMenuNotificationName 
+                                                        object:self];
+  }

The comment needs to be update/removed, and the braces should go.

Comment 107

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 237237 [details] [diff] [review]
Patch v5

rs=pink. i can't believe there's anything i can add at this point...

land it!

Comment 108

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

Attachment: trunk project patch

Comment 109

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

Attachment: branch project patch

Branch project patch, ready for whenever the main patch gets its branch love

Comment 110

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

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

Comment 111

[Nick Kreeger]

Checked into trunk, branch version soon.

Comment 112

[[e-mail]]

(In reply to comment #111)
> Checked into trunk, branch version soon.
> 
I wonder when this "soon" is going to be? ;)
Cause it landed on the trunk some 12 hours ago, and I have been waiting all the time since then for it to land on the 1.8 branch.

Great work, and I can't wait to test it..

Comment 113

[Nick Kreeger]

(In reply to comment #112)
> (In reply to comment #111)
> > Checked into trunk, branch version soon.
> > 
> I wonder when this "soon" is going to be? ;)
> Cause it landed on the trunk some 12 hours ago, and I have been waiting all the
> time since then for it to land on the 1.8 branch.
> 
> Great work, and I can't wait to test it..
> 

sometime today.

Comment 114

[Nick Kreeger]

Checked into the MOZILLA_1_8_BRANCH.

/me pumps fist

Comment 115

[[e-mail]]

Verified fixed on trunk and ...  1.8 branch.

Thank you Nick!