383746 - The Page Info RSS section should use the code in browser.js to enumerate the page feeds

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect)

Description

[(no longer active)]

The current Page Info RSS section implementation <http://mxr.mozilla.org/mozilla/source/browser/base/content/pageinfo/feeds.js#45> iterates over the <link> elements, and chooses the ones with type set to either "application/rss+xml" or "application/atom+xml".  This is not in accordance to the behavior in <http://mxr.mozilla.org/mozilla/source/browser/base/content/browser.js#5635>, which first looks for <link>'s with rel=feed, and then considers those with "application/rss+xml" or "application/atom+xml", and finally "text/xml", "application/xml" or "application/rdf+xml" types with the word "rss" in their titles.

The mentioned code stores the found feeds in gBrowser.selectedBrowser.feeds.  Therefore, the Page Info code should not try to find feeds on its own, and should use gBrowser.selectedBrowser.feeds as its source data.

As an example of a page in which this discrepancy of behavior shows itself, check out the URL for this bug, which shows four feeds in the feed button in location bar, and only shows one in the Page Info feeds section.

Comment 1

[(no longer active)]

Attachment: Enumerate the feeds correctly in the Page Info dialog

I have added a browser argument to the Page Info dialog, which will be used to access the feeds array, which is used by the feed button in the location bar as well.  Therefore, some code necessary to support this have been added to pageInfo.js and browser.js as well.

Comment 2

[Asaf Romano (gone)]

Comment on attachment 267733 [details] [diff] [review]
Enumerate the feeds correctly in the Page Info dialog

Well, gBrowser.contentWindow points to gBrowser.selectedBrowser.contentWindow. With this change applied, one could open the page info dialog in a very inconsistent state.

You could rework the pageInfo dialog to actually take a browser rather than a window, but that seems like an overkill to me. I would rather gather the feeds list again, and put the sniffing/detection code in a shared js file (maybe utilityOverlay.js.

Comment 3

[(no longer active)]

(In reply to comment #2)
> You could rework the pageInfo dialog to actually take a browser rather than a
> window, but that seems like an overkill to me. I would rather gather the feeds
> list again, and put the sniffing/detection code in a shared js file (maybe
> utilityOverlay.js.

I think I'd go with the latter option.  I'll be posting a new patch shortly.

Comment 4

[(no longer active)]

Attachment: Enumerate the feeds correctly in the Page Info dialog (revised)

Revision to address the issues in comment 2.

The function recognizeFeedFromLink() is added to utilityOverlay.js.  This function refactors code from FeedHandler.onLinkAdded in browser.js.  It's used both in browser.js and in feeds.js.

I have tested this patch on various pages, and it works fine both in the location bar button and in the Page Info dialog's feed section.

Comment 5

[Asaf Romano (gone)]

Comment on attachment 267823 [details] [diff] [review]
Enumerate the feeds correctly in the Page Info dialog (revised)

The browser element/variable (which I could easily break btw) seems to be needed only for the security check... could you instead make this method take a principal object? In pageinfo context, that would be gDocument.nodePrincipal, I think.

Also, if the second element in the return-array is null only on failure, you could simplify the return type to be a bit less modern ;)

Comment 6

[(no longer active)]

Attachment: Enumerate the feeds correctly in the Page Info dialog (revised again)

Revised again to address the issues in comment 5.

Comment 7

[Asaf Romano (gone)]

Comment on attachment 268062 [details] [diff] [review]
Enumerate the feeds correctly in the Page Info dialog (revised again)

>Index: browser/base/content/browser.js
>===================================================================
>-      etype = etype.toLowerCase();
>-      isFeed = (etype == "application/rss+xml" ||
>-                etype == "application/atom+xml");
>-      if (!isFeed) {
>-        // really slimy: general XML types with magic letters in the title
>-        isFeed = ((etype == "text/xml" || etype == "application/xml" ||
>-                   etype == "application/rdf+xml") && rssTitleRegex.test(etitle));
>-      }
>-    }
>+    var feed = recognizeFeedFromLink(event.target, gBrowser.contentPrincipal);


While you're here, I think event.target.ownerDocument.nodePrincipal is a bit more correct.

>Index: browser/base/content/utilityOverlay.js
>===================================================================

>+/**
>+ * recognizeFeedFromLink: recognized RSS/ATOM feeds tom DOM link elements.

"recognizes", s/tom/from/?

>+ *
>+ * @param  aLink
>+ *         The DOM link element to check for representing a feed.
>+ * @param  aPrincipal
>+ *         The principal of the document used for security check

s/document/document,/ s/check/check/.

>+ * @return object
>+ *         The feed object containing href, type, and title properties,
>+ *          if successful, otherwise null.
>+ */

>+function recognizeFeedFromLink(aLink, aPrincipal) // code refactored from FeedHandler.onLinkAdded in browser.js

Please remove this comment.

>+{
>+  if (!aLink)
>+    return null;
>+

You could do the |!aPrincipal| check here too.

>+  var erel = aLink.rel;
>+  var etype = aLink.type;
>+  var etitle = aLink.title;
>+  const rssTitleRegex = /(^|\s)rss($|\s)/i;
>+  var rels = {};
>+
>+  if (erel) {
>+    for each (var relValue in erel.split(/\s+/))
>+      rels[relValue] = true;
>+  }
>+  var isFeed = rels.feed;
>+
>+  if (!isFeed &&
>+      (!rels.alternate || rels.stylesheet || !etype))

nit: one line here would be just 67 characters ;)


>+    // successful!  return the feed
>+    return {
>+        href: aLink.href,
>+        type: etype,
>+        title: aLink.title
>+      };
>+  }
>+  else
>+    return null;
>+}

