Pass DOM locale to Skia in FontCache::getFontDataForCharacter

Pass DOM locale to Skia in FontCache::getFontDataForCharacter

Android OS has different font fallback lists for differnet locales.
Originally we let Skia select the fallback fonts based on the system
locale, without respecting the language specified by the web page.

Pass DOM locale to Skia so that it can fallback fonts according to the
DOM locale. This is important for example to display pages in Japanese
while the system locale is English.

Also distinguish FontData and PlatformFontData for different locales
in FontCache, so included locale in the hash key of them, otherwise
we may get glyphs from the glyph cache for wrong locale. 

For now this change affects behavior on Android only.

BUG=266412
TEST=ManualTest/font-fallback-locale.html

[eseidel, 2013-09-19 21:47:55]

This affects way more than just android.  All platforms will now be passed a
DOM-aware locale when looking up fallback fonts for a character.

I'm not sure how this is going to play with teh glyph cache (which is
per-process, not per page).  I think this means that the glyphs in the fallback
page will be specific to whatever locale they were first loaded from.

Suggesting that if a user loads a japanese page first, and then navigates to a
chinese page and the chosen fonts on either page do not have the glyphs in
question, they may have been entered into the glyph cache incorrectly.

I'll ahve eto check.

https://codereview.chromium.org/23503080/diff/3001/ManualTests/font-fallback-...
File ManualTests/font-fallback-locale.html (right):

https://codereview.chromium.org/23503080/diff/3001/ManualTests/font-fallback-...
ManualTests/font-fallback-locale.html:4: The following text should be in
Japanese font even if the system locale is not Japanese.
We shouldn't need a manual test for this, or?

[eseidel, 2013-09-19 21:48:15]

btw, it's good that it affects much more than android. :)

[Xianzhu, 2013-09-19 22:04:41]

On 2013/09/19 21:48:15, eseidel wrote:
> btw, it's good that it affects much more than android. :)

It actually affects Android only :) For other platforms, it only changes how
FontDescriptions store the locale (script before, locale string now), but
doesn't change how the locale/script is used. For now the locale is only used on
Android for FontCache::getFontDataForCharacter().

There are existing usages of FontDescriptions::script() changed to use
localeToScriptCodeForFontSelection(FontDescriptions::locale()), but the behavior
should remain unchanged.

The test is manual because of several reasons:
- layout test on Android doesn't work properly for now;
- it depends on specific system configuration (e.g. the system has a
/system/etc/fallback_fonts.xml containing "ja"-specific rules.)
- seems not feasible to control system locale in a layout test

Just verified the glyph cache issue you mentioned. Looking into it.

[Xianzhu, 2013-09-20 01:01:22]

PTAL.

In the new Patch Set, FontData/PlatformFontData from different locales are
distinguished in FontCache, so glyphs for different locales won't interfere.

This still only changes the actual behavior on Android because locale is
respected in FontCache::getFontDataForCharacter() on Android only.

[Xianzhu, 2013-09-20 17:23:13]

PTAL. The Patch Set 4 reduces the impact of this CL on other platforms by not
including locale into the map key on other platforms. For Patch Set 3, the other
platforms may create unnecessary duplicated FontData/PlatformFontData entries
for locales.

[Xianzhu, 2013-09-23 18:16:13]

ping...

[jungshik at Google, 2013-09-23 20:17:48]

If Skia on Android could 'create' a font other than 'sans', 'serif',
'monospace', we'd do without this change by reading per-script preferences from
fallback fonts xml files and setting accordingly. 
OTOH, this can be still useful for non-Android platforms.

[Xianzhu, 2013-09-23 20:55:26]

On 2013/09/23 20:17:48, Jungshik Shin wrote:
> If Skia on Android could 'create' a font other than 'sans', 'serif',
> 'monospace', we'd do without this change by reading per-script preferences
from
> fallback fonts xml files and setting accordingly. 
> OTOH, this can be still useful for non-Android platforms.

This perhaps would work. Trying now. Thanks for the idea.

[Xianzhu, 2013-09-24 00:36:16]

On 2013/09/23 20:55:26, Xianzhu wrote:
> On 2013/09/23 20:17:48, Jungshik Shin wrote:
> > If Skia on Android could 'create' a font other than 'sans', 'serif',
> > 'monospace', we'd do without this change by reading per-script preferences
> from
> > fallback fonts xml files and setting accordingly. 
> > OTOH, this can be still useful for non-Android platforms.
> 
> This perhaps would work. Trying now. Thanks for the idea.

This may work for characters that are in the preferred font, but will also bring
regressions to Latin characters. The main problem is that Android has only one
fallback font list. For Japanese locale, using the current per-script preference
method, we need to map all generic families to the pseudo family name of
MTLmr3m.ttf, then the original family information is lost. Latin characters
won't be fellback to the font corresponding to the original family.

For now this CL seems the only way to achieve locale-preferred fallback fonts.

[Xianzhu, 2013-09-24 01:39:48]

