563283 - Perform Hankaku to Zenkaku conversion in ISO-2022-JP encoder

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Simon Montagu :smontagu]

From bug 559489 comment 10:
I propose incorporating Hankaku-to-Zenkaku conversion into ISO-2022-JP encoder
(nsUnicodeToISO2022JP).
Rationales:
* Other browsers (at least Internet Explorer, Chrome, Safari for Win, and Opera
for Win) already support the conversion.
* Registration of CP50220 is in progress. It mandates the conversion on
sending.
  http://mail.apps.ietf.org/ietf/charsets/msg01882.html
  This charset is meant to be used as a replacement encoding for HTML5
"misinterpret for compatibility".

Comment 1

[Simon Montagu :smontagu]

Attachment: Patch

Comment 2

[Simon Montagu :smontagu]

Attachment: Patch v.2

Comment-only change from the previous attachment

Comment 3

[Masatoshi Kimura [:emk]]

Comment on attachment 448602 [details] [diff] [review]
Patch v.2

>   while (src < srcEnd) {
>+    if (IS_HANKAKU(*src)) {
>+      bcr = srcEnd - src;
>+      bcw = destEnd - dest;
>+      res = ConvertHankaku(src, &bcr, dest, &bcw);
>+      dest += bcw;
>+      src += bcr;
>+      if (res != NS_OK) {
>+        break;
>+      }
>+    }
>+
>     for (i=0; i< SIZE_OF_TABLES ; i++) {
Could you move IS_HANKAKU check after the table search? Hankaku-kana becomes less and less used. This will have a negative impact on encoder's performance.

Comment 4

[Simon Montagu :smontagu]

Attachment: Patch v.3

Fixed that, added some more tests, and fixed a bug which the tests exposed.

Comment 5

[Masatoshi Kimura [:emk]]

Comment on attachment 448809 [details] [diff] [review]
Patch v.3

>+#define JIS_X_201_INDEX 2
JIS_X_0208_INDEX

>--- a/intl/unicharutil/src/nsHankakuToZenkaku.cpp
>+++ /dev/null
Please keep this until bug 559489 is fixed. Otherwise comm-central will break.

Comment 6

[Simon Montagu :smontagu]

I just noticed that comm-central uses a hidden pref (mailnews.send_hankaku_kana) to disable hankaku conversion. Do you think we need to go on honoring this? If so I need to add code for it to the patch.

Comment 7

[Simon Montagu :smontagu]

Attachment: comm-central patch

Comment 8

[Masatoshi Kimura [:emk]]

(In reply to comment #6)
> I just noticed that comm-central uses a hidden pref
> (mailnews.send_hankaku_kana) to disable hankaku conversion. Do you think we
> need to go on honoring this? If so I need to add code for it to the patch.
The pref was broken by bug 49262 anyway. Now, nsUnicodeToISO2022JP does will convert hankaku kana to NCR which is useless in plain-text mail. I don't think it's worth keeping this rarely used, untested feature.

Comment 9

[Masatoshi Kimura [:emk]]

Comment on attachment 448809 [details] [diff] [review]
Patch v.3

Assuming JIS_X_201_INDEX is renamed to JIS_X_0208_INDEX.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 450423 [details] [diff] [review]
comm-central patch

So how can I test that comm-central still works with both these patches applied? (sorry, I'm not that up on charset conversions).

Also, as you're removing the code that tries to support  mailnews.send_hankaku_kana then you should also remove it from mailnews.js.

Comment 11

[Masatoshi Kimura [:emk]]

1. Compose a new mail.
2. Change charset to ISO-2022-JP.
3. Type Hankaku kana (or copy & paste from test_bug563283.js) on compose window.
4. Save the mail as a draft.
5. Open the draft again.
Hankaku characters should be converted to corresponding Zenkaku characters if the conversion still works.

Comment 12

[Simon Montagu :smontagu]

In case it's not obvious, if testing by copy and paste from test_bug563283.js, you can check the results by comparing the strings in the comments with the strings marked "equivalent to" below each one.

Also be careful not to copy the line below the comment "Hankaku preceded and followed by unencodable characters". If this is in a ISO-2022-JP message it will be silently re-encoded to UTF-8.

(In reply to comment #10)
> Also, as you're removing the code that tries to support 
> mailnews.send_hankaku_kana then you should also remove it from mailnews.js.

:S I don't see mailnews.send_hankaku_kana anywhere in mailnews.js

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 450423 [details] [diff] [review]
comm-central patch

That seems to work nicely. Thanks for the patch.

Comment 14

[Simon Montagu :smontagu]

http://hg.mozilla.org/comm-central/rev/c76d6e234cc5
http://hg.mozilla.org/mozilla-central/rev/596c193a9b67