812 bytes, text/plain
5.67 KB, patch
|Details | Diff | Splinter Review|
9.36 KB, patch
|Details | Diff | Splinter Review|
Assignee: nobody → alta88
Attachment #9044425 - Flags: feedback?(jorgk)
Comment on attachment 9044425 [details] [diff] [review] encoding.patch So if you're reverting my change from bug 1023285, you'll regress that bug (which I had forgotten about), no? Why would that be OK? The sample e-mail in that bug uses "raw" UTF-8 in the e-mail headers which is OK according to https://tools.ietf.org/html/rfc6532#section-3.2 as you pointed out yourself over in that bug. Well, strangely enough, I applied the patch and lo and behold, the Greek stuff still displays OK? How come? Since in a later bug we switched libmime to use jsmime for headers as well? (https://hg.mozilla.org/comm-central/rev/32170e6fb298#l1.38) - Or how do you explain that the regression doesn't happen? I'm still lost in the terminology here. What is "a raw octet binary string"? I think in bytes and encodings, so the way a byte string is interpreted as characters of a charset based on an encoding. In the test you added, clearly the source file is encoded in UTF-8, so "David Håsäther" is a byte-sequence which represent the UTF-8 encoding of those characters. You can even more clearly see it in the Greek example, Π is CE A0 in UTF-8. In summary: I'm OK with backing out my incorrect fix which happened at TB 45(!!) if we can understand why it doesn't cause a problem today.
Attachment #9044425 - Flags: feedback?(jorgk) → feedback+
Comment on attachment 9045240 [details] [diff] [review] encoding.patch (JK 2b) Ok, so the benefit of (all) this as opposed to the one liner is that it avoids an unnecessary conversion since utf8 is already sent up from libmime. But this is also why I said in comment 1 that the better place to look would be libmime, and output the 2047 encoded and raw string to its consumer (jsmime), thus offloading conversion to utf8 from libmime to jsmime. Something obviously never checked when implementing jsmime. Your version adds an extra function for one caller. Is that idl necessary? On the plus side, it does get rid of one deprecated function (though there are several remaining uses), but this is tangential to the central issue and probably should be done separately with all the others. You're confused by the output of unescape(encodeURIComponent()), a very standard and very hardened func, because devtools console. It shows like in comment 7 in stdout if you start from a shell command line. I think I'd almost rather go with the basically meaningless double conversion of these very short strings in a single message display situation than cargo cult yet another jsmime hack and create more idl. Also: https://developer.mozilla.org/en-US/docs/Web/API/WindowBase64/Base64_encoding_and_decoding#The_Unicode_Problem But if you really want to go this way, f+/r+. Also, if you're touching OutputEmailAddresses() to remove the deprecated function, perhaps better to use ``` for (let addr of addresses} ... address.emailAddress = addr.email; ... ``` instead of ``` while (index < numAddresses) ```
Attachment #9045240 - Flags: feedback?(alta88) → feedback+
Comment on attachment 9045384 [details] [diff] [review] 1528496-EncodedHeaderW.patch nice, doesn't feel as bad if use is made of that idl.. it feels like there would be many more instances of this multiple encoding/decoding in libmime and elsewhere. anyway, this sort of thing should have a try, no? r+ with that.
Attachment #9045384 - Flags: review?(alta88) → review+
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 67.0
Comment on attachment 9045603 [details] [diff] [review] encoding.patch (JK 2e) Surely a beta backport won't hurt here.
Attachment #9045603 - Flags: approval-comm-esr60? → approval-comm-esr60+
You need to log in before you can comment on or make changes to this bug.