Enable "system-ui" generic font family

Enable "system-ui" generic font family

This patch enables the "system-ui" generic font family on all
platforms.

Following changes are planned but not included in this CL:
1. Content sets the current system font to Blink on Linux/CrOS[1].
2. Remove internal use of "BlinkMacSystemFont" and add UMA[2].

[1] https://codereview.chromium.org/2138613002
[2] https://codereview.chromium.org/2388623002

BUG=654679
Committed: https://crrev.com/bb3db4f221492076bca16af4d6fb9f358b79cedb
Cr-Commit-Position: refs/heads/master@{#425967}

[kojii, 2016-09-30 06:58:57]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 06:59:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 07:10:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 07:10:46]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[kojii, 2016-09-30 13:47:47]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 13:47:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 14:08:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 14:08:05]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[kojii, 2016-09-30 14:50:14]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 14:50:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-09-30 16:16:39]

The CQ bit was unchecked by kojii@chromium.org

[kojii, 2016-09-30 16:17:32]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 16:17:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-09-30 16:19:51]

The CQ bit was unchecked by kojii@chromium.org

[kojii, 2016-10-01 03:59:52]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-01 04:00:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-01 07:48:36]

The CQ bit was unchecked by kojii@chromium.org

[kojii, 2016-10-01 15:31:52]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-01 15:32:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-01 15:46:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-01 15:46:35]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[kojii, 2016-10-01 15:49:09]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-01 15:49:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-01 17:18:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-01 17:18:16]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-10-03 03:46:15]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 03:46:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 04:57:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 04:57:27]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-10-11 15:18:16]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-11 15:18:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-11 16:23:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-11 16:23:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kojii, 2016-10-12 04:58:13]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 04:58:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-12 05:13:08]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 05:13:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-12 05:13:26]

Patchset #15 (id:280001) has been deleted

[kojii, 2016-10-12 05:23:52]

Description was changed from

==========
WIP: system-ui

BUG=
==========

to

==========
Enable "system-ui" generic font family

This patch enables the "system-ui" generic font family on all
platforms.

Following changes are planned but not included in this CL:
1. Content sets the localized system font to Blink on Linux/CrOS[1].
2. Remove internal use of "BlinkMacSystemFont" and add UMA[2].
3. On Mac, expand "system-ui" to underlying name by size. This is
   expected to improve http://crbug.com/627609 .
4. On Mac, expand "system-ui" to underlying cascading list. This is
   more complex than other items and is expected to take longer.

[1] https://codereview.chromium.org/2138613002
[2] https://codereview.chromium.org/2388623002

BUG=654679
==========

[kojii, 2016-10-12 05:30:38]

The CQ bit was unchecked by kojii@chromium.org

[kojii, 2016-10-12 05:30:49]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[kojii, 2016-10-12 05:31:06]

Patchset #15 (id:260002) has been deleted

[commit-bot: I haz the power, 2016-10-12 05:31:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-12 08:14:29]

kojii@chromium.org changed reviewers:
+ drott@chromium.org, eae@chromium.org

[kojii, 2016-10-12 08:14:30]

PTAL.

[commit-bot: I haz the power, 2016-10-12 08:37:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 08:37:57]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-10-12 10:51:15]

Note, I found LayoutTheme in late stage, incorrectly assuming "font: menu" is
IE-only and thus Windows only, so this CL has little integration with
LayoutTheme. Maybe what Emil meant "menu" was "font: menu", not the menu in
browser. Should this be better integrated with LayoutTheme?

[drott, 2016-10-12 11:01:59]

