305134 - Remove FeedView from Firefox 1.5

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

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.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

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

Comment 2

[Axel Hecht]

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

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: remove feedview

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

Comment 4

[Phil Ringnalda (:philor)]

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".

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

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. 

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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

Comment 7

[Axel Hecht]

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 8

[Ben Goodger (use ben at mozilla dot org for email)]

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 9

[Mike Connor [: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 10

[Ben Goodger (use ben at mozilla dot org for email)]

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

requesting approval

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

landed on the branch...

Comment 13

[Jim Ginn]

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

Comment 14

[Tim Meader]

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

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

Patch ready to land on the trunk. 

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: remove call to nonexistent uninit function

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

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

Comment 18

[Peter Hewitt]

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.

Comment 19

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch for trunk checkin

Comment 20

[Ben Goodger (use ben at mozilla dot org for email)]

Landed on trunk too. 

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

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.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Attachment: additional patch [checked in]

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

Comment 23

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 194074 [details] [diff] [review]
additional patch [checked in]

mozilla/browser/base/content/browser.js; new revision: 1.498;

Comment 25

[Tanner M. Young [:tmyoung]]

Resetting QA Contact to default.