1349722 - Remove nsIScriptableUnicodeConverter from feeds

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, enhancement)

Comment 1

[alta88]

Attachment: uniconv.patch

Some debug left in, to be removed.

The issue is really that of Bug 1342838, that cpp gives js strings ISO-8859-1 encoded (and needs the reverse), and TextEncoder doesn't do anything but utf8.

Some useful sites for testing:
http://feeds.feedburner.com/chinadigitaltimes/IyPt
https://партиявеликоеотечество.рф/feed/

and any feed that might have an umlaut.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 8850192 [details] [diff] [review]
uniconv.patch

Review of attachment 8850192 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. Looks good to me, r=mkmelin

::: mailnews/extensions/newsblog/content/FeedItem.js
@@ +358,5 @@
>      msgFolder.gettingNewMessages = true;
>      // Source is a unicode string, we want to save a char * string in
>      // the original charset. So convert back.
> +FeedUtils.log.info("FeedItem.writeToFolder: source -\n" + source);
> +    source = unescape(encodeURIComponent(source));

So this is the to-utf-8 conversion. Maybe add a short comment that that's what it is.

::: mailnews/extensions/newsblog/content/newsblogOverlay.js
@@ +350,5 @@
>        MsgHdrToMimeMessage(msgHdr, null, function(aMsgHdr, aMimeMsg) {
>          if (aMimeMsg && aMimeMsg.headers["content-base"] &&
>              aMimeMsg.headers["content-base"][0])
>          {
> +          let url = decodeURIComponent(escape(aMimeMsg.headers["content-base"]));

shouldn't this prefrably be decodeURI(......)

and maybe here too add a comment about what's going on with the charset stuff.

Comment 3

[alta88]

(In reply to Magnus Melin from comment #2)
> Comment on attachment 8850192 [details] [diff] [review]
> uniconv.patch
> 
> Review of attachment 8850192 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay. Looks good to me, r=mkmelin
> 
> ::: mailnews/extensions/newsblog/content/FeedItem.js
> @@ +358,5 @@
> >      msgFolder.gettingNewMessages = true;
> >      // Source is a unicode string, we want to save a char * string in
> >      // the original charset. So convert back.
> > +FeedUtils.log.info("FeedItem.writeToFolder: source -\n" + source);
> > +    source = unescape(encodeURIComponent(source));
> 
> So this is the to-utf-8 conversion. Maybe add a short comment that that's
> what it is.
> 
> ::: mailnews/extensions/newsblog/content/newsblogOverlay.js
> @@ +350,5 @@
> >        MsgHdrToMimeMessage(msgHdr, null, function(aMsgHdr, aMimeMsg) {
> >          if (aMimeMsg && aMimeMsg.headers["content-base"] &&
> >              aMimeMsg.headers["content-base"][0])
> >          {
> > +          let url = decodeURIComponent(escape(aMimeMsg.headers["content-base"]));
> 
> shouldn't this prefrably be decodeURI(......)

decodeURI("%3A") returns "%3A" while decodeURIComponent("%3A") returns ":", so the former gives unusable urls..
i tried a number of things to make sure idn and non ascii urls all work when retrieved here.  it would be nice if henri confirmed these encode/decode methods in bug 1347877 as a replacement, but they are pretty standard js apis.

> 
> and maybe here too add a comment about what's going on with the charset
> stuff.

Comment 4

[alta88]

Attachment: uniconv.patch

updated for comments, removed debug.

Comment 5

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/55b04a77e7610a1907960a9268f5e816869cddc8