Last Comment Bug 484379 - Not detecting feed when type has leading and/or trailing whitespace
: Not detecting feed when type has leading and/or trailing whitespace
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Feed Discovery and Preview (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.17
Assigned To: Edmund Wong (:ewong)
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 484187
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-20 06:14 PDT by Robert Kaiser
Modified: 2012-11-27 06:06 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
wontfix
-
wontfix
wontfix
wontfix
fixed
fixed
fixed


Attachments
Include whitespace in test. (v1) (2.34 KB, patch)
2012-11-25 23:28 PST, Edmund Wong (:ewong)
neil: review+
Details | Diff | Splinter Review
Include whitespace in test. (v2) (2.34 KB, patch)
2012-11-26 01:40 PST, Edmund Wong (:ewong)
ewong: review+
bugspam.Callek: approval‑comm‑aurora+
bugspam.Callek: approval‑comm‑beta+
Details | Diff | Splinter Review
Test fix for patch. (v1) (1.29 KB, patch)
2012-11-26 05:55 PST, Edmund Wong (:ewong)
ewong: review+
philip.chee: approval‑comm‑aurora+
philip.chee: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Robert Kaiser 2009-03-20 06:14:26 PDT
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) 2012-01-12 08:59:45 PST
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 neil@parkwaycc.co.uk 2012-01-12 10:03:54 PST
suite/browser/tabbrowser.xml also only does an exact match.
Comment 3 Serge Gautherie (:sgautherie) 2012-01-12 10:35:26 PST
(In reply to neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 2012-01-12 13:02:47 PST
Sorry, but I don't want tabbrowser.xml depending on utilityOverlay.js
Comment 5 Justin Wood (:Callek) 2012-01-26 09:50:02 PST
Not a regression...
Comment 6 Edmund Wong (:ewong) 2012-11-25 23:28:17 PST
Created attachment 685054 [details] [diff] [review]
Include whitespace in test. (v1)
Comment 7 neil@parkwaycc.co.uk 2012-11-26 00:50:45 PST
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) 2012-11-26 01:40:23 PST
Created attachment 685070 [details] [diff] [review]
Include whitespace in test. (v2)
Comment 9 Edmund Wong (:ewong) 2012-11-26 01:43:52 PST
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/a74f2bbce842
Comment 10 Justin Wood (:Callek) 2012-11-26 01:51:15 PST
Comment on attachment 685070 [details] [diff] [review]
Include whitespace in test. (v2)

[Triage Comment]
Comment 11 Edmund Wong (:ewong) 2012-11-26 01:57:46 PST
Pushed to Comm-aurora:
http://hg.mozilla.org/releases/comm-aurora/rev/11e470198aae
Comment 12 Philip Chee 2012-11-26 02:32:14 PST
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] 2012-11-26 04:51:46 PST
(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 2012-11-26 04:55:29 PST
> Should be 'title="11"'?
Yes you're right :P
Comment 15 Edmund Wong (:ewong) 2012-11-26 05:55:28 PST
Created attachment 685127 [details] [diff] [review]
Test fix for patch. (v1)

rs=Ratty over irc.
Comment 16 Philip Chee 2012-11-26 05:58:38 PST
Comment on attachment 685127 [details] [diff] [review]
Test fix for patch. (v1)

[Triage Comment]
Comment 17 Edmund Wong (:ewong) 2012-11-26 06:03:26 PST
Pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/159102806630
Comment 18 Edmund Wong (:ewong) 2012-11-26 06:19:03 PST
Pushed to comm-aurora:
http://hg.mozilla.org/releases/comm-aurora/rev/eb871ba08465
Comment 19 Edmund Wong (:ewong) 2012-11-26 06:42:38 PST
Pushed to comm-beta:
http://hg.mozilla.org/releases/comm-beta/rev/5d098b1861bf

Note You need to log in before you can comment on or make changes to this bug.