Good change, looks mostly good to me, a few comments and questions on the float
parameter for systemFontForSize and the localized font name case.

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/generic-system-ui.html
(right):

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/generic-system-ui.html:1:
<!DOCTYPE html>
Good test, thanks.

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right):

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:72: const
AtomicString& FontCache::systemFontFamily(float) {
When will you need the parameter? If Mac instantiates with "systemFontForSize"
in FontFamilyMatcherMac, do we need this parameter here?

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:237: // been
replaced to the localied name. Use the Skia default family
Can you add some explanations where was it replaced? localied -> localized.
If I understand correctly, this will be in upcoming CLs? In this case, can you
add a TODO with bug link here?

When does this fail and we don't have a localized name? What are the chances it
fails?

As fallback if the localized name is missing, I'd prefer to fallback to one of
the browser side generic families instead of Skia's default family, would that
make sense? Skia's default is more similar to our LastResort - is that the
intention? Or would falling back to another FontFamilyNames category make more
sense? 

If it's closer to LastResort can we extract the name from
FontCache::getLastResortFallbackFont? That gives us more control and we don't
have to rely on Skia's default which may change.

[kojii, 2016-10-12 11:14:52]

Thank you for your quick review, replies inline.

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/generic-system-ui.html
(right):

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/inspector-protocol/layout-fonts/generic-system-ui.html:1:
<!DOCTYPE html>
On 2016/10/12 at 11:01:59, drott wrote:
> Good test, thanks.

;) Thanks for creating this framework, this is really helpful.

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right):

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:72: const
AtomicString& FontCache::systemFontFamily(float) {
On 2016/10/12 at 11:01:59, drott wrote:
> When will you need the parameter? If Mac instantiates with "systemFontForSize"
in FontFamilyMatcherMac, do we need this parameter here?

I'm thinking to replace "system-ui" with ".SF NS Text" or ".SF NS Display", then
with the list of cascading list. By replacing family name in this stage, we get
different FontData for each actual fonts.

This API surface supports only the first phase. I believe it's the significant
part of broken letter spacing, and would like to consider the design when I'm
more in sync with your plan to deprecate FontFallbackList and SimpleFontData.

I haven't really well tested replacing ".SF NS Text"/".SF NS Display" works well
yet, should have it soon. That said, this may need a revise, or two. I just
added size now because that gives me where FontCache can replace the family name
(i.e., only after it computed the size and know the locale.)

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:237: // been
replaced to the localied name. Use the Skia default family
On 2016/10/12 at 11:01:59, drott wrote:
> Can you add some explanations where was it replaced? localied -> localized.
> If I understand correctly, this will be in upcoming CLs? In this case, can you
add a TODO with bug link here?

FontCacheLinux already has setSystemFontFamily() in this CL, but nobody calls it
yet. Getting the string from resources and call it will be in the next CL
https://codereview.chromium.org/2138613002

> When does this fail and we don't have a localized name? What are the chances
it fails?

Linux English version does not have the string, so will use Skia default. I
believe CJK versions have localized font names in the resources. Also CrOS has
the resource even in English version.

> As fallback if the localized name is missing, I'd prefer to fallback to one of
the browser side generic families instead of Skia's default family, would that
make sense? Skia's default is more similar to our LastResort - is that the
intention? Or would falling back to another FontFamilyNames category make more
sense? 
> If it's closer to LastResort can we extract the name from
FontCache::getLastResortFallbackFont? That gives us more control and we don't
have to rely on Skia's default which may change.

It is the intention. When I read through our UI/menu rendering code in the
browser, Linux/English uses nullptr to get the font from Skia, so this is
mimicing our UI/menu rendering code. Our LastResort could be more suitable for
document rendering, the same logic as UI/menu rendering looks better to me, no?

Thanks for catching the spell error, will fix in the next CL.

[kojii, 2016-10-12 11:21:54]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 11:22:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-12 11:22:43]

> Thanks for catching the spell error, will fix in the next CL.

With your feedback, I rewrote the comments. I hope this is clearer.

[commit-bot: I haz the power, 2016-10-12 13:01:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 13:01:33]

Dry run: This issue passed the CQ dry run.

[eae, 2016-10-12 17:15:57]

I like it but please wait for drott. Thank you!

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

Hi Koji, thanks for the explanations, my comments below.

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right):

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:72: const
AtomicString& FontCache::systemFontFamily(float) {
So you're saying you would like to move the replacement/resolution of system-ui
from MatchNSFontFamily to here in order for use to get different
FontPlatformData objects?

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp (right):

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp:61:
mutableSystemFontFamily() =
Let's DCHECK for a non zero-length familyName argument here and assign it.

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:232: #if OS(WIN)
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:02:44]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 09:03:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Thank you for the review drott@, and thank you eae@ for sharing your thoughts.
Replies inline:

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right):

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:72: const
AtomicString& FontCache::systemFontFamily(float) {
On 2016/10/13 at 07:59:34, drott wrote:
> So you're saying you would like to move the replacement/resolution of
system-ui from MatchNSFontFamily to here in order for use to get different
FontPlatformData objects?

Yes, that way we can keep one SkTypeface/SimpleFontData/hb to map to single
OpenType font. It's not working really well yet, I need more your help on this
(will send separate e-mail) but I hope the direction is right?

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp (right):

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp:61:
mutableSystemFontFamily() =
On 2016/10/13 at 07:59:35, drott wrote:
> Let's DCHECK for a non zero-length familyName argument here and assign it.

Done.

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:232: #if OS(WIN)
On 2016/10/13 at 07:59:35, drott wrote:
> 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.

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

[kojii, 2016-10-13 09:17:08]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 09:17:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-13 09:17:39]

