450541 - Atom <title type="html"> should have HTML entities and numeric character references replaced

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[Phil Ringnalda (:philor)]

Although we correctly don't resolve double-escaped entities and NCRs in RSS titles (bug 277243), we incorrectly don't resolve them in Atom titles with type="html" - FeedParser.xmlUnescape() resolves >, <, and &, rather than everything from ’ to ’

The ideal way of fixing it would be to switch entirely to using the toolkit feed parsing service; a reasonable alternative would be to use its nsIScriptableUnescapeHTML service.

Comment 2

[alta88]

Attachment: tired of the bad subjects..

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 524078 [details] [diff] [review]
tired of the bad subjects..

Sorry for the delay in getting to this, do you have an example feed I could verify this on?

I think also we should be considering getting unit tests (probably via mozmill, but maybe xpcshell if feeds support it), for this, so that we can help to ensure stability for feeds in future. I'm willing to help you form some basic tests if you need it.

>diff --git a/mailnews/extensions/newsblog/content/FeedItem.js b/mailnews/extensions/newsblog/content/FeedItem.js

>+    var unescapeHTML = Components.classes["@mozilla.org/feed-unescapehtml;1"].
>+                       getService(Components.interfaces.nsIScriptableUnescapeHTML);
>+    title = unescapeHTML.unescape(title);

Nit: might as well just make this:

Components.classes["@mozilla.org/feed-unescapehtml;1"]
          .getService(Components.interfaces.nsIScriptableUnescapeHTML)
          .unescape(title);

Note that we tend to prefer having the dot at the start of the line for mailnews items.

Comment 4

[alta88]

Attachment: fix nit.

a sample feed might be:
http://www.nakedcapitalism.com/feed/atom

if there is an existing test for feeds you can point me to, i can adapt it for this small change.  i think it would be a bit of a curve though if this change were to start new tests for feeds, as i have no familiarity with them.

Comment 5

[Mark Banner (:standard8)]

Attachment: fix nit v2

Ok, I told you wrong. It should have been title = ...

Comment 6

[Mark Banner (:standard8)]

Attachment: Skeleton Test

I did start writing a test, but got stuck with how to select account central. I'm putting this here so that hopefully someone with a bit more time can pick it up, I'll post on the newsgroup about it.

However, I'm happy that the main patch can land without this.

Comment 7

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/bdd02cca51f5

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> Checked in: http://hg.mozilla.org/comm-central/rev/bdd02cca51f5

http://hg.mozilla.org/releases/comm-2.0/rev/8800391a9c4a