Patch Set 5 is FYI. It hardcodes rules for Japanese script. The regression is
that all Latin characters are also displayed in Japanese font.

[falken, 2013-09-25 08:11:50]

Based on the bug discussion, I'm not sure if this is still the approach you're
taking but I've put some comments for now.

It seems like we want to pass DOM locale to font fallback on all platforms,
right?

https://codereview.chromium.org/23503080/diff/13001/ManualTests/font-fallback...
File ManualTests/font-fallback-locale.html (right):

https://codereview.chromium.org/23503080/diff/13001/ManualTests/font-fallback...
ManualTests/font-fallback-locale.html:6: 一丁
Would it be easier to understand the test if you something like:

The font used for 海 should be different for Chinese vs Japanese, regardless of
system locale:
<div lang="zh">Chinese: 海</div>
<div lang="ja">Japanese: 海</div>

https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
File Source/core/platform/graphics/FontCache.cpp (right):

https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
Source/core/platform/graphics/FontCache.cpp:100: // for different locales of
FontDescriptions.
So eventually all platforms will want this? It seems we should do DOM-level
language based font fallback everywhere.

https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
Source/core/platform/graphics/FontCache.cpp:222: // to avoid conflict of glyphs
from different locale-perferred fallback fonts.
s/perferred/preferred

I think comment can use elaboration. Why is it an issue on Android and not other
platforms? Is it just a bug that we don't handle other platforms yet?

https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
File Source/core/platform/graphics/chromium/FontCacheAndroid.cpp (right):

https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
Source/core/platform/graphics/chromium/FontCacheAndroid.cpp:45: if
(!SkGetFallbackFamilyNameForChar(c,
font.fontDescription().locale().string().ascii().data(), &skiaFamilyName)
What happens if a page has -webkit-locale: '<crazy-non-ascii-text>';

I guess SkGetFallbackFamilyNameForChar can handle it OK?

https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
File Source/core/platform/graphics/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
Source/core/platform/graphics/skia/FontCacheSkia.cpp:176: // to avoid conflict
of glyphs from different locale-perferred fallback fonts.
Same as previous comment.

[djsollen, 2013-09-25 13:20:18]

https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
File Source/core/platform/graphics/chromium/FontCacheAndroid.cpp (right):

https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
Source/core/platform/graphics/chromium/FontCacheAndroid.cpp:45: if
(!SkGetFallbackFamilyNameForChar(c,
font.fontDescription().locale().string().ascii().data(), &skiaFamilyName)
On 2013/09/25 08:11:51, falken wrote:
> What happens if a page has -webkit-locale: '<crazy-non-ascii-text>';
> 
> I guess SkGetFallbackFamilyNameForChar can handle it OK?
> 

Skia uses that value to search over a list of known languages when selecting
which fallback chain to use.  It does however cache the incoming language for
efficient subsequent lookups.  It may be prudent to trim the input to a
reasonable value for a BCP-47 language identifier.  I think the spec recommends
around 42 characters.

see http://tools.ietf.org/html/bcp47#section-4.4

[eseidel, 2013-09-25 15:49:51]

https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
File Source/core/platform/graphics/FontCache.cpp (left):

https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
Source/core/platform/graphics/FontCache.cpp:105: inline unsigned
computeHash(const FontPlatformDataCacheKey& fontKey)
This is webkit's impl:
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/FontCac...

It appears we've inlined the description bits:
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/FontCac...

[eseidel, 2013-09-25 15:51:13]

https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
File Source/core/platform/graphics/FontCache.cpp (right):

https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
Source/core/platform/graphics/FontCache.cpp:101: AtomicString m_locale;
So WebKit doesn't seem to key based on locale.  I'm curious how they handle
these fonts correctly?  Do they?

[Xianzhu, 2013-09-25 15:59:26]

On 2013/09/25 15:51:13, eseidel wrote:
>
https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
> File Source/core/platform/graphics/FontCache.cpp (right):
> 
>
https://codereview.chromium.org/23503080/diff/13001/Source/core/platform/grap...
> Source/core/platform/graphics/FontCache.cpp:101: AtomicString m_locale;
> So WebKit doesn't seem to key based on locale.  I'm curious how they handle
> these fonts correctly?  Do they?

For other platforms, the general family fonts are distinguished for locales in
different glyph trees. For example, sans-serif is mapped to "VL PGothic" for ja
locale, so the glyph trees are separate for ja and other locales and don't need
locale to be a part of the key in FontCache. I implement a
"top-level-distingushing" method in https://codereview.chromium.org/24482002/
which hacks only for ja.

[Xianzhu, 2013-09-25 16:00:59]

On 2013/09/24 01:39:48, Xianzhu wrote:
> Patch Set 5 is FYI. It hardcodes rules for Japanese script. The regression is
> that all Latin characters are also displayed in Japanese font.

Sorry I should not have deleted it. Now it's in a separate change:
https://codereview.chromium.org/24482002/. Now I prefer CL 24482002 for
resolving ja locale font preference issue only.