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

VERIFIED FIXED in Firefox 3 alpha7

Status

defect
VERIFIED FIXED
12 years ago
6 months ago

People

(Reporter: john.p.baker, Assigned: Ehsan)

Tracking

({regression})

Trunk
Firefox 3 alpha7
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

12 years ago
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;
Reporter

Updated

12 years ago
Keywords: regression
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Reporter

Comment 3

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

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

Reporter

Updated

12 years ago
Blocks: 362156
Posted patch Obvious fix (obsolete) — Splinter Review
Attachment #264829 - Flags: review?(mano)
Posted patch Failed testcase (obsolete) — Splinter Review
Unfortunately, I can't figure out how to reach up and grab the equivalent of getBrowser(), to get at the discovered feeds.
Flags: in-testsuite?
Flags: blocking-firefox3?
(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.
Oops, I think Bogus10 is bogusly bogus: the rock-paper-scissors rules must be "stylesheet covers alternate, but feed spills all over both."
Posted patch Working testcase (obsolete) — Splinter Review
Ooh, thanks, windowmediator seems to do the trick nicely.
Assignee: nobody → philringnalda
Attachment #264830 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #264968 - Flags: review?(sayrer)

Comment 10

12 years ago
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.
Reporter

Comment 11

12 years ago
(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

12 years ago
Comment on attachment 264968 [details] [diff] [review]
Working testcase

nice, thanks
Attachment #264968 - Flags: review?(sayrer) → review+

Comment 13

12 years ago
(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;
(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

12 years ago
(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.
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 alpha6
Assignee

Comment 16

12 years ago
Taking this over.  I'll post a patch soon.
Assignee: philringnalda → ehsan.akhgari
Status: ASSIGNED → NEW
Version: unspecified → Trunk
Assignee

Updated

12 years ago
Status: NEW → ASSIGNED
Assignee

Comment 17

12 years ago
Simple patch to fix the problem.
Attachment #264829 - Attachment is obsolete: true
Attachment #269658 - Flags: review?(mano)
Comment on attachment 269658 [details] [diff] [review]
Patch applied to recognizeFeedFromLink()

r=mano.
Attachment #269658 - Flags: review?(mano) → review+
Assignee

Updated

12 years ago
Whiteboard: [checkin needed]
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.
Whiteboard: [checkin needed]
Attachment #264968 - Attachment is obsolete: true
Whiteboard: [checkin needed]
Assignee

Comment 20

12 years ago
(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.
Attachment #270070 - Flags: review?(sayrer)

Updated

12 years ago
Attachment #270070 - Flags: review?(sayrer) → review+
Assignee

Comment 21

12 years ago
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.
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.
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Assignee

Updated

12 years ago
Keywords: verifyme
Reporter

Updated

12 years ago
Status: RESOLVED → VERIFIED
Keywords: verifyme

Updated

6 months ago
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.