Bug 55453 – Pass preferred language information down to font fallback logic

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Takayoshi Kochi]

For Chromium on Linux, we don't use any locale information for selecting fallback font (no default or
CSS-supplied font has glyph for those characters).  This often happens especially for CJK pages
with font-family being 'Arial' or any other latin-only fonts.

Sometimes we see fontconfig pick up undesired font (CJK-unified font, which has wrong glyph for
unified character for some language, e.g. 誤's glyphs should be different between C & J)
and it ends up in wrong typeface rendered.

As Chromium/Linux uses fontconfig for selecting a font from character, fontconfig
returns the top result that contains glyph for it, regardless of font-family or language.

At least, we have to pass the language, then with some fontconfig configuration tweaks
we can pick a more appropriate font.

Attaching a patch to implement this, but this requires changes in Chromium code too.
See also http://crosbug.com/11699

[Takayoshi Kochi]

Created attachment 84205 [details]
This is a tentative patch trying to implement passing preferred language to font fallback logic.

[Alexey Proskuryakov]

(In reply to comment #0)
> For Chromium on Linux, we don't use any locale information for selecting fallback font

Do other versions of Chromium support that?

I thought it was a cross-paltform issue, tracked by bug 10874. The main blocker being that node language is slow to compute.

[Adam Langley]

Comment on attachment 84205 [details]
This is a tentative patch trying to implement passing preferred language to font fallback logic.

View in context: https://bugs.webkit.org/attachment.cgi?id=84205&action=review

> WebKit/chromium/public/gtk/WebFontInfo.h:48
> +    //   preferredLanguage: preferred language for the characters

The comment should specify the format of the language. There are (at least) two and three letter forms in use.

> WebKit/chromium/public/linux/WebSandboxSupport.h:-53
> -    virtual WebString getFontFamilyForCharacters(const WebUChar* characters, size_t numCharacters) = 0;

the indentation here looks to be wrong.

> WebCore/platform/graphics/chromium/FontCacheLinux.cpp:61
> +    icu::Locale locale = icu::Locale::getDefault();

If this is for Linux then is the best way to get the locale? In zygote_main_linux.cc we preload the ICU timezone information, but I don't see anything about locale. So, if ICU tries to access the disk for this then it'll fail.

[Takayoshi Kochi]

(In reply to comment #2)
> (In reply to comment #0)
> > For Chromium on Linux, we don't use any locale information for selecting fallback font
> 
> Do other versions of Chromium support that?
> 
> I thought it was a cross-paltform issue, tracked by bug 10874. The main blocker being that node language is slow to compute.

This change affects only Linux.
As FontCache has per-platform implementation and this is Linux-only issue.

But yes, more generically I think bug 10874 is the right direction to go.

I admit that this is an intermediate solution as it uses UI language, not
HTML language.

[Takayoshi Kochi]

Comment on attachment 84205 [details]
This is a tentative patch trying to implement passing preferred language to font fallback logic.

View in context: https://bugs.webkit.org/attachment.cgi?id=84205&action=review

>> WebKit/chromium/public/gtk/WebFontInfo.h:48
>> +    //   preferredLanguage: preferred language for the characters
> 
> The comment should specify the format of the language. There are (at least) two and three letter forms in use.

Done.

>> WebKit/chromium/public/linux/WebSandboxSupport.h:-53
>> -    virtual WebString getFontFamilyForCharacters(const WebUChar* characters, size_t numCharacters) = 0;
> 
> the indentation here looks to be wrong.

Fixed.

>> WebCore/platform/graphics/chromium/FontCacheLinux.cpp:61
>> +    icu::Locale locale = icu::Locale::getDefault();
> 
> If this is for Linux then is the best way to get the locale? In zygote_main_linux.cc we preload the ICU timezone information, but I don't see anything about locale. So, if ICU tries to access the disk for this then it'll fail.

This doesn't hit the disk.

In the chrome side of the patch, I explicitly set for each renderer the default ICU locale from
commandline option passed from browser process (--lang=xx).  getDefault() returns the value set then.

On Chrome OS, zygote and sandbox helper are spawned off of main browser process in very early stage,
where the browser process does not know about UI language at that point.  After user logs in,
the browser changes its UI language according to the user's pref, and then passes '--lang='
commandline option and this is the only knowledge that renderers know.

[Takayoshi Kochi]

Created attachment 84376 [details]
Reflected comment from agl.

[Jungshik Shin]

Comment on attachment 84376 [details]
Reflected comment from agl.

View in context: https://bugs.webkit.org/attachment.cgi?id=84376&action=review

> WebKit/chromium/public/gtk/WebFontInfo.h:49
> +    //                      or 2-letter with 2-letter region (e.g. "zh-cn")

Language codes can be longer than 2-letter (e.g. fil) .  The same is true of region code (numeric region code is 3-digit like 419). Also, actually, script code is better for the 2nd part (like zh-Hant vs zh-Hans). Anyway, just say 'locale identifier' (along with a couple of examples like zh-CN, ja) would suffice without going to details for this comment

[Jungshik Shin]

Thank you, Kochi, for the patch. 

In reply to comment #2 : 
What this patch does is roughly equivalent to what Chrome Windows does (to deal with CJK glyph preferences). As Kochi wrote, this is a kinda interim solution (that works in most cases but not always) and bug 10874 and related bugs have to be fixed eventually. I earnestly plan to those bugs in March. 

In case of Chrome Windows, everything happens in the renderer (Webkit), but in case of Chrome Linux, fontconfig call goes out to the browser process.

[Adam Langley]

Comment on attachment 84376 [details]
Reflected comment from agl.

LGTM (I am not a WebKit reviewer. I need an r+ from a real reviewer before landing.)

[Takayoshi Kochi]

Created attachment 84543 [details]
Reflected comment from Jungshik.

[Takayoshi Kochi]

Comment on attachment 84376 [details]
Reflected comment from agl.

View in context: https://bugs.webkit.org/attachment.cgi?id=84376&action=review

>> WebKit/chromium/public/gtk/WebFontInfo.h:49
>> +    //                      or 2-letter with 2-letter region (e.g. "zh-cn")
> 
> Language codes can be longer than 2-letter (e.g. fil) .  The same is true of region code (numeric region code is 3-digit like 419). Also, actually, script code is better for the 2nd part (like zh-Hant vs zh-Hans). Anyway, just say 'locale identifier' (along with a couple of examples like zh-CN, ja) would suffice without going to details for this comment

Done.

[Takayoshi Kochi]

Updated the patch for fixes to Jungshik's comment.

Also I included a couple of minor changes in WebFontInfo from the previous patch:
1. made preferredLocale optional (= 0 default parameter)
2. NULL check for preferredLocale

With 1, we can commit WebKit change side only once.
For Chromium, I'll submit http://codereview.chromium.org/6592065/ first
then I'll make follow-up changelist to really use this interface.

Jungshik, could you take another look?

[Tony Chang]

Comment on attachment 84543 [details]
Reflected comment from Jungshik.

View in context: https://bugs.webkit.org/attachment.cgi?id=84543&action=review

> WebKit/chromium/src/gtk/WebFontInfo.cpp:69
> +      FcLangSet* langset = FcLangSetCreate();
> +      FcLangSetAdd(langset, reinterpret_cast<const FcChar8 *>(preferredLocale));
> +      FcPatternAddLangSet(pattern, FC_LANG, langset);
> +      FcLangSetDestroy(langset);

WebKit style uses 4 space indents

[Takayoshi Kochi]

Comment on attachment 84543 [details]
Reflected comment from Jungshik.

View in context: https://bugs.webkit.org/attachment.cgi?id=84543&action=review

>> WebKit/chromium/src/gtk/WebFontInfo.cpp:69
>> +      FcLangSetDestroy(langset);
> 
> WebKit style uses 4 space indents

Fixed.

Thanks for the review!

[Takayoshi Kochi]

Created attachment 84584 [details]
Fix indentation pointed out by Tony.

[Tony Chang]

Comment on attachment 84584 [details]
Fix indentation pointed out by Tony.

View in context: https://bugs.webkit.org/attachment.cgi?id=84584&action=review

r- because this patch is missing a ChangeLog entry.  Also, shouldn't there be a LayoutTest?

> WebKit/chromium/public/gtk/WebFontInfo.h:53
> -    WEBKIT_API static WebCString familyForChars(const WebUChar* characters, size_t numCharacters);
> +    WEBKIT_API static WebCString familyForChars(const WebUChar* characters, size_t numCharacters, const char* preferredLocale = 0);

Please file a bug about making |preferredLocale| not have a default param after the chromium side is landed and rolled in.

[Takayoshi Kochi]

Created attachment 84920 [details]
Added ChangeLogs.

[Takayoshi Kochi]

Comment on attachment 84584 [details]
Fix indentation pointed out by Tony.

View in context: https://bugs.webkit.org/attachment.cgi?id=84584&action=review

>> WebKit/chromium/public/gtk/WebFontInfo.h:53
>> +    WEBKIT_API static WebCString familyForChars(const WebUChar* characters, size_t numCharacters, const char* preferredLocale = 0);
> 
> Please file a bug about making |preferredLocale| not have a default param after the chromium side is landed and rolled in.

Done.

https://bugs.webkit.org/show_bug.cgi?id=55752

[Takayoshi Kochi]

Created attachment 84922 [details]
Fixed wrong ChangeLog entries and diff paths (now Source/ prefixed).

[Takayoshi Kochi]

Tony, could you take another look at this?

If this looks good, could you put this into commit queue?

Thanks,

[Takayoshi Kochi]

(In reply to comment #16)
> (From update of attachment 84584 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84584&action=review
> 
> r- because this patch is missing a ChangeLog entry.  Also, shouldn't there be a LayoutTest?

As I also wrote in one of the ChangeLogs, the code depends on locale setting
and cannot be run the test easily in the framework.

So test will be added in the Chromium side.

[WebKit Commit Bot]

Comment on attachment 84922 [details]
Fixed wrong ChangeLog entries and diff paths (now Source/ prefixed).

Clearing flags on attachment: 84922

Committed r80489: <http://trac.webkit.org/changeset/80489>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.