Remove FeedView from Firefox 1.5

RESOLVED FIXED

Status

RESOLVED FIXED
14 years ago
2 months ago

People

(Reporter: bugs, Assigned: bugs)

Tracking

({fixed1.8})

unspecified
fixed1.8
Dependency tree / graph
Bug Flags:
blocking1.8b4 +
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

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. 
Flags: blocking1.8b4+

Comment 2

14 years ago
I kept a bit silent on the mail discussion, but I do support the back-out.
Created attachment 193185 [details] [diff] [review]
remove feedview

Remove FeedView, leaving the location bar icon. Enhance single-feed
multiple-format autodetect by automatically selecting ATOM feeds if present.
Attachment #193185 - Flags: review?
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. 
Depends on: 257247
You may want to consider bug 304727 when changing the findChildShell code.
Blocks: 304727

Comment 7

14 years ago
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.
Attachment #193185 - Flags: review? → review?(mconnor)

Updated

14 years ago
Flags: blocking1.8b5+
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.
Attachment #193185 - Flags: review?(mconnor) → review+
Comment on attachment 193185 [details] [diff] [review]
remove feedview

requesting approval
Attachment #193185 - Flags: superreview+
Attachment #193185 - Flags: approval1.8b4?
Attachment #193185 - Flags: superreview+
Note, please see bug 305799 for some small recommended changes to the newly
restored 1.0-like functionality.

Updated

14 years ago
Attachment #193185 - Flags: approval1.8b4? → approval1.8b4+
landed on the branch...
Keywords: fixed1.8

Comment 13

14 years ago
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

14 years ago
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. 
Created attachment 193869 [details] [diff] [review]
remove call to nonexistent uninit function
Attachment #193869 - Flags: approval1.8b4+
why do you want to remove it from trunk? Shouldn't we keep it for 2.0?

Comment 18

14 years ago
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. 
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
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.
Created attachment 194074 [details] [diff] [review]
additional patch [checked in]

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')
Attachment #194074 - Flags: review+
Comment on attachment 194074 [details] [diff] [review]
additional patch [checked in]

mozilla/browser/base/content/browser.js; new revision: 1.498;
Attachment #194074 - Attachment description: additional patch → additional patch [checked in]
Resetting QA Contact to default.
QA Contact: nobody → rss.preview

Updated

2 months ago
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.