Enable "system-ui" generic font family

third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp

Index: third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp
diff --git a/third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp b/third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp
index 16678e70b19b88a6feb4f67e57f32b195d9e5e93..c427d7823609a5c1e8afa69d274c1d9498604a54 100644
--- a/third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp
+++ b/third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp
@@ -229,21 +229,33 @@ sk_sp<SkTypeface> FontCache::createTypeface(
   }
 #endif
 
+#if OS(WIN)

[drott, 2016/10/13 07:59:35]

Okay, thanks for your explanations in the other patch regarding the resource
bundle localization of the font name and when this can fail.

Instead of mimicking the behaviour on the browser side and trying to emulate
that here and introducing a fallback to the Skia default font (which we don't do
anywhere else so far, I believe), can we just change the browser side code to
always set a name?

If the name is not localized then I'd suggest to get the default SkTypeface on
the browser side, call SkTypeface::getFamilyName and pass that on, so that we
don't have to special case on the Blink side and we'll always get exactly what
the browser side uses for the UI font.

This way we can remove the #if OS() switch here as well. 

[kojii, 2016/10/13 09:03:20]

Done in Blink. Sorry touching UI in browser is too far beyond my current
knowledge, hope this works for you.

[drott, 2016/10/13 12:36:13]

I'm reluctant to fallback to Skia's default font name here, since we can avoid
it and have this always exactly match what's used on the browser side. In
https://codereview.chromium.org/2138613002 line 173 you're retrieving the font
from the resource bundle. I've tried finding it, but where is the code that
determines the primary UI font on the browser side? IIUC you said that the
browser side prefers the font from the localized resource bundle, and if that
one is not used, Skia's default is used. How is the primary UI font accessed in
the browser? Instead of this change in line 173 in your other CL, I'd prefer a
situation where we always set this to exactly what is used on the Browser side.
We can retrieve Chrome's final UI font same as the UI code does it, and extract
the family name from there. This keeps things consistent and we don't have a
somewhat magic fallback to a default font name that can change below Blink.

[kojii, 2016/10/13 13:37:59]

Not only it's a lot of work for me, but also I don't think it is a good idea.
Linux uses MenuConfig in TOOLKIT_VIEWS, but CrOS uses something different. I
consulted with oshima@ and he recommended to use ResourceBundle instead, so here
we are. Please see the doc, Linux/ChromeOS section:
https://docs.google.com/document/d/1BI0OiWRUvsBOuPxPlF5J-_xtUZ49eVDUEXZXoF32Z...

Even if we wrote two separate code each for Linux and ChromeOS, I'm not sure we
can retrieve the SkTypeface from the menu control; I don't know TOOLKIT_VIEWS at
all and whatever menu system CrOS uses at all either, but generally UI controls
encapsulate the font instance and other such impl-specific details. MenuConfig
is there, AFAIU, so that callers do not have to deal with the font instance, no?

[drott, 2016/10/13 16:45:13]

In the document you're saying:

"ResourceBundle::GetSharedInstance().GetFontWithDelta(0).GetFontName() can get
the system font name on all platforms.
There are some tricky points:
On Linux, this is set to empty. Instead, Chromium relies on Skia’s capability to
find the default font by giving a null family name."

Could you point me to the code in Skia that does figure out a different default
font than Arial? I don't find it. Perhaps Chrome instead relies on specifying an
empty font family name to gfx::FontList, but I don't think Skia has this logic.

https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.cc?q=re...
seems to initialize the defaults for gfx::FontList.

My Chromium open source and my Chrome official build on Linux - if I identify
the font right - use "Ubuntu" as the menu/system font. Perhaps I am
misunderstanding something, but where is this coming from if the ResourceBundle
is empty?

Our last resort on Linux is "Sans", which maps to "DejaVu Sans" on my Ubuntu
Fontconfig. I believe ChromeOS uses Arimo as the system font.
linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt has Arial,
which does not make sense to me in this context. Or is this temporary until the
next CL - what will this result be after
https://codereview.chromium.org/2138613002 ? 

This is the reason I don't want to fallback to Skia's nullptr-initialized
SkTypeface here. 

