Per discussion, due to remaining bugs with FeedView that require either a) nasty hacks or b) architectural changes to Gecko at a late point in the development cycle, we should remove FeedView from the 1.5 release and focus on better integrating it in the 2.0 cycle. Summary of issues leading to this decision (supported so far by cbeard, schrep, beltzner, jst and probably axel): The issues I have been looking at have included: - Some feeds, like the BBC, are not pretty printed, because they are styled with their own XSLT stylesheet. There are a couple of issues here: 1) that the BBC feed is not being pretty printed 2) that functionality such as Add Live Bookmark etc are not being exposed effectively for these feeds. 1 - BBC supplies its own stylesheet because they did not want users clicking on RSS links on their site and getting raw XML display - that's a bad user experience, and up until very very recently no browser has handled RSS internally. The argument for pretty printing the BBC document anyway is that they supply their stylesheet only to work around limitations of certain UAs, so since we are not limited we should treat theirs as a fallback. The counter argument is that they specified it, so it should win. This is a debate that needs to be explored by web platform, RSS and UE people. 2 - if we decide to force pretty print the BBC feed (as Safari does), or if we don't but we need to adjust the browser UI elsewhere to show "Add Live Bookmark" instead of "Add Bookmark", for example - we need the original, untransformed document so we can detect that the document is a feed, not just some random HTML document (which is what the BBC feed ends up looking like to the browser code once it comes out). I've talked to Axel about this one and he suggests maybe exposing some properties on DOMDocument that Microsoft offers - document.XMLDocument and document.XSLDocument - the document is the transformed dom, the XMLDocument is the original DOM, and the XSLDocument is the stylesheet document. Axel also believes though that the notion of registering a pretty print stylesheet for a particular doc type should be part of Gecko... I don't believe that the XSL we use should be part of Gecko since it makes references to Firefox specific things like live bookmarks, but the detection logic that selects feedview.xsl (or camino-rss.xsl, etc) should be. - HTML tags show up in the feed entries. The old code dealt with this I am going to imagine by stripping the contents of < > in all the article text. This might be enough, but some people were complaining that richer markup (tables, lists etc) weren't being properly handled. Images were special cased. The HTML that shows up in entries is often edited by hand by bloggers and as such cannot be relied upon to be well formed XML - thus these blocks are often contained in <![CDATA[ ]]> sections. The fix here is (may be?) to assume that the content of the entry/excerpt fields are quirks HTML (is there a way to detect otherwise?), and parse as fragments, constructing a DOM for the text content of an article and replacing the text content with it, then using <xsl:copy-of /> to copy the DOM nodes over to the target document. The problem is there's no easy way to parse a HTML fragment from a string, there are roundabout, hacky ways, but none I would feel comfortable submitting for code review. For example, typically one constructs a DOM for a fragment string like so: var range = document.createRange(); var fragment = range.createContextualFragment("some arbitrary tag soup"); rssEntryElement.replaceChild(rssEntryElement.firstChild, fragment); The problem is, for the tag soup to be parsed as quirks HTML, we need the range to be created in the context of a HTML document, and since quirks HTML has no namespace, we can't create one with document.implementation.createDocument(). Instead, we have to do something far hackier, e.g. var htmlDocument = Components.classesByID["{5d0fcdd0-4daa-11d2-b328-00805f8a3859}"]. createInstance(Components.interfaces.nsIDOMHTMLDocument); ... but before we can call createContextualFragment using a range established in this document, we need to set the document's content type to text/html. This is a problem - nsIDOMNSHTMLDocument provides only readonly access to the contentType field. The only way to set the contentType property of the newly created document is to call |open| on it and set the type. The problem then is that |open| fails miserably if the HTML document does not have a docshell associated with it (and it doesn't - it's just a hidden document that we're using to create a context within which to parse HTML). So we have to do something like this: try { htmlDocument.open("text/html", false); } catch (e) { } ...and basically rely on the fact that the |open| implementation sets its content type up before getting to the point where it fails and throws an exception. Brittle, hacky. Alternatively, one could create a hidden <iframe> and load a HTML document in it and use that as a context, but creating hidden <iframe>s when this is just something we should be able to expect to get from Gecko ("parse this string into a DOM from quirks HTML!") is a huge hack too. ... This bug covers backing out the FeedView feature, but retaining the RSS link in the location bar, functioning similarly to as it did with Firefox 1.0 but with better autodetection logic, so that it can be more discoverable.

Marking blocking1.8b4+ - we should be feature complete by beta.

I kept a bit silent on the mail discussion, but I do support the back-out.

Remove FeedView, leaving the location bar icon. Enhance single-feed multiple-format autodetect by automatically selecting ATOM feeds if present.

Atom isn't an acronym: Atom, not ATOM. And because of a couple of stalled bugs (bug 262222 and bug 302133) we are actually worse at handling correct Atom than we are at handling correct RSS. Would you consider (here or in a new bug) a different algo, that will probably produce fewer matches, but will certainly produce fewer false positives? If you strip digits and "Atom" and "RSS" from the link titles, and the results then match ("Entries in Atom 1.0" "Entries in RSS 2.0" == "Entries in" "Entries in") then you can safely conclude that they only differ in format; otherwise, you *will* hide at least one of "Entries in Atom 1.0" or "Linklog in RSS 1.0" or "Photostream in RSS 0.91".

I have updated my patch reverting it to selecting the first entry it finds. We can switch back to Atom once an analysis of the bugs has been performed and the worst issues corrected.

You may want to consider bug 304727 when changing the findChildShell code.

Ben, do you have anyone particular in your head to review attachement 193185? As that isn't set, it may just drop off if it's not on a particular review queue.

Comment on attachment 193185 [details] [diff] [review] remove feedview Doesn't particularly matter who reviews since its primarily a backout, but defaulting to mconnor.

Comment on attachment 193185 [details] [diff] [review] remove feedview Ugh, my ISP ate my homework. I haven't looked in-depth at every single change, but my two quick passes didn't turn up anything.

Comment on attachment 193185 [details] [diff] [review] remove feedview requesting approval

Note, please see bug 305799 for some small recommended changes to the newly restored 1.0-like functionality.

landed on the branch...

When closing a browser window I now get an error in the js console: Error: FeedHandler.uninit is not a function Source File: chrome://browser/content/browser.js Line: 907

Just wanted to highly recommend the FeedView extension for anyone who comes across this bug interested in a temporary replacement. Works great so far. https://addons.mozilla.org/extensions/moreinfo.php?application=firefox&id=445

Patch ready to land on the trunk.

why do you want to remove it from trunk? Shouldn't we keep it for 2.0?

with regards to the BBC - have we asked them how they want this done to their pages? Maybe they'd prefer their own stylesheet but we could use the yellow bar, a google image style top frame or something else to indicate it can be made a live bookmark.

Landed on trunk too.

Removing this from the trunk because per discussions the feature needs an actual design document, implementation plan etc - and that may look very different from the way the feature works now.

The trunk patch backed out a little too much, removing some of the patch for bug 304727.

Comment on attachment 194074 [details] [diff] [review] additional patch [checked in] r=me, but with change in param docs for 'doc' to not talk about "document that contains the feed" (instead something like 'the document to find a shell for')

Comment on attachment 194074 [details] [diff] [review] additional patch [checked in] mozilla/browser/base/content/browser.js; new revision: 1.498;

Resetting QA Contact to default.