Some charsets have no langgroup defined

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: smontagu, Assigned: smontagu)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

11 years ago
Created attachment 284273 [details] [diff] [review]
Patch

The patch is based on the output from
grep LangGroup intl/uconv/src/charsetData.properties | cut -dL -f1 | sed -e "s/.$//" | sort -f > langgroups.txt
grep NS_UCONV_REG_UNREG[^_] intl/uconv/src/nsUConvModule.cpp | cut -d \" -f2 | sort -f > charsets.txt 
diff -i --old-line-format="" --unchanged-line-format="" langgroups.txt charsets.txt
Attachment #284273 - Flags: review?(dbaron)
Comment on attachment 284273 [details] [diff] [review]
Patch

Should ISO-8859-10 be x-baltic or x-western?  (I'm not sure I see the reason to distinguish in the first place, though.)

I can't even find any information about what a few of these are (t.61-8bit).

Nothing will be surprised to get x-gujr or x-guru, right?

r=dbaron
Attachment #284273 - Flags: review?(dbaron) → review+
And could you add a test that fails if any charsets are missing based on the test in comment 0?
(Assignee)

Comment 4

11 years ago
I think x-western is better than x-baltic for ISO-8859-10: it's supposed to cover "Nordic" languages, and we use x-western for Danish, Icelandic, Norwegian, and Swedish; and x-baltic for Estonian, Lithuanian and Latvian, which are covered by ISO-8859-4 and ISO-8859-13. (You may be right though that the distinction is moot: x-baltic and x-central-european are possibly both unnecessary).

t.61-8bit is a kind of pan-European character set, see http://ra.dkuug.dk/i18n/charmaps/T.61-8BIT. I believe it's used by LDAP.

x-gujr and x-guru shouldn't be a problem, we already use them for content tagged with lang="gu" and lang="pa[-IN]" respectively.
(Assignee)

Comment 5

11 years ago
Comment on attachment 284273 [details] [diff] [review]
Patch

Requesting approval. This is a very low-risk change, and blocks bug 391868, which is a blocker. Even though it is marked fixed, the fix checked in is a fallback and this is the real fix for the issue. See comments there, especially bug 391868 comment 22 and bug 391868 comment 23
Attachment #284273 - Flags: approval1.9?
(Assignee)

Comment 6

11 years ago
Comment on attachment 284613 [details] [diff] [review]
Test

Hmm, this test isn't great: it will happily pass even if the langGroup in the properties file is non-existent or misspelled. I'll add a pref check for font.size.variable.[langGroup]
(Assignee)

Comment 7

11 years ago
Created attachment 285200 [details] [diff] [review]
Another test

Slight change of plan: there are several langGroups that have no default font-family or font-size, and this isn't necessarily a problem as long as we fall back to something sane. This is a mochitest that checks that text in all charsets ends up with a non-zero font size.

Updated

11 years ago
Attachment #284273 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 8

11 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 481692
You need to log in before you can comment on or make changes to this bug.