Patchset #17 (id:350001) has been deleted

[commit-bot: I haz the power, 2016-10-13 10:24:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-13 10:24:39]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right):

https://codereview.chromium.org/2137483004/diff/310001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:72: const
AtomicString& FontCache::systemFontFamily(float) {
On 2016/10/13 at 09:03:20, kojii wrote:


> Yes, that way we can keep one SkTypeface/SimpleFontData/hb to map to single
OpenType font. It's not working really well yet, I need more your help on this
(will send separate e-mail) but I hope the direction is right?

Yes, sounds reasonable.

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:232: #if OS(WIN)
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]

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:232: #if OS(WIN)
On 2016/10/13 at 12:36:13, drott wrote:
> 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.

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:14]

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:232: #if OS(WIN)
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]

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:232: #if OS(WIN)
On 2016/10/13 at 16:45:13, drott wrote:
> 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.

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...

> 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 ?

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.

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

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.

> 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?

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.

[kojii, 2016-10-14 04:42:04]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 04:42:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-14 05:03:27]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 05:03:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-14 05:08:18]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 05:08:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-14 05:08:46]

Patchset #19 (id:410001) has been deleted

[kojii, 2016-10-14 05:26:08]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[kojii, 2016-10-14 05:26:22]

Patchset #19 (id:430001) has been deleted

[commit-bot: I haz the power, 2016-10-14 05:26:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 06:44:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 06:44:46]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp (right):

https://codereview.chromium.org/2137483004/diff/330001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp:232: #if OS(WIN)
On 2016/10/14 at 04:40:19, kojii wrote:
> On 2016/10/13 at 16:45:13, drott wrote:
> > 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.
> 
> Can you elaborate your question more? 

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. 

> 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?

Yes.

> > 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?

Could you try to answer this question, please?

> > 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 ?
> 
> 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.

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.

> > This is the reason I don't want to fallback to Skia's nullptr-initialized
SkTypeface here. 
> 
> 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.

Yes, but the Skia default font is not what we want since it's not the system
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.

Same here, this is not the real system font. 

> > 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?
> 
> 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.

If MenuConfig works, then this can be used in the upcoming CL to set this value
in Linux, can't it?

> 2. According to oshima@, ResourceBundle is how CrOS determines the system
font, and this is empty on Linux.

Does this work for languages where the ResourceBundle does not explicitly set
this value?

> 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.

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...
)

[kojii, 2016-10-17 14:12:04]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 14:12:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 14:57:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-17 14:57:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kojii, 2016-10-17 15:58:00]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 15:58:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 18:28:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-17 18:28:49]

Dry run: Exceeded global retry quota

[kojii, 2016-10-18 04:05:04]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 04:05:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-18 04:10:20]

Description was changed from

==========
Enable "system-ui" generic font family

This patch enables the "system-ui" generic font family on all
platforms.

Following changes are planned but not included in this CL:
1. Content sets the localized system font to Blink on Linux/CrOS[1].
2. Remove internal use of "BlinkMacSystemFont" and add UMA[2].
3. On Mac, expand "system-ui" to underlying name by size. This is
   expected to improve http://crbug.com/627609 .
4. On Mac, expand "system-ui" to underlying cascading list. This is
   more complex than other items and is expected to take longer.

[1] https://codereview.chromium.org/2138613002
[2] https://codereview.chromium.org/2388623002

BUG=654679
==========

to

==========
Enable "system-ui" generic font family

This patch enables the "system-ui" generic font family on all
platforms.

Following changes are planned but not included in this CL:
1. Content sets the localized system font to Blink on Linux/CrOS[1].
2. Remove internal use of "BlinkMacSystemFont" and add UMA[2].
3. On Mac, expand "system-ui" to underlying cascading list. This is
   more complex than other items and is expected to take longer.

