Remove nsIScriptableUnicodeConverter from feeds

RESOLVED FIXED in Thunderbird 55.0

Status

MailNews Core
Feed Reader
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: alta88, Assigned: alta88)

Tracking

(Blocks: 1 bug)

unspecified
Thunderbird 55.0

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qf-])

Attachments

(1 attachment, 1 obsolete attachment)

4.99 KB, patch
alta88
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

8 months ago
Created attachment 8850192 [details] [diff] [review]
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.
Assignee: nobody → alta88
Attachment #8850192 - Flags: review?(mkmelin+mozilla)
(Assignee)

Updated

8 months ago
Blocks: 1347877
Whiteboard: [qf]
Whiteboard: [qf] → [qf-]

Comment 2

7 months ago
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.
Attachment #8850192 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 3

7 months ago
(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.
(Assignee)

Comment 4

7 months ago
Created attachment 8858991 [details] [diff] [review]
uniconv.patch

updated for comments, removed debug.
Attachment #8850192 - Attachment is obsolete: true
Attachment #8858991 - Flags: review+
(Assignee)

Updated

7 months ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/55b04a77e7610a1907960a9268f5e816869cddc8
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
You need to log in before you can comment on or make changes to this bug.