Skip to content

Commit 603f6c5

Browse files
kojiishiChromium LUCI CQ
authored andcommitted
Reland "Support i18n serif font on Android"
This reverts commit 3228cc2. Reason for revert: * Confirmed 94.0.4581.0 does not have the issue. * See crbug.com/1252383#c17 for the new culprit. Original change's description: > Revert "Support i18n `serif` font on Android" > > This reverts commit 65ac769. > > Reason for revert: crbug.com/1252383 > > Original change's description: > > Support i18n `serif` font on Android > > > > Android's `fonts.xml` system used to support only one typeface > > for i18n. We started lifting this limitation in 2016. Android > > then supported locale-specific `serif` font in P, and Skia > > supported this in 2018[2]. > > > > This patch enables Blink to support the i18n `serif` fonts > > during cascading generic families; e.g.: > > ``` > > <div lang="ja" style="font-family: serif"> > > ``` > > should use the `serif` typeface for Japanese. > > > > Computing the actual font family in |FontSelector:: > > FamilyNameFromSettings| is not as cheap as other platforms, > > as it involves actual font look ups and instantiations. This > > patch changes it to no-op for Android, but instead, uses > > locale-specific family support in |FontCache|. > > > > Note, the system fallback goes different code path; e.g.: > > ``` > > <div lang="en" style="font-family: serif"> > > ``` > > uses English `serif` typeface. When it has Japanese text, it > > is the system fallback who finds the fonts. Supporting the > > system fallback will be in a separate patch. > > > > Design doc (Google only): > > https://docs.google.com/document/d/16k21QBf-FVsB-3yTHf2bmp5vI56uf44G-Xj_CWfT9Xs/edit?usp=sharing > > > > [1] https://skia-review.googlesource.com/c/skia/+/171644 > > > > Bug: 822142, 1206946 > > Change-Id: I3fe7e53868968342b70caffbb17092630a51e42e > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3003361 > > Reviewed-by: Kent Tamura <[email protected]> > > Commit-Queue: Koji Ishii <[email protected]> > > Cr-Commit-Position: refs/heads/master@{#902901} > > Bug: 1252383, 822142, 1206946 > Change-Id: Ieceb4b3ec5e07f0ab14df4f29e6fe6589da23d5c > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3191131 > Commit-Queue: Koji Ishii <[email protected]> > Reviewed-by: Ian Kilpatrick <[email protected]> > Cr-Commit-Position: refs/heads/main@{#925993} Bug: 1252383, 822142, 1206946 Change-Id: I2f1fadb51649624641e7acbfa22d54fb26928d93 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3195433 Bot-Commit: Rubber Stamper <[email protected]> Commit-Queue: Koji Ishii <[email protected]> Reviewed-by: Yoshifumi Inoue <[email protected]> Cr-Commit-Position: refs/heads/main@{#927560}
1 parent e891f5f commit 603f6c5

File tree

5 files changed

+128
-1
lines changed

5 files changed

+128
-1
lines changed

third_party/blink/renderer/platform/fonts/android/font_cache_android.cc

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,41 @@ const AtomicString& FontCache::SystemFontFamily() {
8282
// static
8383
void FontCache::SetSystemFontFamily(const AtomicString&) {}
8484

85+
sk_sp<SkTypeface> FontCache::CreateLocaleSpecificTypeface(
86+
const FontDescription& font_description,
87+
const char* locale_family_name) {
88+
const char* bcp47 = font_description.LocaleOrDefault().LocaleForSkFontMgr();
89+
DCHECK(bcp47);
90+
SkFontMgr* font_manager =
91+
font_manager_ ? font_manager_.get() : SkFontMgr::RefDefault().get();
92+
sk_sp<SkTypeface> typeface(font_manager->matchFamilyStyleCharacter(
93+
locale_family_name, font_description.SkiaFontStyle(), &bcp47,
94+
/* bcp47Count */ 1,
95+
// |matchFamilyStyleCharacter| is the only API that accepts |bcp47|, but
96+
// it also checks if a character has a glyph. To look up the first
97+
// match, use the space character, because all fonts are likely to have
98+
// a glyph for it.
99+
kSpaceCharacter));
100+
if (!typeface)
101+
return nullptr;
102+
103+
// When the specified family of the specified language does not exist, we want
104+
// to fall back to the specified family of the default language, but
105+
// |matchFamilyStyleCharacter| falls back to the default family of the
106+
// specified language. Get the default family of the language and compare
107+
// with what we get.
108+
SkString skia_family_name;
109+
typeface->getFamilyName(&skia_family_name);
110+
sk_sp<SkTypeface> fallback(font_manager->matchFamilyStyleCharacter(
111+
nullptr, font_description.SkiaFontStyle(), &bcp47,
112+
/* bcp47Count */ 1, kSpaceCharacter));
113+
SkString skia_fallback_name;
114+
fallback->getFamilyName(&skia_fallback_name);
115+
if (typeface != fallback)
116+
return typeface;
117+
return nullptr;
118+
}
119+
85120
scoped_refptr<SimpleFontData> FontCache::PlatformFallbackFontForCharacter(
86121
const FontDescription& font_description,
87122
UChar32 c,
@@ -160,6 +195,11 @@ scoped_refptr<SimpleFontData> FontCache::PlatformFallbackFontForCharacter(
160195
AtomicString FontCache::GetGenericFamilyNameForScript(
161196
const AtomicString& family_name,
162197
const FontDescription& font_description) {
198+
// If this is a locale-specifc family name, |FontCache| can handle different
199+
// typefaces per locale. Let it handle.
200+
if (GetLocaleSpecificFamilyName(family_name))
201+
return family_name;
202+
163203
// If monospace, do not apply CJK hack to find i18n fonts, because
164204
// i18n fonts are likely not monospace. Monospace is mostly used
165205
// for code, but when i18n characters appear in monospace, system

third_party/blink/renderer/platform/fonts/android/font_cache_android_test.cc

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,17 @@
1111
namespace blink {
1212

1313
class FontCacheAndroidTest : public testing::Test {
14+
protected:
15+
// Returns a locale-specific `serif` typeface, or `nullptr` if the system
16+
// does not have a locale-specific `serif`.
17+
sk_sp<SkTypeface> CreateSerifTypeface(const LayoutLocale* locale) {
18+
FontCache* font_cache = FontCache::GetFontCache();
19+
FontDescription font_description;
20+
font_description.SetLocale(locale);
21+
font_description.SetGenericFamily(FontDescription::kSerifFamily);
22+
return font_cache->CreateLocaleSpecificTypeface(font_description, "serif");
23+
}
24+
1425
FontCachePurgePreventer purge_preventer;
1526
};
1627

@@ -31,6 +42,46 @@ TEST_F(FontCacheAndroidTest, FallbackFontForCharacter) {
3142
EXPECT_TRUE(font_data);
3243
}
3344

45+
TEST_F(FontCacheAndroidTest, FallbackFontForCharacterSerif) {
46+
// Test is valid only if the system has a locale-specific `serif`.
47+
const LayoutLocale* ja = LayoutLocale::Get("ja");
48+
sk_sp<SkTypeface> serif_ja_typeface = CreateSerifTypeface(ja);
49+
if (!serif_ja_typeface)
50+
return;
51+
52+
// When |GenericFamily| set to |kSerifFamily|, it should find the
53+
// locale-specific serif font.
54+
FontDescription font_description;
55+
font_description.SetGenericFamily(FontDescription::kSerifFamily);
56+
font_description.SetLocale(ja);
57+
FontCache* font_cache = FontCache::GetFontCache();
58+
ASSERT_TRUE(font_cache);
59+
const UChar32 kTestChar = 0x4E00; // U+4E00 CJK UNIFIED IDEOGRAPH-4E00
60+
scoped_refptr<SimpleFontData> font_data =
61+
font_cache->FallbackFontForCharacter(font_description, kTestChar,
62+
nullptr);
63+
EXPECT_TRUE(font_data);
64+
EXPECT_EQ(serif_ja_typeface.get(), font_data->PlatformData().Typeface());
65+
}
66+
67+
TEST_F(FontCacheAndroidTest, LocaleSpecificTypeface) {
68+
// Test is valid only if the system has a locale-specific `serif`.
69+
const LayoutLocale* ja = LayoutLocale::Get("ja");
70+
sk_sp<SkTypeface> serif_ja_typeface = CreateSerifTypeface(ja);
71+
if (!serif_ja_typeface)
72+
return;
73+
74+
// If the system has one, it must be different from the default font.
75+
FontDescription standard_ja_description;
76+
standard_ja_description.SetLocale(ja);
77+
standard_ja_description.SetGenericFamily(FontDescription::kStandardFamily);
78+
std::string name;
79+
FontCache* font_cache = FontCache::GetFontCache();
80+
sk_sp<SkTypeface> standard_ja_typeface = font_cache->CreateTypeface(
81+
standard_ja_description, FontFaceCreationParams(), name);
82+
EXPECT_NE(serif_ja_typeface.get(), standard_ja_typeface.get());
83+
}
84+
3485
TEST(FontCacheAndroid, GenericFamilyNameForScript) {
3586
FontDescription english;
3687
english.SetLocale(LayoutLocale::Get("en"));

third_party/blink/renderer/platform/fonts/font_cache.h

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,13 @@ class PLATFORM_EXPORT FontCache {
251251
static AtomicString GetGenericFamilyNameForScript(
252252
const AtomicString& family_name,
253253
const FontDescription&);
254+
// Locale-specific families can use different |SkTypeface| for a family name
255+
// if locale is different.
256+
static const char* GetLocaleSpecificFamilyName(
257+
const AtomicString& family_name);
258+
sk_sp<SkTypeface> CreateLocaleSpecificTypeface(
259+
const FontDescription& font_description,
260+
const char* locale_family_name);
254261
#endif // defined(OS_ANDROID)
255262

256263
#if defined(OS_LINUX) || defined(OS_CHROMEOS)
@@ -406,6 +413,7 @@ class PLATFORM_EXPORT FontCache {
406413

407414
friend class SimpleFontData; // For fontDataFromFontPlatformData
408415
friend class FontFallbackList;
416+
FRIEND_TEST_ALL_PREFIXES(FontCacheAndroidTest, LocaleSpecificTypeface);
409417
};
410418

411419
class PLATFORM_EXPORT FontCachePurgePreventer {
@@ -420,6 +428,18 @@ class PLATFORM_EXPORT FontCachePurgePreventer {
420428

421429
AtomicString ToAtomicString(const SkString&);
422430

431+
#if defined(OS_ANDROID)
432+
// TODO(crbug.com/1241875) Can this be simplified?
433+
// static
434+
inline const char* FontCache::GetLocaleSpecificFamilyName(
435+
const AtomicString& family_name) {
436+
// Only `serif` has `fallbackFor` according to the current `fonts.xml`.
437+
if (family_name == font_family_names::kSerif)
438+
return "serif";
439+
return nullptr;
440+
}
441+
#endif
442+
423443
} // namespace blink
424444

425445
#endif // THIRD_PARTY_BLINK_RENDERER_PLATFORM_FONTS_FONT_CACHE_H_

third_party/blink/renderer/platform/fonts/font_description.cc

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@
3838
#include "third_party/blink/renderer/platform/wtf/text/string_hash.h"
3939
#include "third_party/blink/renderer/platform/wtf/text/string_hasher.h"
4040

41-
#if defined(OS_LINUX) || defined(OS_CHROMEOS)
41+
#if defined(OS_LINUX) || defined(OS_CHROMEOS) || defined(OS_ANDROID)
4242
#include "third_party/blink/renderer/platform/fonts/font_cache.h"
4343
#endif
4444

@@ -268,6 +268,12 @@ FontCacheKey FontDescription::CacheKey(
268268
options | font_selection_request_.GetHash() << 9,
269269
device_scale_factor_for_key, variation_settings_,
270270
is_unique_match);
271+
#if defined(OS_ANDROID)
272+
if (const LayoutLocale* locale = Locale()) {
273+
if (FontCache::GetLocaleSpecificFamilyName(creation_params.Family()))
274+
cache_key.SetLocale(locale->LocaleForSkFontMgr());
275+
}
276+
#endif // defined(OS_ANDROID)
271277
return cache_key;
272278
}
273279

third_party/blink/renderer/platform/fonts/skia/font_cache_skia.cc

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,16 @@ sk_sp<SkTypeface> FontCache::CreateTypeface(
217217
font_description.GenericFamily() == FontDescription::kStandardFamily);
218218
name = family.Utf8();
219219

220+
#if defined(OS_ANDROID)
221+
// If this is a locale-specific family, try looking up locale-specific
222+
// typeface first.
223+
if (const char* locale_family = GetLocaleSpecificFamilyName(family)) {
224+
if (sk_sp<SkTypeface> typeface =
225+
CreateLocaleSpecificTypeface(font_description, locale_family))
226+
return typeface;
227+
}
228+
#endif // defined(OS_ANDROID)
229+
220230
#if defined(OS_WIN)
221231
// TODO(vmpstr): Deal with paint typeface here.
222232
if (sideloaded_fonts_) {

0 commit comments

Comments
 (0)