[1] https://codereview.chromium.org/2138613002
[2] https://codereview.chromium.org/2388623002

BUG=654679
==========

[kojii, 2016-10-18 04:12:10]

Description was changed from

==========
Enable "system-ui" generic font family

This patch enables the "system-ui" generic font family on all
platforms.

Following changes are planned but not included in this CL:
1. Content sets the localized system font to Blink on Linux/CrOS[1].
2. Remove internal use of "BlinkMacSystemFont" and add UMA[2].
3. On Mac, expand "system-ui" to underlying cascading list. This is
   more complex than other items and is expected to take longer.

[1] https://codereview.chromium.org/2138613002
[2] https://codereview.chromium.org/2388623002

BUG=654679
==========

to

==========
Enable "system-ui" generic font family

This patch enables the "system-ui" generic font family on all
platforms.

Following changes are planned but not included in this CL:
1. Content sets the localized system font to Blink on Linux/CrOS[1].
2. Remove internal use of "BlinkMacSystemFont" and add UMA[2].

[1] https://codereview.chromium.org/2138613002
[2] https://codereview.chromium.org/2388623002

BUG=654679
==========

[commit-bot: I haz the power, 2016-10-18 05:29:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 05:29:13]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-10-18 07:35:07]

PTAL. All our GV discussion reflected, and the following Linux patch[1] looks
good.

[drott, 2016-10-18 07:48:15]

LGTM now, with a nit on the nested #ifdefs. Thanks for updating the CL.

[kojii, 2016-10-18 08:55:30]

On 2016/10/18 at 07:48:15, drott wrote:
> LGTM now, with a nit on the nested #ifdefs. Thanks for updating the CL.

Thank you for your prompt review.

I can't find the nits...is it about adding comments like this:
#endif // !OS(MACOSX)
?

[drott, 2016-10-18 10:16:30]

> I can't find the nits...is it about adding comments like this:
> #endif // !OS(MACOSX)
> ?

Sorry, I somehow replied instead of posting my comment, it seems. Hope this
includes them now.

https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt
(right):

https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt:3:
"Times New Roman" : 37
Curious: Are we going to set this to something else for testing, in a subsequent
CL?

https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right):

https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:124: #else
Nit perhaps, but I am considering how we could make this more readable and
remove the #ifdef nesting here. Could you move this block up, first, and use
#ifdef OS(MACOSX), then just return getFontPlatformData, then #endif, then write
the rest? I think that would avoid the nesting.

[kojii, 2016-10-18 10:37:36]

https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt
(right):

https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt:3:
"Times New Roman" : 37
On 2016/10/18 at 10:16:29, drott wrote:
> Curious: Are we going to set this to something else for testing, in a
subsequent CL?

Yes and no. It used to be the Skia default font, so Arial. In PS22, we handle
"system-ui" as not found, and we fallback to the document default font.

Chrome will set correctly in a subsequent CL, but content-shell/run-layout-test
will not, so this -expected file will not change, but you will see the correct
font in Chrome.

https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right):

https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:124: #else
On 2016/10/18 at 10:16:30, drott wrote:
> Nit perhaps, but I am considering how we could make this more readable and
remove the #ifdef nesting here. Could you move this block up, first, and use
#ifdef OS(MACOSX), then just return getFontPlatformData, then #endif, then write
the rest? I think that would avoid the nesting.

Ok, it's still nesting IIUC but nest in #else might be better.

[kojii, 2016-10-18 10:41:08]

The CQ bit was checked by kojii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 10:41:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-18 10:42:41]

nit done.

https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/fonts/FontCache.cpp (right):

https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/FontCache.cpp:124: #else
On 2016/10/18 at 10:37:35, kojii wrote:
> On 2016/10/18 at 10:16:30, drott wrote:
> > Nit perhaps, but I am considering how we could make this more readable and
remove the #ifdef nesting here. Could you move this block up, first, and use
#ifdef OS(MACOSX), then just return getFontPlatformData, then #endif, then write
the rest? I think that would avoid the nesting.
> 
> Ok, it's still nesting IIUC but nest in #else might be better.

Note, this part will need some re-write, since when we add UMA, this function
doesn't have Document. I'm still seeking for the best place to convert
BlinkMacSystemFont in a following CL.

