484379 - Not detecting feed when type has leading and/or trailing whitespace

Status: RESOLVED FIXED

(SeaMonkey :: Feed Discovery and Preview, defect)

Description

[Robert Kaiser]

feed_discovery.html ported from Firefox in bug 484187 uncovered that the following test case doesn't work correctly in SeaMonkey right now and is therefore marked todo() for now:

    <!-- type can have leading and trailing whitespace -->
    <link rel="alternate" type="  application/atom+xml  " title="todo11" href="/11.atom" />

Comment 1

[Serge Gautherie (:sgautherie)]

Hum, time to complete this, I think ;-<

I'm not familiar with this code, but I tried to have a look, fwiw:
http://mxr.mozilla.org/comm-central/ident?i=isValidFeed
*utilityOverlay.js: isValidFeed() regex (and code) are identical.
*feeds.js: initFeedTab() regex (and code) are different...
*...

Comment 2

[[e-mail]]

suite/browser/tabbrowser.xml also only does an exact match.

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to [e-mail] from comment #2)
> suite/browser/tabbrowser.xml also only does an exact match.

http://mxr.mozilla.org/comm-central/ident?i=onLinkAdded
*has "same" regex as SM initFeedTab()...
*FF calls isValidFeed() instead...
 Should SM do that too?

Comment 4

[[e-mail]]

Sorry, but I don't want tabbrowser.xml depending on utilityOverlay.js

Comment 5

[Justin Wood (:Callek)]

Not a regression...

Comment 6

[Edmund Wong (:ewong)]

Attachment: Include whitespace in test. (v1)

Comment 7

[[e-mail]]

Comment on attachment 685054 [details] [diff] [review]
Include whitespace in test. (v1)

>+                          /^(\s*)application\/(?:atom|rss)\+xml(\s*)$/i.test(type));
You don't need the ()s around \s*. r=me with that fixed.
isValidFeed actually accepts slightly more, it looks like this:
^\s*application\/(?:atom|rss)\+xml\s*(?:;.*)?$

Comment 8

[Edmund Wong (:ewong)]

Attachment: Include whitespace in test. (v2)

Comment 9

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/a74f2bbce842

Comment 10

[Justin Wood (:Callek)]

Comment on attachment 685070 [details] [diff] [review]
Include whitespace in test. (v2)

[Triage Comment]

Comment 11

[Edmund Wong (:ewong)]

Pushed to Comm-aurora:
http://hg.mozilla.org/releases/comm-aurora/rev/11e470198aae

Comment 12

[Philip Chee]

Can we remove the other todo checks in feed_discovery.html and test_feed_discovery.html now that we don't have any more todo tests here?

Comment 13

[Stefan [:stefanh]]

(In reply to Edmund Wong (:ewong) from comment #9)
> Pushed to comm-central:
> http://hg.mozilla.org/comm-central/rev/a74f2bbce842

-    <link rel="alternate" type="  application/atom+xml  " title="todo11" href="/11.atom" />
+    <link rel="alternate" type="  application/atom+xml  " title="10" href="/11.atom" />

Should be 'title="11"'?

Comment 14

[Philip Chee]

> Should be 'title="11"'?
Yes you're right :P

Comment 15

[Edmund Wong (:ewong)]

Attachment: Test fix for patch. (v1)

rs=Ratty over irc.

Comment 16

[Philip Chee]

Comment on attachment 685127 [details] [diff] [review]
Test fix for patch. (v1)

[Triage Comment]

Comment 17

[Edmund Wong (:ewong)]

Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/159102806630

Comment 18

[Edmund Wong (:ewong)]

Pushed to comm-aurora:
http://hg.mozilla.org/releases/comm-aurora/rev/eb871ba08465

Comment 19

[Edmund Wong (:ewong)]

Pushed to comm-beta:
http://hg.mozilla.org/releases/comm-beta/rev/5d098b1861bf