Summary: | Pass preferred language information down to font fallback logic | ||
---|---|---|---|
Product: | WebKit | Reporter: | Takayoshi Kochi <kochi> |
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | agl, ap, commit-queue, jshin, kochi, mitz, tony |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | Linux | ||
Attachments: |
Description
Takayoshi Kochi
2011-03-01 02:57:46 PST
Created attachment 84205 [details]
This is a tentative patch trying to implement passing preferred language to font fallback logic.
(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. 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. (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. 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. Created attachment 84376 [details]
Reflected comment from agl.
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 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. 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.)
Created attachment 84543 [details]
Reflected comment from Jungshik.
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. 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? 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 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! Created attachment 84584 [details]
Fix indentation pointed out by Tony.
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. Created attachment 84920 [details]
Added ChangeLogs.
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 Created attachment 84922 [details]
Fixed wrong ChangeLog entries and diff paths (now Source/ prefixed).
Tony, could you take another look at this? If this looks good, could you put this into commit queue? Thanks, (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. 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> All reviewed patches have been landed. Closing bug. |