302121 - (feedview) Integrate Feedview for beta

Status: RESOLVED FIXED

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

Description

[Mike Connor [:mconnor]]

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.

Comment 1

[Mike Connor [:mconnor]]

Attachment: feedview source

Comment 2

[Mike Connor [:mconnor]]

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.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

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

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: quick hack to demonstrate concept

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.

Comment 6

[Phil Ringnalda (:philor)]

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

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: work in progress, binding approach

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.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: checkmark indicating feed is read

One of the two image files referenced in the CSS for the previously attached
patch.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: background image highlighting selected item

The other of the two image files referenced in the CSS for the previously
attached patch.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Attachment: second approach that forgoes binding

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?

Comment 11

[Myk Melez [:myk] [@mykmelez]]

cc:ing some folks who might be able to help with the security issues.  See
comment 7 and comment 10 for more details.

Comment 12

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

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.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

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

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 16

[Mike Connor [:mconnor]]

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.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Attachment: implementation v1: zip of extensions/feedview directory

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.

Comment 18

[Myk Melez [:myk] [@mykmelez]]

Attachment: patches allmakefiles.sh, configure.in and configure to include feedview extension

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Attachment: implementation v2: integrated, not an extension

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

Comment 20

[Axel Hecht]

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.

Comment 21

[Axel Hecht]

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

Comment 22

[Tom Germeau]

you can replace it with [e-mail] if a url is not allowed

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Attachment: implementation v2: code cleanup

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

Comment 24

[Caleb]

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.

Comment 25

[Myk Melez [:myk] [@mykmelez]]

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

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 27

[Phil Ringnalda (:philor)]

(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 28

[Myk Melez [:myk] [@mykmelez]]

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?

Comment 29

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 190950 [details] [diff] [review]
implementation v2: code cleanup

Removing review request in preparation for attaching newer version of patch.

Comment 30

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 190950 [details] [diff] [review]
implementation v2: code cleanup

Removing review request in preparation for attaching newer version of patch.

Comment 31

[Myk Melez [:myk] [@mykmelez]]

Attachment: implementation v3: lots more code cleanup

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?

Comment 32

[Caleb]

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 <[e-mail]>
As Axel Hecht suggested previously.

You'll need to change that everywhere.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 34

[Mike Connor [:mconnor]]

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.

Comment 35

[Axel Hecht]

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

Comment 36

[Myk Melez [:myk] [@mykmelez]]

Attachment: implementation v5: review fixes

> + * The Initial Developer of the Original Code is
> + * Tom Germeau (www.epigoon.com).
> 
> Should be:
> + * Tom Germeau <[e-mail]>

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.

Comment 37

[Axel Hecht]

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?

Comment 38

[Myk Melez [:myk] [@mykmelez]]

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

Comment 39

[Phil Ringnalda (:philor)]

Attachment: patch-patch for Atom 1.0

Wups, if we identify Atom 1.0 as a feed, we should probably do something with
it, too.

Comment 40

[Myk Melez [:myk] [@mykmelez]]

> 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 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 42

[Myk Melez [:myk] [@mykmelez]]

Attachment: implementation v6: Phil's Atom 1.0 fixes + canonicalize names

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.

Comment 43

[Myk Melez [:myk] [@mykmelez]]

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

Comment 44

[Myk Melez [:myk] [@mykmelez]]

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

Comment 45

[Myk Melez [:myk] [@mykmelez]]

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 46

[Mike Connor [:mconnor]]

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

Comment 47

[Myk Melez [:myk] [@mykmelez]]

Attachment: implementation v7: global pref vars initialized to default values

This version initializes the variables to the default values so if something
throws they're still set.

Comment 48

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 49

[Axel Hecht]

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.

Comment 50

[Caleb]

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%

Comment 51

[Myk Melez [:myk] [@mykmelez]]

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

Comment 52

[Bastian Grupe]

Latest feedview has problems with unicode chars, see bug 303206

Comment 53

[Tom Germeau]

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

Comment 54

[Axel Hecht]

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.

Comment 55

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

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.

Comment 56

[Rowan Lewis]

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.

Comment 57

[Caleb]

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

Comment 58

[Rowan Lewis]

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?

Comment 59

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

(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 :)

Comment 60

[Jo Hermans]

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

Comment 61

[Tanner M. Young [:tmyoung]]

Resetting QA Contact to default.