How does Chrome figure out the system font - perhaps just a default constructed
gfx::FontList (after it's been initialised through resource bundle) does the
trick? In that case, using this in the upcoming CL for setting the system font
on Linux and extracting the family name of the first font in that list, and
setting this to the WebPreferences might be the solution?

[kojii, 2016/10/14 04:40:19]

Can you elaborate your question more? On Linux, there are 3 SkFontMgr's for
FontConfig, I admit I don't understand the relationship very well, but
SkFontConfig_direct always returns Arial. Is this what you're talking about?

SkFontMgr_fontconfig has a switch when familyName is null:

https://cs.chromium.org/chromium/src/third_party/skia/src/ports/SkFontMgr_fon...

but it goes to Fc* API, so hard to trace further. Android is easier to
understand "nullptr" maps to the default font:

https://cs.chromium.org/chromium/src/third_party/skia/src/ports/SkFontMgr_and...
There are two factors that prevents "-expected" to be the same font as you see
on your Linux:
1. defaultFontFamily() in this CL returns "Arial" with --run-layout-test flag
but "DejaVu Sans" without the flag. I believe this is to avoid test results
being flaky by different lang or resources, but I can't explain why more.
2. After the resource CL landed, we look at resources, but content-shell may
load different resources. See render_view_test.cc in the CL; it loads
"content_shell.pak" for CrOS since the content-shell.pak doesn't seem to have
it.

So when the next CL lands, rendering will change. On CrOS, it'll be to
IDS_UI_FONT_FAMILY_CROS (which looks like it is Roboto to me,) but "-expected"
might not reflect it.
True that I haven't really understood all the code paths on Linux/CrOS including
inside of FontConfig, but the part I've read so far looks nullptr returns the
Skia default font.

There's another alternative. In order to support "font: menu" etc. in CSS2/3:
https://drafts.csswg.org/css-fonts-3/#propdef-font

we already have LayoutThemeFontProvider::defaultGUIFont():

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La...

This always returns "Arial" on Linux/CrOS. If you prefer this on Linux/CrOS, I'm
fine with this.
I don't know all the code paths yet, it's quite too much work and overkill I
think, but in summary:

1. Linux uses MenuConfig. Not sure if this comes from IDS_UI_FONT_FAMILY (set to
"default" in .grd/.xtb file) or kept empty intentionally, but local
content-shell/chromium has empty. True not all code paths were really
identified.
2. According to oshima@, ResourceBundle is how CrOS determines the system font,
and this is empty on Linux.
3. Blink hardcodes "font: menu" to "Arial" on all Linux flavors today.

So I think this is good enough to start and look for feedback if any, but the
same method as "font: menu" is also fine with me. That might be safer, since
localization-dependent bug is harder to be found before stable.

[drott, 2016/10/14 07:34:21]

The phrasing in the document sounds to me like Skia would do anything smart to
figure out the _system_ default font. I don't think it does. 
Yes.
Could you try to answer this question, please?
It's okay to have something that does not match the system font when running
tests or content shell, but the UI case should work in all languages, so
independent of when the ResourceBundle for a language is empty.
Yes, but the Skia default font is not what we want since it's not the system
font. 
Same here, this is not the real system font. 
If MenuConfig works, then this can be used in the upcoming CL to set this value
in Linux, can't it?
Does this work for languages where the ResourceBundle does not explicitly set
this value?
I'd be much in favour of completing the story before we land this. 

The way I see it:
1) The system font family name on Linux and CrOS should always come from outside
Blink and be set through WebFontrendering and not allow an empty value. Falling
back to a Skia default in the Blink code, always Arial as much as I can tell,
does not have much to do with the concept of system-ui. 
2) On Linux, MenuConfig seems to be a promising candidate to set this to
WebPreferences.
3) On CrOS, ResourceBundle seems to be a good candidate, but how about languages
where the resource files don't mention a font name explicitly? How does
gfx::FontList know about the system font?
4) For testing: It's okay to initialise WebFontRendering's setting with
something hard-coded that's part of the set of fonts that content shell on Linux
loads (
https://cs.chromium.org/chromium/src/content/shell/app/blink_test_platform_su...
)

+  DCHECK_NE(family, FontFamilyNames::system_ui);
+  const char* familyNameForSkia = name.data();
+#else
+  // When the system UI font is set by setSystemFontFamily(), "system-ui" should
+  // have been replaced to it in getFontPlatformData. Use the Skia default
+  // family if it is not set, to be consistent with our UI/menu rendering.
+  const char* familyNameForSkia =
+      family == FontFamilyNames::system_ui ? nullptr : name.data();
+#endif
+
 #if OS(LINUX) || OS(WIN)
   // On linux if the fontManager has been overridden then we should be calling
   // the embedder provided font Manager rather than calling
   // SkTypeface::CreateFromName which may redirect the call to the default font
   // Manager.  On Windows the font manager is always present.
-  if (m_fontManager)
+  if (m_fontManager) {
     return sk_sp<SkTypeface>(m_fontManager->matchFamilyStyle(
-        name.data(), fontDescription.skiaFontStyle()));
+        familyNameForSkia, fontDescription.skiaFontStyle()));
+  }
 #endif
 
   // FIXME: Use m_fontManager, matchFamilyStyle instead of
   // legacyCreateTypeface on all platforms.
   sk_sp<SkFontMgr> fm(SkFontMgr::RefDefault());
-  return sk_sp<SkTypeface>(
-      fm->legacyCreateTypeface(name.data(), fontDescription.skiaFontStyle()));
+  return sk_sp<SkTypeface>(fm->legacyCreateTypeface(
+      familyNameForSkia, fontDescription.skiaFontStyle()));
 }
 
 #if !OS(WIN)