[drott, 2016-10-18 10:51:33]

On 2016/10/18 at 10:37:36, kojii wrote:
>
https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt
(right):
> 
>
https://codereview.chromium.org/2137483004/diff/510001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt:3:
"Times New Roman" : 37
> On 2016/10/18 at 10:16:29, drott wrote:
> > Curious: Are we going to set this to something else for testing, in a
subsequent CL?
> 
> Yes and no. It used to be the Skia default font, so Arial. In PS22, we handle
"system-ui" as not found, and we fallback to the document default font.
> 
> Chrome will set correctly in a subsequent CL, but
content-shell/run-layout-test will not, so this -expected file will not change,
but you will see the correct font in Chrome.

Okay, thanks. Are we not setting WebFontRendering preferences in testing at all?
Or are we just not setting the system font? In the latter case, perhaps we could
set it to one of the fonts loaded in fontconfig_util_linux just to make it
different from Times in testing, but that's a minor thing.

[drott, 2016-10-18 10:55:10]

On 2016/10/18 at 10:42:41, kojii wrote:

> Note, this part will need some re-write, since when we add UMA, this function
doesn't have Document. I'm still seeking for the best place to convert
BlinkMacSystemFont in a following CL.

What do you plan to measure with UMA, BlinkMacSystemFont usage? And you need the
document for UMA? You can perhaps check HarfBuzzFace zeroCopySuccessHistogram, I
am doing a BooleanHistogram there without a document, if that helps.

[commit-bot: I haz the power, 2016-10-18 11:59:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 11:59:54]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-10-18 14:01:45]

Let's address these questions in following CLs. Having a code should help our
discussions.

[drott, 2016-10-18 14:24:55]

On 2016/10/18 at 14:01:45, kojii wrote:
> Let's address these questions in following CLs. Having a code should help our
discussions.

Yes, sure. This CL is good to go from my point of view, as LGTM'ed in #98.

[kojii, 2016-10-18 15:25:07]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2016-10-18 15:25:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2016-10-18 15:30:09]

Description was changed from

==========
Enable "system-ui" generic font family

This patch enables the "system-ui" generic font family on all
platforms.

Following changes are planned but not included in this CL:
1. Content sets the localized system font to Blink on Linux/CrOS[1].
2. Remove internal use of "BlinkMacSystemFont" and add UMA[2].

[1] https://codereview.chromium.org/2138613002
[2] https://codereview.chromium.org/2388623002

BUG=654679
==========

to

==========
Enable "system-ui" generic font family

This patch enables the "system-ui" generic font family on all
platforms.

Following changes are planned but not included in this CL:
1. Content sets the current system font to Blink on Linux/CrOS[1].
2. Remove internal use of "BlinkMacSystemFont" and add UMA[2].

[1] https://codereview.chromium.org/2138613002
[2] https://codereview.chromium.org/2388623002

BUG=654679
==========

[commit-bot: I haz the power, 2016-10-18 15:30:43]

Committed patchset #23 (id:530001)

[commit-bot: I haz the power, 2016-10-18 15:32:53]

Description was changed from

==========
Enable "system-ui" generic font family

This patch enables the "system-ui" generic font family on all
platforms.

Following changes are planned but not included in this CL:
1. Content sets the current system font to Blink on Linux/CrOS[1].
2. Remove internal use of "BlinkMacSystemFont" and add UMA[2].

[1] https://codereview.chromium.org/2138613002
[2] https://codereview.chromium.org/2388623002

BUG=654679
==========

to

==========
Enable "system-ui" generic font family

This patch enables the "system-ui" generic font family on all
platforms.

Following changes are planned but not included in this CL:
1. Content sets the current system font to Blink on Linux/CrOS[1].
2. Remove internal use of "BlinkMacSystemFont" and add UMA[2].

[1] https://codereview.chromium.org/2138613002
[2] https://codereview.chromium.org/2388623002

BUG=654679

Committed: https://crrev.com/bb3db4f221492076bca16af4d6fb9f358b79cedb
Cr-Commit-Position: refs/heads/master@{#425967}
==========

[commit-bot: I haz the power, 2016-10-18 15:32:55]

Patchset 23 (id:??) landed as
https://crrev.com/bb3db4f221492076bca16af4d6fb9f358b79cedb
Cr-Commit-Position: refs/heads/master@{#425967}