361230 - Add a way to tell the parser the feed has been sniffed

Status: RESOLVED FIXED

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

Description

[Robert Sayre]

If we encounter a document served as application/atom+xml, application/rss+xml, or linked via autodiscovery (and thus purported to be RSS/Atom), we should parse at all costs.

Conversely, if we find something we sniffed as a feed, but don't turn up useful information, we should reload as we do when we miss-sniff RDF feeds.

This should take care of sniffing bsmedberg's PHP Atom template and the like.

Comment 1

[Robert Sayre]

Attachment: set a response header that tells us whether we sniffed the feed

This should work out well. This detects clicks in the location bar and feed:// links, which never showed in the content area before. Both of those clicks are supposed to be to RSS/Atom feeds. It also assumes content served with feed-specific MIME types are supposed to be feeds. 

This also takes care of bsmedberg's Atom template, because it's shown to be sniffed, and doesn't turn up a useful result, so it's shown as text/plain after an unsuccessful parse (edge case).

Comment 2

[Asaf Romano (gone)]

Comment on attachment 246244 [details] [diff] [review]
set a response header that tells us whether we sniffed the feed

>? browser/components/feeds/src/347897.patch
>Index: browser/base/content/browser.js
>===================================================================

>    */
>   subscribeToFeed: function(href, event) {
>     // Just load the feed in the content area to either subscribe or show the
>     // preview UI
>     if (!href)
>       href = event.target.getAttribute("feed");
>     urlSecurityCheck(href, gBrowser.currentURI.spec,
>                      Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT_OR_DATA);
>+    var feedURI = makeURI(href, document.characterSet);
>+    if (feedURI.scheme == "http" || feedURI.scheme == "https")
>+      href = "feed:" + href;

Change this to |if (/^https?/.test(feedURI.scheme))| so we don't call GetScheme twice.
 
>Index: browser/components/feeds/src/FeedConverter.js
>===================================================================
>@@ -101,16 +101,21 @@ FeedConverter.prototype = {
>    */
>   _data: null,
>   
>   /**
>    * This is the object listening to the conversion, which is ultimately the
>    * docshell for the load.
>    */
>   _listener: null,
>+
>+  /**
>+   * Records if the feed was sniffed
>+   */
>+  _sniffed: false,
>   
>   /**
>    * See nsIStreamConverter.idl
>    */
>   canConvert: function FC_canConvert(sourceType, destinationType) {
>     // We only support one conversion.
>     return destinationType == TYPE_ANY && sourceType == TYPE_MAYBE_FEED;
>   },
>@@ -215,17 +220,22 @@ FeedConverter.prototype = {
>           }
>         }
>       }
>           
>       var ios = 
>           Cc["@mozilla.org/network/io-service;1"].
>           getService(Ci.nsIIOService);
>       var chromeChannel;
>-      if (result.doc) {
>+
>+      // show the feed page if it wasn't sniffed and we have a doc,
>+      // or we have a document, title, and link or id
>+      if ((!this._sniffed && result.doc) ||
>+          (result.doc && (result.doc.title != null) && 
>+           (result.doc.link || result.doc.id))) {

nit: make this:

> if (result.doc && (!this._sniffed ||
>     (result.doc.title && (result.doc.link || result.doc.id)))) {

>@@ -256,27 +266,40 @@ FeedConverter.prototype = {
>                                     sourceOffset, count);
>   },
>   
>   /**
>    * See nsIRequestObserver.idl
>    */
>   onStartRequest: function FC_onStartRequest(request, context) {
>     var channel = request.QueryInterface(Ci.nsIChannel);
>+    var httpChannel = channel.QueryInterface(Ci.nsIHttpChannel);
>+
>+    // Check for a header that tells us there was no sniffing
>+    if (httpChannel) {

QueryInterface would throw if not.

>+      try {
>+        var noSniff = httpChannel.getResponseHeader("X-Moz-Is-Feed");
>+        if (noSniff != "1")
>+          this._sniffed = true;

Unlike the feedsniffer change, this takes care of |X-Moz-Is-Feed: 0|.
Since this is an internal header either way is fine as long as you're consistent.



>     var channel = ios.newChannelFromURI(uri, null);
>-    channel.originalURI = uri;
>-    return channel;
>+    var httpChannel = channel.QueryInterface(Ci.nsIHttpChannel);

nit: use channel directly, i.e.

var channel = ios.newChannelFromURI(uri, null)
                 .QueryInterface(Ci.nsIHttpChannel);

>Index: browser/components/feeds/src/nsFeedSniffer.cpp
>===================================================================
>@@ -294,25 +294,41 @@ nsFeedSniffer::GetMIMETypeFromContent(ns
>   }
> 
>   // Check the Content-Type to see if it is set correctly. If it is set to 
>   // something specific that we think is a reliable indication of a feed, don't
>   // bother sniffing since we assume the site maintainer knows what they're 
>   // doing. 
>   nsCAutoString contentType;
>   channel->GetContentType(contentType);
>-  if (contentType.EqualsLiteral(TYPE_RSS) ||
>-      contentType.EqualsLiteral(TYPE_ATOM)) {
>-    
>+  PRBool noSniff;
>+  noSniff = contentType.EqualsLiteral(TYPE_RSS) ||
>+            contentType.EqualsLiteral(TYPE_ATOM);

nit: declare and init on the same line here

>+  // Check to see if this was a feed request from the location bar or from
>+  // the feed: protocol. This is also a reliable indication.
>+  if (!noSniff) {
>+    nsCAutoString sniffHeader;
>+    nsresult foundHeader =
>+      channel->GetRequestHeader(NS_LITERAL_CSTRING("X-Moz-Is-Feed"),
>+                                sniffHeader);
>+    noSniff = NS_SUCCEEDED(foundHeader);

see above.

Comment 3

[Robert Sayre]

Attachment: fix Mano's comments

Comment 4

[Robert Sayre]

Comment on attachment 246344 [details] [diff] [review]
fix Mano's comments

er, wrong patch... hold on

Comment 5

[Robert Sayre]

Attachment: fix Mano's comments

Comment 6

[Robert Sayre]

Attachment: fix Mano's comments

Comment 7

[Asaf Romano (gone)]

Comment on attachment 246350 [details] [diff] [review]
fix Mano's comments

>Index: browser/base/content/browser.js
>===================================================================
>    */
>   subscribeToFeed: function(href, event) {
>     // Just load the feed in the content area to either subscribe or show the
>     // preview UI
>     if (!href)
>       href = event.target.getAttribute("feed");
>     urlSecurityCheck(href, gBrowser.currentURI.spec,
>                      Ci.nsIScriptSecurityManager.DISALLOW_SCRIPT_OR_DATA);
>+    var feedURI = makeURI(href, document.characterSet);
>+    if (/^https?/.test(feedURI.scheme))
>+      href = "feed:" + href;

I would comment on the location bar case here rather than in FeedSniffer.

>Index: browser/components/feeds/src/FeedConverter.js
>===================================================================

>           
>       var ios = 
>           Cc["@mozilla.org/network/io-service;1"].
>           getService(Ci.nsIIOService);
>       var chromeChannel;
>-      if (result.doc) {
>+
>+      // show the feed page if it wasn't sniffed and we have a doc,
>+      // or we have a document, title, and link or id

nit: s/doc/document.

r=mano.

Comment 8

[Robert Sayre]

Attachment: patch to check in

Comment 9

[Robert Sayre]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.739; previous revision: 1.738
done
Checking in browser/components/feeds/src/FeedConverter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedConverter.js,v  <--  FeedConverter.js
new revision: 1.23; previous revision: 1.22
done
Checking in browser/components/feeds/src/nsFeedSniffer.cpp;
/cvsroot/mozilla/browser/components/feeds/src/nsFeedSniffer.cpp,v  <--  nsFeedSniffer.cpp
new revision: 1.16; previous revision: 1.15
done

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 246359 [details] [diff] [review]
patch to check in

approved for 1.8 branch, a=dveditz for drivers

Comment 11

[Robert Sayre]

Checking in browser/components/feeds/src/FeedConverter.js;
/cvsroot/mozilla/browser/components/feeds/src/FeedConverter.js,v  <--  FeedConverter.js
new revision: 1.1.2.22; previous revision: 1.1.2.21
done
Checking in browser/components/feeds/src/nsFeedSniffer.cpp;
/cvsroot/mozilla/browser/components/feeds/src/nsFeedSniffer.cpp,v  <--  nsFeedSniffer.cpp
new revision: 1.1.2.9; previous revision: 1.1.2.8
done
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.479.2.210; previous revision: 1.479.2.209
done

Comment 12

[Robert Sayre]

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

Comment 13

[Robert Sayre]

QA: http://svn.smedbergs.us/wordpress-atom10/tags/0.4/wp-atom10-comments.php

should not display the feed preview.