369403 - GBK aliases should be aligned with IANA registration

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Mike Hommey [:glandium]]

Attachment: charsetalias patch

charsetalias.properties defines gbk and derivatives as aliases to x-gbk and euc-tw and derivatives as aliases to x-euc-tw.
x-gbk and x-euc-tw don't exist with native uconv. They are respectively gbk and euc-tw.

I have a patch to charsetalias.properties, that I am attaching, that addresses the alias issue, I think the non-native uconv should be updated so that it provides converters for gbk and euc-tw instead of x-gbk and x-euc-tw.

Comment 1

[Simon Montagu :smontagu]

Our policy is that charsets not defined in the IANA charset registry http://www.iana.org/assignments/character-sets have the x- prefix. I see that gbk is defined there, but euc-tw is not.

There are several other charset names with the x- prefix in non-native uconv and without it in native uconv, e.g. johab, mac-cyrillic. Maybe there could be a more general solution?

Comment 2

[Reed Loden [:reed]]

Is this still a problem on trunk today?

Comment 3

[Mike Hommey [:glandium]]

Yes, it still is, but the fix is not clear. The patch only outlines the problem.

Comment 4

[Simon Montagu :smontagu]

Comment on attachment 254072 [details] [diff] [review]
charsetalias patch

r- per comment 3

Comment 5

[Mike Hommey [:glandium]]

I don't even use native uconv anymore

Comment 6

[Manfred Zhu]

I found this bug is affected lots of Chinese users as x-gbk causes many MTAs are unable to decode it. It would be better we just use gbk which became an IANA character set in 2002: <http://www.iana.org/assignments/charset-reg/GBK>

Comment 7

[Simon Montagu :smontagu]

You're right of course. I noted that gbk was registered in IANA back in comment 1, and I should have thought through the implications and at least pushed that change in.

Axel, can we take changing x-gbk to gbk in FF 4 from the l10n point of view? There is an x-gbk.title key in dom/locales/en-US/chrome/charsetTitles.properties that will need to be renamed, and the encoding menus in toolkit/locales/en-US/chrome/global/intl.properties will also need to change.

Comment 8

[Simon Montagu :smontagu]

Really cc-ing [e-mail] this time -- Axel please see comment 7.

Comment 9

[Axel Hecht [:Pike]]

Not sure what the impact is, what happens as long as a locale didn't update 

intl.charsetmenu.browser.more3=GB2312, x-gbk, gb18030, HZ-GB-2312, ISO-2022-CN, 
Big5, Big5-HKSCS, x-euc-tw, EUC-JP, ISO-2022-JP, Shift_JIS, EUC-KR, x-windows-94
9, x-johab, ISO-2022-KR

?

Comment 10

[Simon Montagu :smontagu]

I'm fairly sure that then the menu wouldn't work, because we don't do alias resolution on charset names hard-coded in the UI.

Comment 11

[Axel Hecht [:Pike]]

I guess now is as bad a time as any, assuming that the actual patch is low-risk.

It's gonna require just tons of massaging.

... which makes me wonder, could we do something more intelligent about those menus? It is some pretty awkward code, I wonder how many localizers actually understand what they can do here, and why.

Comment 12

[Simon Montagu :smontagu]

Good questions. I don't have an answer to the "why" part myself. Do any localizers change the menus in practice?

Comment 13

[Simon Montagu :smontagu]

Attachment: Patch

Comment 14

[Axel Hecht [:Pike]]

Didn't do an extensive analysis, but ad-hoc I only found intl.charsetmenu.browser.static to be customized.

Comment 15

[Masatoshi Kimura [:emk]]

Comment on attachment 481508 [details] [diff] [review]
Patch

Should we move intl.charsetmenu.* (except intl.charsetmenu.static) to non-localizable pref as I'd done in bug 470209?

Comment 16

[Simon Montagu :smontagu]

I discovered some broken customizations, i.e. using a localized form of the name instead of the canonical name

http://mxr.mozilla.org/l10n-central/source/hr/toolkit/chrome/global/intl.properties
http://mxr.mozilla.org/l10n-central/source/ne-NP/toolkit/chrome/global/intl.properties
Unfortunately mxr doesn't show complete lines, so I would need to check out a complete l10n tree for a full analysis

Comment 17

[Simon Montagu :smontagu]

Comment on attachment 481508 [details] [diff] [review]
Patch

Asking approval 2.0. This is very low-risk: the only user-facing change is that we mark gbk output as "gbk" instead of "x-gbk". (For input we recognize both forms before and after this patch).

Per comment 6, outputting "x-gbk" causes problems for many Chinese users.

Comment 18

[Benjamin Smedberg]

beltzner/gavin, can one of you make a quick call on this one, it involves string changes so needs to land for b7 if we take it at all.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 481508 [details] [diff] [review]
Patch

This missed beta 7; I think the l10n complications described in comment 11 preclude this from landing for 2.0. Seems like we should get a bug on file to have these menus depend on localizable files less...

Comment 20

[Simon Montagu :smontagu]

Filed bug 610638 to make the menus non-localizable

Comment 21

[Simon Montagu :smontagu]

Attachment: Patch merged to bug 369403

Transferring r and rerequesting approval. This patch is identical to the previous, except that it patches all.js (after the changes from bug 610638) instead of intl.properties, and so has no l10n impact.

Comment 22

[Simon Montagu :smontagu]

Comment on attachment 489179 [details] [diff] [review]
Patch merged to bug 369403

Really requesting approval

Comment 23

[Benjamin Smedberg]

It's past string-freeze now.

Comment 24

[Henri Sivonen (:hsivonen)]

What's native uconv? It seems a bit scary to outsource character decoding considering how Web compat sensitive it is.

Comment 25

[Masatoshi Kimura [:emk]]

(In reply to comment #24)
Native uconv is no longer used (comment #5), and it doesn't even compile at the moment (bug 623864).
The purpose of this bug is changing the alias to align it with IANA registration and making more interoperable with other MTAs (comment #6). Updating title to reflect the current status.

Comment 26

[Mike Hommey [:glandium]]

(In reply to comment #25)
> (In reply to comment #24)
> Native uconv is no longer used (comment #5)

Note that I wasn't the only user, the wince port was using that code.

Comment 27

[Masatoshi Kimura [:emk]]

Attachment: updated to tip: for checkin

Please apply it over the patch of bug 604317.

Comment 28

[Simon Montagu :smontagu]

http://hg.mozilla.org/mozilla-central/rev/df0edb3a3d99