328643 - support non-ASCII font name for pref

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Currently, we cannot use non-ASCII name font for default font.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

fix.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Jungshik:

Please hurry up to review this. Because this brakes PRE element's rendering. We have  inconvenience for using bugzilla.

Comment 3

[Jungshik Shin]

Comment on attachment 213242 [details] [diff] [review]
Patch rv1.0

r=jshin

I guess we should get rid of '*WithConversion' methods that have been obsolete for a long time. They're used so often by some  developers leading to I18N bugs like this.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

Comment 5

[Jungshik Shin]

Comment on attachment 213242 [details] [diff] [review]
Patch rv1.0


>             rv = prefs->GetCharPref(prefName.get(), getter_Copies(value));
>             if (NS_SUCCEEDED(rv) && value.get()) {
>                 mFamilies.AppendLiteral(",");
>-                mFamilies.AppendWithConversion(value);
>+                AppendUTF8toUTF16(value, mFamilies);

I forgot to add that you could have used GetComplexValue with nsISupportString, but it would make your code a little more verbose while saving the conversion. Anyway, will you add a comment that 'pref. file is always(?) in UTF-8 so that it's safe to treat what's obtained with GetCharPref as UTF-8'. 

Btw, can you make sure that this pref. is not localizable at the moment? If it is, I guess we need to use nsIPrefLocalizedString.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #5)
> (From update of attachment 213242 [details] [diff] [review] [edit])
> 
> >             rv = prefs->GetCharPref(prefName.get(), getter_Copies(value));
> >             if (NS_SUCCEEDED(rv) && value.get()) {
> >                 mFamilies.AppendLiteral(",");
> >-                mFamilies.AppendWithConversion(value);
> >+                AppendUTF8toUTF16(value, mFamilies);
> 
> I forgot to add that you could have used GetComplexValue with nsISupportString,
> but it would make your code a little more verbose while saving the conversion.
> Anyway, will you add a comment that 'pref. file is always(?) in UTF-8 so that
> it's safe to treat what's obtained with GetCharPref as UTF-8'. 
Oh, you're right. I'll work it.

> Btw, can you make sure that this pref. is not localizable at the moment? If it
> is, I guess we need to use nsIPrefLocalizedString.
Old gfx code used |nsISupportString| for getting this pref, maybe, we don't need it.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: additional comment

Please check this English. And do I need sr?

Comment 8

[Jungshik Shin]

Comment on attachment 213326 [details] [diff] [review]
additional comment

>Index: gfx/thebes/src/gfxFont.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/thebes/src/gfxFont.cpp,v
>retrieving revision 1.6
>diff -u -8 -p -r1.6 gfxFont.cpp
>--- gfx/thebes/src/gfxFont.cpp	27 Feb 2006 15:04:31 -0000	1.6
>+++ gfx/thebes/src/gfxFont.cpp	27 Feb 2006 16:36:29 -0000
>@@ -134,16 +134,20 @@ gfxFontGroup::ForEachFont(FontCreationCa
>             {
>                 generic = PR_TRUE;
> 
>                 nsCAutoString prefName("font.name.");
>                 prefName.AppendWithConversion(family);
>                 prefName.AppendLiteral(".");
>                 prefName.Append(mStyle.langGroup);
> 
>+                // XXX We should use |GetComplexValue| with |nsISupportString|.
>+                // However, |GetCharPref| always returns raw bytes that is
>+                // always encoded by UTF-8 at this pref. So, it's safe to treat
>+                // what's obtained with |GetCharPref| as UTF-8.

It's a little too verbose :-) I wanted something similar to

> prefs file always uses (must use) UTF-8 and fontconfig
> expects to get a UTF-8 string so that using |GetCharPref|
> is fine.

(at http://lxr.mozilla.org/seamonkey/source/gfx/src/shared/nsFontConfigUtils.cpp#258)

How about this? 
 
prefs file always uses (must use) UTF-8 so that we can use |GetCharPref| and treat the result as a UTF-8 string. 

If you really wanna  say sth. about |GetComplexValue|, .....

prefs file always uses (must use) UTF-8 so that we can get away with 
using |GetCharPref| and treat the result as a UTF-8 string instead of calling
|GetComplexValue| with |nsISupportString|. 
 
Whichever you like you can check in without further review.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

There are some bugs that is saving the native encoded string on Thunderbird.(But I don't confirm it, it's reported to Mozilla Japan.) So, we cannot say that the pref must be encoded by UTF-8 at all prefs...

But O.K. We should (must) use UTF-8 for prefs, that is true.
I'll check in following comment. thanks.

> prefs file always uses (must use) UTF-8 so that we can use |GetCharPref| and
> treat the result as a UTF-8 string. 

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: additional comment for check-in.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in. thanks.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Please don't mark patches as obsolete when they're not (especially when they've been checked in!) -- makes it hard to see what the actual patch is/patches are..