Closed Bug 302121 (feedview) Opened 19 years ago Closed 19 years ago

Integrate Feedview for beta

Categories

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

Product:
Firefox Graveyard
Component:
RSS Discovery and Preview
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox1.5

People

(Reporter: mconnor, Assigned: myk)

References

Details

(Keywords: late-l10n)

Attachments

(4 files, 11 obsolete files)

feedview source
15.01 KB, application/x-zip
Details
checkmark indicating feed is read
243 bytes, image/png
Details
background image highlighting selected item
459 bytes, image/png
Details
implementation v7: global pref vars initialized to default values
47.78 KB, patch
mconnor
: review+
mconnor
: approval1.8b4+
Details | Diff | Splinter Review
We're going to integrate Feedview in time for beta. Tom Germeau has agreed to relicense to the MPL tri-license, and has provided an updated batch of code which I'll attach here.
Attached file feedview source —
Status: NEW → ASSIGNED
Flags: blocking1.8b4+
We're also going to make the following enhancements: - Make it easy to add the feed as a live bookmark. - Allow the author to see the old pretty-printed view. - We may want to add a pref for this.
feedview transforms feeds via an XSL stylesheet, and the XML pretty printer does the same thing, so my first thought is to patch the pretty printer to use feedview's XSL stylesheet instead of its own for feeds.
*** Bug 299354 has been marked as a duplicate of this bug. ***
Assignee: mconnor → myk
Status: ASSIGNED → NEW
Blocks: branching1.8
Here's a quick hack to demonstrate the concept. It modifies the pretty printer to run XML through the feedview XSLT stylesheet. It displays styled feed items, but it doesn't quite work right, since something prevents feedview JavaScript from accessing DOM properties.
You'll want to support Atom 1.0 (reasonably certain not to change significantly from http://www.ietf.org/internet-drafts/draft-ietf-atompub-format-10.txt) as well as 0.3, especially if you handle application/atom+xml instead of having it force a download. Main differences: namespace is http://www.w3.org/2005/Atom, the link you want is either rel="alternate" or no rel attribute (that existing rel!=service. is fairly broken even for 0.3, since there were non-service. values defined there, too), required date element is now atom:updated, and for bonus points, since atom:content can either contain or link to content, it would be better to only grab it for a description when it doesn't have a src attribute (<atom:content>foo</atom:content> you want, <atom:content src="/bar"/> you don't).
Here's a work in progress that uses the approach from comment 3: it modifies prettyprinter to use feedview's stylesheet to transform feed XML (it continues to use the default prettyprint stylesheet to transform other XML). Like prettyprint, this patch inserts the generated feed content into the document as anonymous nodes via an XBL binding to avoid altering the original XML documents in ways that break websites using them as datastores. One major problem with this approach is that security restrictions prevent XBL bound to content, even if the XBL itself is chrome, from accessing XPConnect, so we can't do things like get user preferences or even access the string bundle. cc:ing jst and bryner, who might know if there's any way around that. Otherwise, we might have to forgoe the binding to get this to work the way we want, which means potentially breaking sites.
Attachment #190518 - Attachment is obsolete: true
One of the two image files referenced in the CSS for the previously attached patch.
The other of the two image files referenced in the CSS for the previously attached patch.
Here's a second approach that foregoes the binding and calls nsXMLContentSink::LoadXSLStyleSheet to load the stylesheet. Unfortunately, even though chrome does the loading/linking of the stylesheet with the document, this fails with the error message: Security Error: Content at http://mozillazine.org/atom.xml may not load or link to chrome://global/content/xml/feedview.xsl. To summarize, I know of three ways to implement feedview in Firefox: 1. Watch loads in a load observer and transform feed documents with the stylesheet. The feedview extension currently does this. Besides the potential performance hit of watching all loads, this modifies each original document's DOM, which runs the risk of breaking sites that load RSS content as data. 2. Watch XML loads in the XML content sink and transform feed documents with the stylesheet. This is what this patch does. Besides suffering from the same drawback as the first approach (it modifies the original document, creating the potential for bustage), it doesn't work, as script generated/included by the stylesheet runs unprivileged. 3. Watch for pretty print requests and transform feed documents with the stylesheet, binding each document to an XBL widget that hides generated content in anonymous nodes. This approach is what the work in progress, attachment 190685 [details] [diff] [review], is about. It correctly displays the transformed document without changing its apparent DOM, but its script runs unprivileged, so it can't access necessary preferences, string bundles, etc. If worse comes to worst, I can put together a patch that implements the first method along with some logic for avoiding as much data XML as possible. But the third approach is the most promising if it can be made to work, and the second approach is also probably better than the first. Thoughts?
cc:ing some folks who might be able to help with the security issues. See comment 7 and comment 10 for more details.
Can we do #1, but only transform documents that are being loaded into a <browser>? (That is, where some browser's contentDocument is the document being loaded?) I'm not sure where the right place would be too hook that, though.
(In reply to comment #12) > Can we do #1, but only transform documents that are being loaded into a > <browser>? (That is, where some browser's contentDocument is the document being > loaded?) I'm not sure where the right place would be too hook that, though. Good question. cc:ing Jonas Sicking, prettyprinter author, for his insight into whether this would address the issue of sites loading data XML that shouldn't be transformed.
How does #1 solve the security problem? I'd think it's possible to check that the current document is loaded into a <browser>. There are two downsides with this: 1. It's sort of neat that the xml-prettyprinter works in iframes. I'd think this is even more true for RSS documents. Especially since feedview is intended for normal users. (xml-prettyprint is only interesting to developers) 2. It is in theory possible that someone loads an RSS document for data-mining even inside a <browser> (though document.open or such). Though this is probably pretty unlikly. All in all though it sounds like an ok solution to me. As long as we can ensure that we only catch RSS and Atom documents and not, for example, all rdf documents. An alternative solution to using xslt could be to normal XBL bindings. We'd just detect when a loaded document is an xslt document and then attach a CSS stylesheet that attaches bindings to all needed elements. But I don't know enough about the RSS/Atom formats to know if this is possible. I couldn't do this for xml-prettyprint since I wanted to style comments and PIs.
Hmm.. one thing though. A <xul:browser> doesn't neccessarily need to be the one in firefox. Remote content can use that element too instead of an iframe.
So I've been thinking about this for the last few hours, and I don't know if we gain enough from being able to have a "make this a Live Bookmark" button in the page (I think if we can pref this off so users get prettyprint, that's adequate to what is an edge case for end-users). Feedview's implementation gives an option to create a Live Bookmark in the standard way, we could reference this in the transformed page text, that might be sufficient at this stage. Add to Bookmarks could be tweaked to give the choice if we're viewing a feed, and between those two things I think we're probably good. I'd rather get this in, and look at improvements after, instead of trying for the ideal implementation for something that's happening at the last minute.
Ok, here's an implementation that takes the first approach. It's actually just the current feedview extension relicensed, reorganized into a mozilla/extensions/feedview directory, integrated into the build system (builds by default--I'll attach the patch for build changes shortly), and with the one fix discussed above: it only transforms the feed file if the file is loaded into one of the tabbrowser's browser widgets. This is the minimal work required to integrate the extension. mconnor, is this what you're looking for? There's a lot more to do for this to be ready for beta, IMHO, but perhaps that's better done in the tree than in my personal working copy.
Attachment #190790 - Flags: review?(mconnor)
Keywords: late-l10n
Alias: feedview
mconnor says this should be integrated into the browser chrome, not an extension, so I've moved content, locale, and skin files to browser/components, browser/locales, and browser/themes/, respectively. I want to make sure this is all correct before actually creating the directories, so this is a "diff -urN browser-orig browser" instead of "cvs diff -uN browser". It should apply the same, though, from inside mozilla/: patch -p0 < patch.diff
Attachment #190685 - Attachment is obsolete: true
Attachment #190690 - Attachment is obsolete: true
Attachment #190790 - Attachment is obsolete: true
Attachment #190791 - Attachment is obsolete: true
Indent with spaces, not tabs? CVS is going to love it if you fix that first. + /* + XXX: Checking for XMLDocument is NOT enough. Some feeds are served as text/plain or + some unsupported format like application/xml+rss + */ Silly. We're not going to default to mimetype sniffing or default to xml docs for feedview. If we need that comment, it should read more something like: /* * Sadly we can't catch bad mime types from servers, long live evangelism. */ Stylesheet: license block right after the xml pi. More use of xsl attribute value templates. + <link rel="stylesheet"> + <xsl:attribute name="href"> + <xsl:value-of select="$externalCSS"/> + </xsl:attribute> + </link> vs. <link rel="stylesheet" href="{$externalCSS}" /> Sadly I don't know any way to work around the "*[local-name()='pubDate']" stuff, just to let you know, that's slow. Remove OPML Support into a separate patch until it is ready? locale/browser/feedview.properties is missing from the patch.
Talking about license plates, you should get someone to verify that we accept websites as contributor address instead of emails. And you should change the paranthesis to angle brackets, so that we don't have to adjust all our license header regexps to catch yet another style. (() -> <>, just to be clear about names.)
you can replace it with tom.germeau@epigoon.com if a url is not allowed
Attached patch implementation v2: code cleanup (obsolete) — — Splinter Review
(In reply to comment #20) > Indent with spaces, not tabs? CVS is going to love it if you fix that first. Indeed. Done. > + /* > + XXX: Checking for XMLDocument is NOT enough. Some feeds are served as > text/plain or > + some unsupported format like application/xml+rss > + */ > > Silly. We're not going to default to mimetype sniffing or default to xml docs > for feedview. Sure, for generic XML content types, but it's worth sniffing for known feed types like application/rss+xml and application/atom+xml. I removed this comment and instead document the function thusly: // Transforms feed files, which we define as XML documents with one or more // of the following attributes: // 1. a known feed-specific content type like application/(rss|atom)+xml // (XXX not doing this yet, but should be) // 2. a feed-specific default namespace // 3. a top-level feed tag like <rss> or <feed> > Stylesheet: license block right after the xml pi. Ok, done. > More use of xsl attribute value templates. > + <link rel="stylesheet"> > + <xsl:attribute name="href"> > + <xsl:value-of select="$externalCSS"/> > + </xsl:attribute> > + </link> > vs. > <link rel="stylesheet" href="{$externalCSS}" /> Agreed. The latter seems much more readable. But I'll leave this for a subsequent cleanup. > Remove OPML Support into a separate patch until it is ready? Sounds good. > locale/browser/feedview.properties is missing from the patch. Ok, added. I also: * fixed numerous style nits and made style consistent within and across files; * eliminated exception alerts and try/catch blocks whose only purpose was to throw such alerts; * eliminated a conditional block spanning an entire function.
Attachment #190917 - Attachment is obsolete: true
I've tried the feedview that's available as a Firefox extension and I ran into a problem. Viewing roc's blog: http://weblogs.mozillazine.org/roc/atom.xml seems problematic. Increase the article length to the max, and notice that the list (<ul>) is not rendered properly in the first post (Gecko 1.9). I can create a new bug for this if you want to.
> Viewing roc's blog: http://weblogs.mozillazine.org/roc/atom.xml seems > problematic... I can create a new bug for this if you want to. Sounds like a good idea. I'm sure we'll run into many problems with various feeds that we'll want to correct; we should focus this initial work on getting a version into the tree that is organized appropriately and conforms with general coding practices.
The code currently relies on default namespace checks which is a very bad thing to do (and case insensitive checks at that). This is especially important since we modify the DOM so we have to be very restrictive about when that is done. I still don't understand how this approach deals with the security problems mentioned before, but I don't really know what the problems were. Looking at the js-code for feedview it looks like it would be fairly easy to make that work within an xbl binding and use a pretty-print approach, feel free to poke me if you need help with this.
(In reply to comment #26) > The code currently relies on default namespace checks which is a very bad thing > to do (and case insensitive checks at that). Actually, the default namespace checks aren't entirely unreasonable. In the case of Atom it's dead wrong: anything with a document element of either {http://purl.org/atom/ns#}:feed or {http://www.w3.org/2005/Atom}:feed is an Atom feed, anything without is not, no matter what the default namespace, but for RSS 0.90 and RSS 1.0, anything with a document element of {http://www.w3.org/1999/02/22-rdf-syntax-ns#}:RDF and a child of either {http://my.netscape.com/rdf/simple/0.9/}:item or {http://purl.org/rss/1.0/}:item is no more than 'quite likely' to be a feed, not something else reusing an rss:item. However, both specs require that RSS be the default namespace, and anything reusing their elements is quite unlikely to use them in the default namespace, so for them the default namespace check is a bit less likely to give false positives, while only giving false negatives on invalid feeds. The alternative would be, for every RDF file, iterate through every child node looking for enough RSS elements to make it seem likely that it's a feed (a rss:channel and at least one rss:item would probably do).
Comment on attachment 190950 [details] [diff] [review] implementation v2: code cleanup Mike, any further issues before I make a round of changes based on comments?
Attachment #190950 - Flags: review?(mconnor)
Attachment #190790 - Flags: review?(mconnor)
Attachment #190791 - Flags: review?(mconnor)
Depends on: 302749
Comment on attachment 190950 [details] [diff] [review] implementation v2: code cleanup Removing review request in preparation for attaching newer version of patch.
Attachment #190950 - Flags: review?(mconnor)
Comment on attachment 190950 [details] [diff] [review] implementation v2: code cleanup Removing review request in preparation for attaching newer version of patch.
mconnor said: > I'd like to see spaces around operators, after /* and before */, > 2-space vs. 4-space (to fit with other code in browser that isn't dated > to seamonkey) and a removal of all of the cruft, the 2-space would be > good now, everything else is whenever I guess > > removing all of the checklist/testing/non-working code is probably good too > and the stuff in feedviewOverlay.js totally needs to be in an initFeedview() > call we can hit from browser.js > > myk, other than that, and the changes from the other comments, > I'd rather just land this and get people poking at it, including > our security people Ok, I changed to 2-space indent everywhere, changed "/*foo*/" to "// foo", removed almost all the cruft (except a rare commented-out line that looked like it might be informative during future development). I also added "feedview" to the names of all variables and functions in feedviewOverlay.js to avoid naming conflicts with current and future code. I created initFeedview() and call it from browser.js::delayedStartup(). It initializes the global feedview variables (the stylesheet, the processor) and registers the load listener. I rewrote the code that determines which documents are feeds per Phil's comments, except that I check for a "channel" child of RSS 1.0 and 0.90 feeds, since items can in theory be wholly contained within the channel tag of RSS 1.0 feeds (and thus not children of the root element). I changed the prefs location from extensions.feedview to layout.xml.feedview (corresponds to layout.xml.prettyprint). Finally, I fixed a bunch of strict JavaScript warnings in feedview.js by declaring variables before using them, and I fixed a CSS syntax error by removing a line incorrectly commented out with //. Asa's going to take a look at the CSS tomorrow with an eye towards improving the design, but I think we can check in those changes (like many others we are likely to make in the near future) separately. Mike, does this look good?
Attachment #190950 - Attachment is obsolete: true
Attachment #191039 - Flags: review?(mconnor)
Comment on attachment 191039 [details] [diff] [review] implementation v3: lots more code cleanup + * The Initial Developer of the Original Code is + * Tom Germeau (www.epigoon.com). Should be: + * Tom Germeau <tom.germeau@epigoon.com> As Axel Hecht suggested previously. You'll need to change that everywhere.
Comment on attachment 191039 [details] [diff] [review] implementation v3: lots more code cleanup >+function feedviewIsFeed(doc) { Another thing that would be very good to check for in this function is check doc.ownerDocument.styleSheets.length and ensure that that is 0. We do that for xml prettyprint to ensure that if the author has provided stylesheets to display the document we use those rather then our own view. >+ var channel = doc.getElementsByTagName('channel')[0]; >+ var channelNS = channel ? channel.namespaceURI : null; Actually, this does not just check children but checks all descendents at any level. Also, this won't work if channel has a prefix. Either you could do manually walk all children and check .localName and .namespaceURI, or you could do something like: channel = doc.getElementsByTagNameNS(RSS_09_NS, 'channel')[0]; if (!channel) channel = doc.getElementsByTagNameNS(RSS_10_NS, 'channel')[0]; if (channel && channel.parentNode != doc) channel = null and then below do else if (rootName == "RDF" && rootNS == RDF_NS && channel) return true; Though this will break if a channel-element appears anywhere else in the document rather then just below the documentelement. It could also potentially be pretty slow. Hmm.. come to think of it, you could write the code as: else if (rootName == "RDF" && rootNS == RDF_NS) { ... check for channel in here } To avoid doing any DOM walking unless needed. >+ // Atom feeds have a root "feed" element in one of two namespaces. >+ if (rootName == "feed" && (rootNS == ATOM_10_NS || rootNS == ATOM_03_NS)) >+ return true; >+ >+ // RSS 2.0, 0.92, and 0.91 feeds have a non-namespaced root "rss" element. >+ else if (rootName == "rss") >+ return true; Add |&& rootNS == null| here. Though I'm worried that we'll mangle xml documents that just happen to use 'rss' as the root name. Can't we do any additional syntax checks here? >+ // If it didn't match any criteria yet, it's probably not a feed, >+ // or perhaps it's a nonconformist feed. If you see a number of those >+ // and they match some pattern, add a check for that pattern here. >+ else >+ return false; It'd probably be good to add something about that we want to be very restrictive about which documents we return true for since we don't want to mangle ordinary xml documents. And you can remove the 'else'.
Comment on attachment 191039 [details] [diff] [review] implementation v3: lots more code cleanup >diff -urN --exclude=CVS --exclude=Makefile browser-orig/base/content/browser-sets.inc browser/base/content/browser-sets.inc >--- browser-orig/base/content/browser-sets.inc 2005-07-28 19:42:01.000000000 -0700 >+++ browser/base/content/browser-sets.inc 2005-07-28 13:55:58.000000000 -0700 >@@ -48,6 +48,7 @@ > <stringbundle id="bundle_shell" src="chrome://browser/locale/shellservice.properties"/> > <stringbundle id="bundle_findBar" src="chrome://global/locale/findbar.properties"/> > <stringbundle id="bundle_preferences" src="chrome://browser/locale/preferences/preferences.properties"/> >+ <stringbundle id="feedViewString" src="chrome://browser/locale/feedview.properties"/> this should probably be in browser.xul, otherwise it gets added everywhere macBrowserOverlay.xul gets overlaid, when we only want/need it for browser.xul >+++ browser/base/content/global-scripts.inc 2005-07-28 14:24:59.000000000 -0700 >@@ -47,3 +47,4 @@ > <script type="application/x-javascript" src="chrome://global/content/viewZoomOverlay.js"/> > <script type="application/x-javascript" src="chrome://browser/content/browser.js"/> > <script type="application/x-javascript" src="chrome://browser/content/sanitize.js"/> >+<script type="application/x-javascript" src="chrome://browser/content/feedview/feedviewOverlay.js"/> Same thing here, less js to parse on non-main-window cases. >+// Add the name of the feed to the title. >+function setFeed() >+{ >+ var title = document.getElementsByTagName("h1")[0].textContent; >+ document.title = document.getElementsByTagName("title")[0].textContent + " " + title; >+} since I'm imposing browser/toolkit style, function foo() { please. There's also a bunch of two or three line gaps in this file. >+ // If none of the feed items has a date, remove the "Hide Date" link >+ // and hide the date span. >+ if (!found) { >+ document.getElementById("switchdate").style.display = "none"; >+ >+ divs = document.getElementsByTagName("div"); >+ for (i = 0; i < divs.length; i++) >+ if (divs[i].getElementsByTagName("span").length > 0 ) >+ if (divs[i].getElementsByTagName("span")[0].getAttribute("class") == "date") >+ divs[i].getElementsByTagName("span")[0].style.display = "none"; >+ } >+} you mentioned this on IRC, let's kill the show/hide date stuff entirely, and just always show the date. Alternate stylesheets can show or not show the date. >+// XXX This function should not exist. >+// It tries to fix as many special characters as posible. >+function fixchars(txt) >+{ >+ txt = txt.replace(/&nbsp;/g, " "); >+ txt = txt.replace(/&amp;/g, "&"); >+ txt = txt.replace(/&gt;/g, ">"); >+ txt = txt.replace(/&lt;/g, "<"); >+ txt = txt.replace(/&quot;/g, "'"); >+ txt = txt.replace(/&#8217;/g, "'"); >+ txt = txt.replace(/&#8216;/g, "'"); >+ txt = txt.replace(/&#8212;/g, "—"); >+ txt = txt.replace(/&#33;/g, "!"); >+ txt = txt.replace(/&#38;/g, "&"); >+ txt = txt.replace(/&#39;/g, "'"); >+ return txt; >+} I think we had a much better solution for this for live bookmarks, but that can be a followup. >+// Show hide the date, used in the XSL/HTML. >+function switchdate() yeah, kill this >+var gFeedviewPrefs; >+var gFeedviewProcessor; >+var gFeedviewStylesheet; >+ >+function initFeedview() >+{ >+ gFeedviewPrefs = Components.classes["@mozilla.org/preferences-service;1"] >+ .getService(Components.interfaces.nsIPrefService) >+ .getBranch("layout.xml.feedview."); if we're moving the feedview impl into layout for 2.0, ok, but otherwise this should be browser.feedview or somesuch. Don't forget this can throw for various reasons, which could break startup. >+ // Import all settings and set default values if they have bad values. >+ // XXX: This should be coded nicer >+ >+ try{ articleSize = gFeedviewPrefs.getIntPref("articleSize"); } catch(ex) { articleSize = 50; } >+ gFeedviewProcessor.setParameter(null, "articleSize", articleSize); >+ >+ try{ showDate = gFeedviewPrefs.getIntPref("showDate"); } catch(ex) { showDate = 1; } >+ gFeedviewProcessor.setParameter(null, "showDate", showDate); >+ >+ try{ showBar = gFeedviewPrefs.getBoolPref("showBar"); } catch(ex) { showBar = true; } >+ gFeedviewProcessor.setParameter(null, "showBar", showBar); >+ >+ try{ showImage = gFeedviewPrefs.getBoolPref("showImage"); } catch(ex) { showImage = true; } >+ gFeedviewProcessor.setParameter(null, "showImage", showImage); >+ >+ try{ timerInterval = gFeedviewPrefs.getIntPref("timerInterval"); } catch(ex) { timerInterval = 0; } >+ gFeedviewProcessor.setParameter(null, "timerInterval", timerInterval); >+ >+ try{ externalCSS = gFeedviewPrefs.getCharPref("externalCSS"); } catch(ex) { externalCSS = ""; } >+ gFeedviewProcessor.setParameter(null, "externalCSS", externalCSS ); so, checking six prefs in a try/catch every time we're parsing a feed seems wasteful. These are all hidden prefs, we should init these on startup and persist the values. observeFeedviewSettings can then compare against the persisted values, and change those and update the pref if there's a change, instead of always setting the pref. The only pref that isn't UI-accessible and should probably be more live is the externalCSS bit, which we can use a pref observer to update the persisted values. Are these prefs actually set by default somewhere, or do we start off hitting the fallback? >+ // Get the length and give it to the description thingie. >+ var len = dataXML.getElementsByTagName("item").length; >+ if (len == 0) len = dataXML.getElementsByTagName("entry").length; if (foo) bar; r=me with the changes here, and the previous comments (from Jonas and Caleb) as well.
Attachment #191039 - Flags: review?(mconnor) → review+
(In reply to comment #31) <...> > I changed the prefs location from extensions.feedview to layout.xml.feedview > (corresponds to layout.xml.prettyprint). Actually, layout.xml.prettyprint was following other layout.xml stuff back then. I'd advocate dom. or toolkit. for platform prefs, and browser for firefox application ones. Things we reuse for l10n, too.
Attached patch implementation v5: review fixes (obsolete) — — Splinter Review
> + * The Initial Developer of the Original Code is > + * Tom Germeau (www.epigoon.com). > > Should be: > + * Tom Germeau <tom.germeau@epigoon.com> Ok, done. > Another thing that would be very good to check for in this function is check > doc.ownerDocument.styleSheets.length and ensure that that is 0. We do that for > xml prettyprint to ensure that if the author has provided stylesheets to > display the document we use those rather then our own view. Author stylesheets seem generally designed for users who accidentally click on a feed link in a browser that doesn't understand feed XML. As far as I'm aware, feed readers universally ignore them in favor of displaying stories via their own custom chrome (often aggregating feeds from multiple sources into an composed view). Firefox has previously been "a browser that doesn't understand feed XML," (except for live bookmarks, of course) but with the addition of feedview, it's now a feed reader, and I strongly suspect that as we build out this functionality, we'll do so in ways that make feedview even more a feed reader. Ultimately, the question isn't what other feed readers do or what feed authors want, however, but rather what's best for our users. Intuitively, I'd say that's to display the main structure of a feed (the feed title, each story box, and story metadata like title, datestamp, etc.) as chrome styled by feedview while displaying story content according to the author's stylesheet. To do that, though, we need to stop transforming feed XML into HTML, transforming it instead to XUL or some other XML presentation format. And we need to display the HTML in story descriptions/content instead of converting it to text. But those are bigger changes than we can make now. > Hmm.. come to think of it, you could write the code as: > > else if (rootName == "RDF" && rootNS == RDF_NS) { > ... check for channel in here > } > > To avoid doing any DOM walking unless needed. Ok, done. > >+ // Atom feeds have a root "feed" element in one of two namespaces. > >+ if (rootName == "feed" && (rootNS == ATOM_10_NS || rootNS == ATOM_03_NS)) > >+ return true; > >+ > >+ // RSS 2.0, 0.92, and 0.91 feeds have a non-namespaced root "rss" element. > >+ else if (rootName == "rss") > >+ return true; > > Add |&& rootNS == null| here. Though I'm worried that we'll mangle xml > documents that just happen to use 'rss' as the root name. Can't we do any > additional syntax checks here? We could. I've added the "root element has channel child" check here. But note that there's an opportunity cost to being strict, and it's worth the pain of a few false positives to reduce false negatives significantly. > It'd probably be good to add something about that we want to be very > restrictive about which documents we return true for since we don't want to > mangle ordinary xml documents. I have added: // making sure to specify the strictest check that matches that pattern // to minimize false positives. > And you can remove the 'else'. Done. > this should probably be in browser.xul, otherwise it gets added everywhere > macBrowserOverlay.xul gets overlaid, when we only want/need it for browser.xul Ok, done. Something should be done about this comment in browser.xul, though, given that it is apparently obsolete: # All sets except for popupsets (commands, keys, stringbundles and broadcasters) *must* go into the # browser-sets.inc file for sharing with hiddenWindow.xul. > since I'm imposing browser/toolkit style, function foo() { please. Cool, done. > There's also a bunch of two or three line gaps in this file. 2|3 -> 1 > you mentioned this on IRC, let's kill the show/hide date stuff entirely, and > just always show the date. Alternate stylesheets can show or not show the > date. Done. > I think we had a much better solution for this for live bookmarks, but that > can be a followup. Indeed. Incidentally, there'll be a lot of followups to this, f.e. better date parsing (which I have a solution for). > >+// Show hide the date, used in the XSL/HTML. > >+function switchdate() > > yeah, kill this Yup. > if we're moving the feedview impl into layout for 2.0, ok, but otherwise this > should be browser.feedview or somesuch. Don't forget this can throw for > various reasons, which could break startup. It's unclear what we're doing with it, so I've made it browser.feedview for now. > so, checking six prefs in a try/catch every time we're parsing a feed seems > wasteful. These are all hidden prefs, we should init these on startup and > persist the values. observeFeedviewSettings can then compare against the > persisted values, and change those and update the pref if there's a change, > instead of always setting the pref. Ok, done. I also changed the way we listen for changes to the article size pref, as the previous code was flaky in my testing. > The only pref that isn't UI-accessible and should probably be more live is the > externalCSS bit, which we can use a pref observer to update the persisted > values. Actually only article size is UI-accessible, and I'm not sure the others should be (or even exist for that matter, f.e. show bar). Let's address this in a followup. > Are these prefs actually set by default somewhere, or do we start off hitting > the fallback? They are now (in firefox.js). > >+ // Get the length and give it to the description thingie. > >+ var len = dataXML.getElementsByTagName("item").length; > >+ if (len == 0) len = dataXML.getElementsByTagName("entry").length; > > if (foo) > bar; Done.
Attachment #191039 - Attachment is obsolete: true
Attachment #191119 - Flags: review?(mconnor)
Jonas and I are wondering if xml prettyprinting actually kicks in intermediatly in this approach. Myk, could you test that? From looking at the code, xml pp kicks in before onload, it may tear itself down later again. Or something like that. It'd sure be good to know what we're really dealing with. Re the parsing of the entry content, would using innerHTML be a security issue?
(In reply to comment #37) > Jonas and I are wondering if xml prettyprinting actually kicks in > intermediatly in this approach. Myk, could you test that? Yes, tested. It does. That seems suboptimal performance-wise, although it doesn't seem to affect the user experience (except possibly on a slow machine). I'm not sure what the solution is. > Re the parsing of the entry content, would using innerHTML be a security issue? I'm not sure, but I don't see any use of innerHTML in the extension. Or are you thinking we should use that method? In any case, we should definitely have some of the security experts look this over for security issues. Friday shaver said on IRC that any l10n changes need to happen by today (Sunday). I'm not sure if that's still the case. cc:ing him for confirmation. If so, we should get this in today, so if there's anything else that must be taken care of before it can go in, let me know.
Attached patch patch-patch for Atom 1.0 (obsolete) — — Splinter Review
Wups, if we identify Atom 1.0 as a feed, we should probably do something with it, too.
> I still don't understand how this approach deals with the security problems > mentioned before, but I don't really know what the problems were. > > Looking at the js-code for feedview it looks like it would be fairly easy to > make that work within an xbl binding and use a pretty-print approach, feel > free to poke me if you need help with this. The problem is that script in XBL bound to content doesn't have chrome privileges. This means, among other things, that when the user drags the slider to change how much of each story is being shown, we can't save the change back to the relevent preference, because we don't have access to preferences. It also means we can't obtain the stringbundle, so we can't display "This feed has %S posts." in the UI. As far as I (and dveditz, whom I talked to about it) know, there's no solution to this problem at the moment. Ultimately the prettyprint approach makes more sense to me, since it doesn't disturb the apparent DOM and more cleanly separates chrome from content. But we need the security model to change first, and although folks have been talking about doing just that, it's not going to happen before 1.5, so if we want this to work for 1.5 we have to do it the way we're doing it now.
Comment on attachment 191119 [details] [diff] [review] implementation v5: review fixes The current solution looks fine to me. We can always aim for some XBL solution later. WRT prettyprint firing, it would be good to avoid, but it's something that can wait. It would probably require hooking in somewhere before prettyprint has a chance to fire and maybe even modifying some API to allow js code to tell the xml sink to back off from prettyprinting. As I recall it isn't possible to fire prettyprint later due to bugs in XBL code. I think you're right on the styleSheets.length issue. If we know that the document is a feed then our view should have higher priority then the authors. >+ else if (rootName == "rss" && rootNS == null) { >+ channel = doc.getElementsByTagName('channel')[0]; This should be getElementsByTagNameNS('', 'channel') just to be on the safe side. I'm still a little bit worried that we'll get false positives here and screw up an authors xml doc. How would you feel about checking styleSheets.length just for this case? Or could that break a lot of feeds that contain a fallback css?
Thanks Phil! Here's the patch with your changes integrated. I also made some updates to variable/function/id/label names, primarily: * size -> length * post -> article * news feed -> feed And I removed more cruft from the stylesheets.
Attachment #191119 - Attachment is obsolete: true
Attachment #191140 - Attachment is obsolete: true
Attachment #191148 - Flags: review?(mconnor)
> This should be getElementsByTagNameNS('', 'channel') just to be on the safe > side. Ok, I can roll this into the next iteration of the patch (or fix it in a followup if the current patch gets r+). Incidentally, what's the difference? > I'm still a little bit worried that we'll get false positives here and screw > up an authors xml doc. How would you feel about checking styleSheets.length > just for this case? Or could that break a lot of feeds that contain > a fallback css? I think it'll break a lot more than it fixes, and if there are any docs out there with a non-namespaced "rss" element containing a non-namespaced "channel" element, but which are not RSS feeds, it's not clear that the presence of a stylesheet is the best way to differentiate them from feeds. I think the best approach is to keep an eye out for any instances of bustage and derive the necessary distinguishing patterns from the ones we find (if any).
Incidentally, we should use this routine (factoring it out, ideally) for date string parsing: http://lxr.mozilla.org/mozilla/source/mail/extensions/newsblog/content/FeedItem.js#470
Attachment #191119 - Flags: review?(mconnor)
Comment on attachment 191148 [details] [diff] [review] implementation v6: Phil's Atom 1.0 fixes + canonicalize names Note: this patch accidentally includes feedview.xsl.orig, the version of that file before I applied Phil's patch, but of course I'll not check that in.
Comment on attachment 191119 [details] [diff] [review] implementation v5: review fixes >+function initFeedview() { >+ try { >+ gFeedviewPrefs = Components.classes["@mozilla.org/preferences-service;1"] >+ .getService(Components.interfaces.nsIPrefService) >+ .getBranch("browser.feedview."); >+ >+ gFeedviewPrefArticleSize = gFeedviewPrefs.getIntPref("articleSize"); >+ gFeedviewPrefShowBar = gFeedviewPrefs.getBoolPref("showBar"); >+ gFeedviewPrefShowImage = gFeedviewPrefs.getBoolPref("showImage"); >+ gFeedviewPrefTimerInterval = gFeedviewPrefs.getIntPref("timerInterval"); >+ gFeedviewPrefExternalCSS = gFeedviewPrefs.getCharPref("externalCSS"); >+ } >+ catch (ex) {} We need to initialize these vars to something so if something throws we don't break. With that fixed, we should be good to go. If you want to include the Atom 1.0 fixes in Phil's patch, please go ahead (we'll get bugs anyway, let's just get this in and start fixing followup issues).
Attachment #191119 - Attachment is obsolete: false
Attachment #191119 - Flags: review+
Attachment #191119 - Flags: approval1.8b4+
Attachment #191148 - Flags: review?(mconnor)
This version initializes the variables to the default values so if something throws they're still set.
Attachment #191119 - Attachment is obsolete: true
Attachment #191148 - Attachment is obsolete: true
Attachment #191156 - Flags: review?(mconnor)
Attachment #191156 - Flags: review?(mconnor)
Attachment #191156 - Flags: review+
Attachment #191156 - Flags: approval1.8b4+
getElementsByTagName('channel') will select all the elements named 'channel' (and with no prefix) no matter which namespace they are in. getElementsByTagNameNS('', 'channel') will select all elements with localName channel and null namespace (no matter which prefix they have). The reason checking styleSheets.length is an ok way to differentiate is that if the document doesn't have any stylesheets we would just display the prettyprint anyway, which wouldn't be a big loss to mangle it with the rss view. Btw, one way to add additional checks for feeds would be to let the xslt stylesheet do some sanity checks as it performs the transformation and use an <xsl:message terminate="yes"> if it sees that the document isn't a valid feed. That makes transformToFragment throw which would be easy to catch. Something to consider for later...
From an l10n as well as permissions point of view I'd prefer the use of a DTD for the localized strings. <!ENTITY title "Feed: "> // LOCALIZATION NOTE: Do not translate {$length} <!ENTITY description "This feed contains {$length} articles."> <!ENTITY lengthSliderLabel "Article length: "> That way, we only have to worry about the pref, when it comes down to xpconnect access.
Yikes, this patch just caused a major mlk on Linux balsa: From: RLk:1.08KB Lk:333KB MH:7.82MB A:219K To: RLk:49.8KB Lk:1.30MB MH:7.96MB A:226K --NEW-LEAKS-----------------------------------leaks------leaks%----------------------- nsDocument 936 - nsXPCWrappedJSClass 88 - HTMLCSSStyleSheetImpl 64 - nsGenericDOMDataNode 11760 - nsBarProp 32 - nsJAR 7032 - nsJSContext 240 - nsGenericFactory 20 - nsZipArchive 6360 - nsPrincipal 360 - nsScreenGtk 48 - nsOnloadBlocker 24 - nsJSRuntimeServiceImpl 28 - nsNodeInfo 2432 - nsHTMLStyleSheet::GenericTableRule 120 - nsScreenManagerGtk 20 - nsJARProtocolHandler 28 - nsBaseURLParser 24 - nsSimpleURI 4 - nsLoadGroup 8 - nsDSURIContentListener 72 - nsZipReaderCache 76 - nsDefaultURIFixup 104 - nsFontCache 32 - XPCWrappedNativeProto 196 - nsExternalHelperAppService 88 - nsSupportsArray 56 - nsHashKey 96 - RDFServiceImpl 280 - nsScreen 32 - nsCSSRule 200 - nsScriptSecurityManager 88 - nsCStringKey 240 - nsXPCThreadJSContextStackImpl 20 - nsPrefBranch 64 - nsCategoryManager 80 - nsNavigator 84 - nsHashtable 616 - nsRDFResource 288 - nsDOMEventGroup 12 - xptiInterfaceInfo 40 - nsBindingManager 512 - nsEventQueueImpl 36 - imgLoader 16 - nsDocShell::InterfaceRequestorProxy 32 - nsEventListenerManager 528 - nsDOMClassInfo 40 - nsXULPDGlobalObject 28 - nsStandardURL 1288 - nsJARChannel 312 - XPCNativeScriptableInfo 56 - nsPrefService 44 - nsXPCWrappedJS 192 - nsScriptLoaderObserverProxy 32 - nsWebNavigationInfo 20 - CSSLoaderImpl 368 - DeviceContextImpl 160 - nsScriptLoader 88 - nsEventQueueServiceImpl 48 - nsJSEventListener 360 - nsGenericElement 7056 - nsCSSDeclaration 280 - nsJARURI 88 - nsNodeInfoManager 40 - nsDocLoader 304 - nsGlobalWindow 1736 - nsEntropyCollector 1048 - nsHTMLStyleSheet 184 - nsVoidArray 352 4300.00% AtomImpl 320 566.67% nsWeakReference 80 400.00% XPCWrappedNative 672 300.00% nsLocalFile 1680 250.00% XPCNativeScriptableShared 432 100.00%
Checked in. Thanks everyone for your help. Let's address any remaining or new issues in followup bugs. Checking in browser/app/profile/firefox.js; /cvsroot/mozilla/browser/app/profile/firefox.js,v <-- firefox.js new revision: 1.65; previous revision: 1.64 done Checking in browser/base/content/browser.js; /cvsroot/mozilla/browser/base/content/browser.js,v <-- browser.js new revision: 1.466; previous revision: 1.465 done Checking in browser/base/content/browser.xul; /cvsroot/mozilla/browser/base/content/browser.xul,v <-- browser.xul new revision: 1.267; previous revision: 1.266 done Checking in browser/components/Makefile.in; /cvsroot/mozilla/browser/components/Makefile.in,v <-- Makefile.in new revision: 1.40; previous revision: 1.39 done RCS file: /cvsroot/mozilla/browser/components/feedview/Makefile.in,v done Checking in browser/components/feedview/Makefile.in; /cvsroot/mozilla/browser/components/feedview/Makefile.in,v <-- Makefile.in initial revision: 1.1 done RCS file: /cvsroot/mozilla/browser/components/feedview/jar.mn,v done Checking in browser/components/feedview/jar.mn; /cvsroot/mozilla/browser/components/feedview/jar.mn,v <-- jar.mn initial revision: 1.1 done RCS file: /cvsroot/mozilla/browser/components/feedview/content/feedview.js,v done Checking in browser/components/feedview/content/feedview.js; /cvsroot/mozilla/browser/components/feedview/content/feedview.js,v <-- feedview.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/browser/components/feedview/content/feedview.xsl,v done Checking in browser/components/feedview/content/feedview.xsl; /cvsroot/mozilla/browser/components/feedview/content/feedview.xsl,v <-- feedview.xsl initial revision: 1.1 done RCS file: /cvsroot/mozilla/browser/components/feedview/content/feedviewOverlay.js,v done Checking in browser/components/feedview/content/feedviewOverlay.js; /cvsroot/mozilla/browser/components/feedview/content/feedviewOverlay.js,v <-- feedviewOverlay.js initial revision: 1.1 done Checking in browser/locales/jar.mn; /cvsroot/mozilla/browser/locales/jar.mn,v <-- jar.mn new revision: 1.22; previous revision: 1.21 done RCS file: /cvsroot/mozilla/browser/locales/en-US/chrome/browser/feedview.properties,v done Checking in browser/locales/en-US/chrome/browser/feedview.properties; /cvsroot/mozilla/browser/locales/en-US/chrome/browser/feedview.properties,v <-- feedview.properties initial revision: 1.1 done Checking in browser/themes/pinstripe/browser/jar.mn; /cvsroot/mozilla/browser/themes/pinstripe/browser/jar.mn,v <-- jar.mn new revision: 1.11; previous revision: 1.10 done RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/feedview/check.png,v done Checking in browser/themes/pinstripe/browser/feedview/check.png; /cvsroot/mozilla/browser/themes/pinstripe/browser/feedview/check.png,v <-- check.png initial revision: 1.1 done RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/feedview/feedview.css,v done Checking in browser/themes/pinstripe/browser/feedview/feedview.css; /cvsroot/mozilla/browser/themes/pinstripe/browser/feedview/feedview.css,v <-- feedview.css initial revision: 1.1 done RCS file: /cvsroot/mozilla/browser/themes/pinstripe/browser/feedview/itemSelected.png,v done Checking in browser/themes/pinstripe/browser/feedview/itemSelected.png; /cvsroot/mozilla/browser/themes/pinstripe/browser/feedview/itemSelected.png,v <-- itemSelected.png initial revision: 1.1 done Checking in browser/themes/winstripe/browser/jar.mn; /cvsroot/mozilla/browser/themes/winstripe/browser/jar.mn,v <-- jar.mn new revision: 1.12; previous revision: 1.11 done RCS file: /cvsroot/mozilla/browser/themes/winstripe/browser/feedview/check.png,v done Checking in browser/themes/winstripe/browser/feedview/check.png; /cvsroot/mozilla/browser/themes/winstripe/browser/feedview/check.png,v <-- check.png initial revision: 1.1 done RCS file: /cvsroot/mozilla/browser/themes/winstripe/browser/feedview/feedview.css,v done Checking in browser/themes/winstripe/browser/feedview/feedview.css; /cvsroot/mozilla/browser/themes/winstripe/browser/feedview/feedview.css,v <-- feedview.css initial revision: 1.1 done RCS file: /cvsroot/mozilla/browser/themes/winstripe/browser/feedview/itemSelected.png,v done Checking in browser/themes/winstripe/browser/feedview/itemSelected.png; /cvsroot/mozilla/browser/themes/winstripe/browser/feedview/itemSelected.png,v <-- itemSelected.png initial revision: 1.1 done
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox1.1
Depends on: 303043
Depends on: 303065
Blocks: 303105
Blocks: 303115
Blocks: 303121
Depends on: 303195
Depends on: 303199
Latest feedview has problems with unicode chars, see bug 303206
Depends on: 303206
Feedview could be generalized to support other XML formats. Like use rss20.xsl if it is a feed, use wsdl.xsl if it is a wsdl file. There could be an API to easily register an xsl file to be used when a specific namespace or documentElement is found. Small extensions can be installed to add support for an extra format. Possible formats: opml, wsdl, rss, atom, rdf, foaf, custome made...
Adding such a design has been discussed on #developers, but didn't make it. It sure is the right way to go for future developments.
No longer depends on: 303043
Depends on: 303400
Depends on: 303398
Depends on: 303412
Depends on: 303043
No longer blocks: 303121
Depends on: 303121
Depends on: 303645
Depends on: 303647
Depends on: 303684
No longer blocks: branching1.8
No longer depends on: 303043
Depends on: 303043
Component: General → RSS Discovery and Preview
Depends on: 304791
Depends on: 304553
No longer depends on: 304791
Note that active work on feedview seems to be occuring in bug 303848. I'm not sure if that should be added as a dependency or a blocker, here; they seem to share a lot of the same dependencies, actually.
I know that this is supposedly fixed, but the lack of easy way to add a feed to the bookmarks is getting to me. Perhaps pages with feeds should get an extra menu item in the bookmarks menu? Bookmark Feed? It is nice to be able to see a prety listing of the feeds content, but sometimes on my slow connection the feed takes ages to load.
(In reply to comment #56) > I know that this is supposedly fixed, but the lack of easy way to add a feed to > the bookmarks is getting to me. Perhaps pages with feeds should get an extra > menu item in the bookmarks menu? Bookmark Feed? > > It is nice to be able to see a prety listing of the feeds content, but sometimes > on my slow connection the feed takes ages to load. Feedview is not going to be implemented in 1.5 anymore.
I see Caleb, will we get back the normal RSS icon that allows us to bookmark a feed? The new positioning is good. Is there somewhere (another bug) more suited for talking about the RSS icon?
(In reply to comment #58) > I see Caleb, will we get back the normal RSS icon that allows us to bookmark a > feed? The new positioning is good. The pre-existing functionality will be restored (bug 305134), with some extra niceness. Specifically: - the icon will be moved up to the URL bar where it's more discoverable - when multiple feeds of the same format are detected, only one will be presented to the user - we're adding a menu item in the bookmarks menu for sec 508 compliance (bug 304497) > Is there somewhere (another bug) more suited for talking about the RSS icon? See bug 305799 for any changes to the newly restored 1.0-like RSS behaviour :)
*** Bug 306140 has been marked as a duplicate of this bug. ***
No longer depends on: 303400
No longer depends on: 303645
Resetting QA Contact to default.
QA Contact: general → rss.preview
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: