Closed Bug 484379 Opened 11 years ago Closed 7 years ago

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

Categories

(SeaMonkey :: Feed Discovery and Preview, defect)

defect
Not set

Tracking

(seamonkey2.6 wontfix, seamonkey2.7- wontfix, seamonkey2.8 wontfix, seamonkey2.9 wontfix, seamonkey2.15 fixed, seamonkey2.16 fixed, seamonkey2.17 fixed)

RESOLVED FIXED
seamonkey2.17
Tracking Status
seamonkey2.6 --- wontfix
seamonkey2.7 - wontfix
seamonkey2.8 --- wontfix
seamonkey2.9 --- wontfix
seamonkey2.15 --- fixed
seamonkey2.16 --- fixed
seamonkey2.17 --- fixed

People

(Reporter: kairo, Assigned: ewong)

References

()

Details

Attachments

(2 files, 1 obsolete file)

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" />
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...
*...
Component: UI Design → Feed Discovery and Preview
QA Contact: ui-design → feed.handling
suite/browser/tabbrowser.xml also only does an exact match.
(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?
Sorry, but I don't want tabbrowser.xml depending on utilityOverlay.js
Not a regression...
Attached patch Include whitespace in test. (v1) (obsolete) — Splinter Review
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #685054 - Flags: review?(neil)
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*(?:;.*)?$
Attachment #685054 - Flags: review?(neil) → review+
Attachment #685054 - Attachment is obsolete: true
Attachment #685070 - Flags: review+
Comment on attachment 685070 [details] [diff] [review]
Include whitespace in test. (v2)

[Triage Comment]
Attachment #685070 - Flags: approval-comm-beta+
Attachment #685070 - Flags: approval-comm-aurora+
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?
Flags: needinfo?
(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"'?
Flags: needinfo?
> Should be 'title="11"'?
Yes you're right :P
rs=Ratty over irc.
Attachment #685127 - Flags: review+
Comment on attachment 685127 [details] [diff] [review]
Test fix for patch. (v1)

[Triage Comment]
Attachment #685127 - Flags: approval-comm-beta+
Attachment #685127 - Flags: approval-comm-aurora+
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.17
Keywords: helpwanted
You need to log in before you can comment on or make changes to this bug.