377611 - RSS feed undiscovered - value of rel attribute should not be treated as case sensitive

Status: VERIFIED FIXED

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

Description

[John P Baker]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a4pre) Gecko/20070413 Minefield/3.0a4pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a4pre) Gecko/20070413 Minefield/3.0a4pre

The value of the rel attribute on a link is now treated as case sensitive for feed discovery.

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.bristol.ac.uk/is/news/

Actual Results:  
feed undiscovered

Expected Results:  
feed discovered

This is regression caused by the code for bug 362156 - before that the code checked using /(^|\s)alternate($|\s)/i , now it just checks the attribute as is.
Possible fix:

5621     if (erel) {
+          erel = erel.toLowerCase();
5622       for each (var relValue in erel.split(/\s+/))
5623         rels[relValue] = true;
5624     }
5625     var isFeed = rels.feed;
5626 
5627     if (!isFeed &&
5628         (!rels.alternate || rels.stylesheet || !etype))
5629       return;

Comment 1

[Phil Ringnalda (:philor)]

It's not automated, and doesn't cover feed && !stylesheet, but at the time attachment 191767 [details] covered the bits of http://diveintomark.org/tests/client/autodiscovery/ that seemed most in need of testing.

Comment 2

[Phil Ringnalda (:philor)]

Interesting, http://www.whatwg.org/specs/web-apps/current-work/#other1 describes a proposed rel value that differs from another only in case as "confusingly similar" not as "functionally identical" and case insensitivity isn't mentioned anywhere else.

Comment 3

[John P Baker]

(In reply to comment #2)
> ... and case insensitivity isn't mentioned anywhere else.

http://www.whatwg.org/specs/web-apps/current-work/#stability

Comment 4

[Phil Ringnalda (:philor)]

Attachment: Obvious fix

Comment 5

[Phil Ringnalda (:philor)]

Attachment: Failed testcase

Unfortunately, I can't figure out how to reach up and grab the equivalent of getBrowser(), to get at the discovered feeds.

Comment 6

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

(In reply to comment #5)
> Unfortunately, I can't figure out how to reach up and grab the equivalent of
> getBrowser(), to get at the discovered feeds.

You can't reach up through that boundary directly, IIRC, because that test harness just loads chrome:// URLs in the browser's type="content-primary" <browser>. You could probably use the windowmediator to get a reference to the browser window (which might be a bit fragile), or you could wait until bug 375469 is fixed.

Comment 7

[Phil Ringnalda (:philor)]

Oops, I think Bogus10 is bogusly bogus: the rock-paper-scissors rules must be "stylesheet covers alternate, but feed spills all over both."

Comment 8

[Asaf Romano (gone)]

Comment on attachment 264829 [details] [diff] [review]
Obvious fix

r=mano.

Comment 9

[Phil Ringnalda (:philor)]

Attachment: Working testcase

Ooh, thanks, windowmediator seems to do the trick nicely.

Comment 10

[Adam Kowalczyk]

FWIW, you can also do:

var outerWindow = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
                   .getInterface(Components.interfaces.nsIWebNavigation)
                   .QueryInterface(Components.interfaces.nsIDocShellTreeItem)
                   .rootTreeItem
                   .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
                   .getInterface(Components.interfaces.nsIDOMWindow)

It may be more reliable, I think.

Comment 11

[John P Baker]

(In reply to comment #4)
> Created an attachment (id=264829) [details]
> Obvious fix 

Surely this patch will regress bug 367356 and cause a new one.

> -    var erel = event.target.rel;
> -    var etype = event.target.type;
> +    var erel = event.target.rel.toLowerCase();
> +    var etype = event.target.type.toLowerCase();

You can't toLowerCase these before you have checked that they are defined

>      var etitle = event.target.title;
>      const rssTitleRegex = /(^|\s)rss($|\s)/i;
>      var rels = {};
>  
>      if (erel) {
HERE       ^^^^
>        for each (var relValue in erel.split(/\s+/))
>          rels[relValue] = true;
>      }
> @@ -5636,17 +5636,16 @@ var FeedHandler = {
>          (!rels.alternate || rels.stylesheet || !etype))
AND HERE                                          ^^^^^^ 
>        return;

Comment 12

[Robert Sayre]

Comment on attachment 264968 [details] [diff] [review]
Working testcase

nice, thanks

Comment 13

[Robert Sayre]

(In reply to comment #10)
> 
> It may be more reliable, I think.

...because?

(In reply to comment #11)
> > +    var erel = event.target.rel.toLowerCase();
> > +    var etype = event.target.type.toLowerCase();
> 
> You can't toLowerCase these before you have checked that they are defined

A good point. No need to dive into the stuff below though.

var erel = event.target.rel ? event.target.rel.toLowerCase() : null;

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> var erel = event.target.rel ? event.target.rel.toLowerCase() : null;

var erel = event.target.rel && event.target.rel.toLowerCase();

/be

Comment 15

[Adam Kowalczyk]

(In reply to comment #13)
> (In reply to comment #10)
> > 
> > It may be more reliable, I think.
> 
> ...because?

As Gavin said in comment 6, using windowmediator for this purpose is somewhat fragile and it's been recommended to use the other way. I am not sure about the specifics but I guess using windowmediator is indirect a bit hacky - it relies on the window in which we're running being the last one focused. Going through nsIDocShellTreeItem always allows us to get the top-level window of the current document.

Comment 16

[(no longer active)]

Taking this over.  I'll post a patch soon.

Comment 17

[(no longer active)]

Attachment: Patch applied to recognizeFeedFromLink()

Simple patch to fix the problem.

Comment 18

[Asaf Romano (gone)]

Comment on attachment 269658 [details] [diff] [review]
Patch applied to recognizeFeedFromLink()

r=mano.

Comment 19

[Phil Ringnalda (:philor)]

Don't forget the simple testcase to prevent the problem from coming back (and also to prevent anyone from completely breaking the feature). Pretty sure all you need to do is to open it in a popup, so it'll run when it's in the iframe in the all-tests-at-once run, not just when run separately.

Comment 20

[(no longer active)]

Attachment: Updated testcase

(In reply to comment #19)
> Don't forget the simple testcase to prevent the problem from coming back (and
> also to prevent anyone from completely breaking the feature). Pretty sure all
> you need to do is to open it in a popup, so it'll run when it's in the iframe
> in the all-tests-at-once run, not just when run separately.

Done.  The attached testcase can be run both in standalone mode and in all-tests-at-once run.  It passes all tests with the patch in attachment 269658 [details] [diff] [review] applied on the trunk.

Comment 21

[(no longer active)]

The checkin for this bug should include both attachment 269658 [details] [diff] [review] as the fix and attachment 270070 [details] [diff] [review] as the testcase.

Comment 22

[Phil Ringnalda (:philor)]

And it would have, two hours ago, if people would just have had the consideration not to turn the tree orange when I wanted to check something in.

Comment 23

[Phil Ringnalda (:philor)]

Thanks for picking up my slack, Ehsan.

browser/base/Makefile.in 1.20
browser/base/content/utilityOverlay.js 1.52
browser/base/content/test/Makefile.in 1.1
browser/base/content/test/feed_discovery.html 1.1
browser/base/content/test/test_feed_discovery.html 1.1