nit: no |else| after return.

>Index: browser/base/content/pageinfo/feeds.js
>===================================================================

> {
>   const feedTypes = {
>     "application/rss+xml": gBundle.getString("feedRss"),
>-    "application/atom+xml": gBundle.getString("feedAtom")
>+    "application/atom+xml": gBundle.getString("feedAtom"),
>+    "text/xml": gBundle.getString("feedXML"),
>+    "application/xml": gBundle.getString("feedXML"),
>+    "application/rdf+xml": gBundle.getString("feedXML")
>   };
> 
>   // get the feeds
>   var linkNodes = gDocument.getElementsByTagName("link");
>   var length = linkNodes.length;
>   for (var i = 0; i < length; i++) {
>-    if (linkNodes[i].rel == "alternate" &&
>-        linkNodes[i].type in feedTypes &&
>-        linkNodes[i].href) {
>-      addRow(linkNodes[i].title,
>-             feedTypes[linkNodes[i].type],
>-             linkNodes[i].href);
>+    var feed = recognizeFeedFromLink( linkNodes[i], gDocument.nodePrincipal );

nit: remove the extra spaces.

Looks good otherwise, r=mano.

Comment 8

[(no longer active)]

Attachment: Enumerate the feeds correctly in the Page Info dialog (cleaned up)

Revised again to address the issues in comment 7.  I was not sure whether I should deprecate the old patch and ask a review on this one or not.  Therefore, I'm leaving the old patch like it was, and asking a review on this one as well.  Mano, please let me know if the old one should be deprecated.

Thanks!

Comment 9

[Asaf Romano (gone)]

Comment on attachment 268204 [details] [diff] [review]
Enumerate the feeds correctly in the Page Info dialog (cleaned up)

r=mano

Comment 10

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

Is there a reason you're asking for SR? /browser code doesn't require SR unless it's requested by a reviewer (see http://www.mozilla.org/projects/firefox/review.html).

Comment 11

[(no longer active)]

Comment on attachment 268204 [details] [diff] [review]
Enumerate the feeds correctly in the Page Info dialog (cleaned up)

(In reply to comment #10)
> Is there a reason you're asking for SR? /browser code doesn't require SR unless
> it's requested by a reviewer (see
> http://www.mozilla.org/projects/firefox/review.html).

Nope, my mistake.  So, I guess this patch is ready for check-in.

Comment 12

[(no longer active)]

(In reply to comment #10)
> Is there a reason you're asking for SR? /browser code doesn't require SR unless
> it's requested by a reviewer (see
> http://www.mozilla.org/projects/firefox/review.html).
> 

Comment 13

[Kenneth Herron]

Checked in "Enumerate the feeds correctly in the Page Info dialog (cleaned up)". Clearing checkin-needed status.

Comment 14

[(no longer active)]

(In reply to comment #13)
> Checked in "Enumerate the feeds correctly in the Page Info dialog (cleaned
> up)". Clearing checkin-needed status.
> 

It seems that the checkin somehow did not affect the browser/base/content/browser.js file.  Check out <http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/base/content/browser.js>, the bonsai blame does not show the code available in the patch attached as attachment 268204 [details] [diff] [review].  This happens not to break the trunk code, but I think it should be corrected ASAP.

Kenneth, can you investigate this please?  Thanks!

Comment 15

[Kenneth Herron]

Ehsan, I don't know how it happened, but you're right--browser.js was still waiting to be checked in. I've done that now.

Comment 16

[(no longer active)]

(In reply to comment #15)
> Ehsan, I don't know how it happened, but you're right--browser.js was still
> waiting to be checked in. I've done that now.
> 

Thanks!

Comment 17

[John P Baker]

Note that the list of feeds could be incomplete, even if consistent, until a patch for recognizeFeedFromLink is checked in from bug 377611; Most likely:

-  var erel = aLink.rel;
-  var etype = aLink.type;
+  var erel = aLink.rel && aLink.rel.toLowerCase();
+  var etype = aLink.type && aLink.etype.toLowerCase();

-    etype = etype.toLowerCase();

Comment 18

[(no longer active)]

(In reply to comment #17)
> Note that the list of feeds could be incomplete, even if consistent, until a
> patch for recognizeFeedFromLink is checked in from bug 377611; Most likely:

I've posted a patch on that bug, which is ready for checkin with r=mano.

Comment 19

[(no longer active)]

(In reply to comment #18)
> (In reply to comment #17)
> > Note that the list of feeds could be incomplete, even if consistent, until a
> > patch for recognizeFeedFromLink is checked in from bug 377611; Most likely:
> 
> I've posted a patch on that bug, which is ready for checkin with r=mano.

Done.  Checked in together with an automated testcase to make sure feed discovery won't break in the future.