Set the font family for the "system-ui" generic font family on Linux

Set the font family for the "system-ui" generic font family on Linux

This patch sets the font family for the "system-ui" generic font family
added in [1] on Linux.

With this change, all platforms use the correct font. On Windows, the
browser already pass menu_font_family_name to the renderer. On Mac and
Android, the render can find the system font without needing the
browser to set.

On Linux and Chrome OS, gfx::Font() calls LinuxFontDelegate to figure
out the current system font. On the default Ubuntu, this resolves to
"sans", which Skia resolves to "DejaVu Sans", or "Arial" in
run-layout-test.

[1] http://crrev.com/2137483004

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

[kojii, 2016-09-21 07:36:17]

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

[commit-bot: I haz the power, 2016-09-21 07:36:30]

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-21 07:38:08]

The CQ bit was unchecked by kojii@chromium.org

[kojii, 2016-09-28 02:33:24]

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

[commit-bot: I haz the power, 2016-09-28 02:33:35]

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-28 03:41:03]

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

[commit-bot: I haz the power, 2016-09-28 03:41:04]

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

[kojii, 2016-09-28 07:31:54]

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

[commit-bot: I haz the power, 2016-09-28 07:32:05]

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-28 07:46:48]

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

[commit-bot: I haz the power, 2016-09-28 07:46:49]

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

[kojii, 2016-09-28 11:00:38]

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

[commit-bot: I haz the power, 2016-09-28 11:00:56]

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-28 11:13:11]

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

[commit-bot: I haz the power, 2016-09-28 11:13:12]

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-...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kojii, 2016-09-29 05:21:17]

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

[commit-bot: I haz the power, 2016-09-29 05:21:29]

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-29 06:22:32]

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

[commit-bot: I haz the power, 2016-09-29 06:22:33]

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

[drott, 2016-09-29 13:48:53]

drott@chromium.org changed reviewers:
+ drott@chromium.org

[drott, 2016-09-29 13:48:54]

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

https://codereview.chromium.org/2138613002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:62:
DEFINE_STATIC_LOCAL(AtomicString, systemFontFamilyName, ("system-ui"));
Does this work on all OS X versions?

[kojii, 2016-09-29 15:06:20]

On 2016/09/29 at 13:48:54, drott wrote:
>
https://codereview.chromium.org/2138613002/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right):
> 
>
https://codereview.chromium.org/2138613002/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:62:
DEFINE_STATIC_LOCAL(AtomicString, systemFontFamilyName, ("system-ui"));
> Does this work on all OS X versions?

Not at this point ;-P Need to change FontMatcherMac.mm to support this constant.

[kojii, 2016-09-30 16:38:35]

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:38:53]

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 18:09:57]

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

[commit-bot: I haz the power, 2016-09-30 18:09:58]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kojii, 2016-10-03 03:49:14]

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:49:18]

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 05:18:23]

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

[commit-bot: I haz the power, 2016-10-03 05:18:24]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-10-12 06:26:55]

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

[commit-bot: I haz the power, 2016-10-12 06:27:09]

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-12 09:53:08]

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

[commit-bot: I haz the power, 2016-10-12 09:53:09]

Dry run: Exceeded global retry quota

[kojii, 2016-10-13 10:55:23]

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

[commit-bot: I haz the power, 2016-10-13 10:55:40]

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-13 12:25:39]

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

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

Dry run: This issue passed the CQ dry run.

[kojii, 2016-10-18 06:45:55]

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

[commit-bot: I haz the power, 2016-10-18 06:46:08]

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-18 08:45:33]

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

[commit-bot: I haz the power, 2016-10-18 08:45:34]

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

[kojii, 2016-10-18 16:45:24]

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

[commit-bot: I haz the power, 2016-10-18 16:45:39]

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 17:18:13]

Description was changed from

==========
WIP: system-font

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

to

==========
Set the font family for the "system-ui" generic font family on Linux

This patch sets the font family for the "system-ui" generic font family
added in [1] on Linux.

All platforms are supported With this change. On Windows, the browser
already pass menu_font_family_name to the renderer. On Mac and Android,
the render can find the system font.

[1] http://crrev.com/2137483004

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

[commit-bot: I haz the power, 2016-10-18 17:43:26]

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

[commit-bot: I haz the power, 2016-10-18 17:43:27]

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

[kojii, 2016-10-19 01:41:52]

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

[commit-bot: I haz the power, 2016-10-19 01:42: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-19 01:43:30]

Description was changed from

==========
Set the font family for the "system-ui" generic font family on Linux

This patch sets the font family for the "system-ui" generic font family
added in [1] on Linux.

All platforms are supported With this change. On Windows, the browser
already pass menu_font_family_name to the renderer. On Mac and Android,
the render can find the system font.

[1] http://crrev.com/2137483004

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

to

==========
Set the font family for the "system-ui" generic font family on Linux

This patch sets the font family for the "system-ui" generic font family
added in [1] on Linux.

All platforms are supported With this change. On Windows, the browser
already pass menu_font_family_name to the renderer. On Mac and Android,
the render can find the system font without needing the browser to set.

[1] http://crrev.com/2137483004

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

[commit-bot: I haz the power, 2016-10-19 02:38:52]

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

[commit-bot: I haz the power, 2016-10-19 02:38:53]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[kojii, 2016-10-19 06:28:59]

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

[kojii, 2016-10-19 06:57:26]

Description was changed from

==========
Set the font family for the "system-ui" generic font family on Linux

This patch sets the font family for the "system-ui" generic font family
added in [1] on Linux.

All platforms are supported With this change. On Windows, the browser
already pass menu_font_family_name to the renderer. On Mac and Android,
the render can find the system font without needing the browser to set.

[1] http://crrev.com/2137483004

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

to

==========
Set the font family for the "system-ui" generic font family on Linux

This patch sets the font family for the "system-ui" generic font family
added in [1] on Linux.

All platforms use the correctc font with this change. On Windows, the
browser already pass menu_font_family_name to the renderer. On Mac and
Android, the render can find the system font without needing the
browser to set.

[1] http://crrev.com/2137483004

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

[kojii, 2016-10-19 06:58:28]

The CQ bit was unchecked by kojii@chromium.org

[kojii, 2016-10-19 06:58:34]

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

[kojii, 2016-10-19 07:03:22]

drott@: content_shell can see the correct font by moving the set-code from
renderer_preferences_util.cc to render_view_host_impl.cc in PS18, so
"-expected.txt" was updated.

[commit-bot: I haz the power, 2016-10-19 09:30:15]

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-19 12:18:44]

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

[commit-bot: I haz the power, 2016-10-19 12:18:45]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-10-19 12:56:59]

Description was changed from

==========
Set the font family for the "system-ui" generic font family on Linux

This patch sets the font family for the "system-ui" generic font family
added in [1] on Linux.

All platforms use the correctc font with this change. On Windows, the
browser already pass menu_font_family_name to the renderer. On Mac and
Android, the render can find the system font without needing the
browser to set.

[1] http://crrev.com/2137483004

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

to

==========
Set the font family for the "system-ui" generic font family on Linux

This patch sets the font family for the "system-ui" generic font family
added in [1] on Linux.

With this change, all platforms use the correct font. On Windows, the
browser already pass menu_font_family_name to the renderer. On Mac and
Android, the render can find the system font without needing the
browser to set.

[1] http://crrev.com/2137483004

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

[kojii, 2016-10-20 06:58:01]

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

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

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-20 08:47:17]

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

[commit-bot: I haz the power, 2016-10-20 08:47:18]

Dry run: This issue passed the CQ dry run.

[kojii, 2016-10-20 09:08:58]

kojii@chromium.org changed reviewers:
+ dcheng@chromium.org, esprehn@chromium.org

[kojii, 2016-10-20 09:08:59]

PTAL.

dcheng@:  content/common/view_messages.h
          content/public/common/renderer_preferences.h
esprehn@: all the rest (content/browser, content/renderer, third_party/WebKit)
drott@:   all

[dcheng, 2016-10-20 09:16:03]

ipc lgtm

https://codereview.chromium.org/2138613002/diff/380001/third_party/WebKit/pub...
File third_party/WebKit/public/web/linux/WebFontRendering.h (right):

https://codereview.chromium.org/2138613002/diff/380001/third_party/WebKit/pub...
third_party/WebKit/public/web/linux/WebFontRendering.h:52: BLINK_EXPORT static
void setSystemFontFamily(const char*);
Out of curiosity, why const char* instead of something like WebCString? const
char* in the public API is pretty rare.

[kojii, 2016-10-20 11:06:02]

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

[commit-bot: I haz the power, 2016-10-20 11:06: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-20 11:08:13]

https://codereview.chromium.org/2138613002/diff/380001/third_party/WebKit/pub...
File third_party/WebKit/public/web/linux/WebFontRendering.h (right):

https://codereview.chromium.org/2138613002/diff/380001/third_party/WebKit/pub...
third_party/WebKit/public/web/linux/WebFontRendering.h:52: BLINK_EXPORT static
void setSystemFontFamily(const char*);
On 2016/10/20 at 09:16:03, dcheng wrote:
> Out of curiosity, why const char* instead of something like WebCString? const
char* in the public API is pretty rare.

Followed too old example...fixed to WebString in PS21. Thank you for catching
this.

[commit-bot: I haz the power, 2016-10-20 12:18:48]

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

[commit-bot: I haz the power, 2016-10-20 12:18:49]

Dry run: This issue passed the CQ dry run.

[drott, 2016-10-20 15:18:40]

https://codereview.chromium.org/2138613002/diff/400001/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/2138613002/diff/400001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt:3:
"Arial" : 37
Could you explain why this ends up being Arial? Is this because
setSystemFontFamily is not set for layout tests, and then we do the empty
SkTypeface fallback (which I thought should give us a font that fontconfig
mapped to "Sans", or if it doesn't find Sans, then Arial as secondary fallback,
see FontCache::getLastResortFallbackFont). Or does this have a different reason,
like does this come from gfx::Font()?

Could you add such an explanation to the CL description?

[drott, 2016-10-20 15:20:31]

So, LGTM with the above request of adding an explanation for the layout test
result. Also, I assume gfx::Font() works on CrOS as well? Perhaps it's worth
mentioning this platform explicitly in the CL description.

[kojii, 2016-10-21 03:23:05]

Description was changed from

==========
Set the font family for the "system-ui" generic font family on Linux

This patch sets the font family for the "system-ui" generic font family
added in [1] on Linux.

With this change, all platforms use the correct font. On Windows, the
browser already pass menu_font_family_name to the renderer. On Mac and
Android, the render can find the system font without needing the
browser to set.

[1] http://crrev.com/2137483004

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

to

==========
Set the font family for the "system-ui" generic font family on Linux

This patch sets the font family for the "system-ui" generic font family
added in [1] on Linux.

With this change, all platforms use the correct font. On Windows, the
browser already pass menu_font_family_name to the renderer. On Mac and
Android, the render can find the system font without needing the
browser to set.

On Linux and Chrome OS, gfx::Font() calls LinuxFontDelegate to figure
out the current system font. On the default Ubuntu, this resolves to
"sans", which Skia resolves to "DejaVu Sans", or "Arial" in
run-layout-test.

[1] http://crrev.com/2137483004

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

[kojii, 2016-10-21 03:25:45]

setSystemFontFamily is set in content_shell and in layout tests by moving the
code to render_view_host_impl.cpp, but Skia resolves "sans" differently when in
run-layout-test. Updated the description, and added CrOS too.

[drott, 2016-10-21 13:42:48]

On 2016/10/21 at 03:25:45, kojii wrote:
> setSystemFontFamily is set in content_shell and in layout tests by moving the
code to render_view_host_impl.cpp, but Skia resolves "sans" differently when in
run-layout-test. Updated the description, and added CrOS too.

Great, thanks. Nice CL.

[kojii, 2016-10-21 17:00:14]

kojii@chromium.org changed reviewers:
+ jochen@chromium.org
- esprehn@chromium.org

[kojii, 2016-10-21 17:00:16]

jochen@, PTAL at following files:
    content/browser/renderer_host/render_view_host_impl.cc
    content/public/common/renderer_preferences.h
    content/renderer/render_view_linux.cc

[jochen (gone - plz use gerrit), 2016-10-24 07:24:26]

lgtm with nit

https://codereview.chromium.org/2138613002/diff/400001/content/renderer/rende...
File content/renderer/render_view_linux.cc (right):

https://codereview.chromium.org/2138613002/diff/400001/content/renderer/rende...
content/renderer/render_view_linux.cc:73: if
(!prefs.system_font_family_name.empty())
please add { } around the if body

[kojii, 2016-10-24 07:41:05]

The CQ bit was checked by kojii@chromium.org

[kojii, 2016-10-24 07:41:07]

https://codereview.chromium.org/2138613002/diff/400001/content/renderer/rende...
File content/renderer/render_view_linux.cc (right):

https://codereview.chromium.org/2138613002/diff/400001/content/renderer/rende...
content/renderer/render_view_linux.cc:73: if
(!prefs.system_font_family_name.empty())
On 2016/10/24 at 07:24:26, jochen wrote:
> please add { } around the if body

Done, missed this, thank you.

[kojii, 2016-10-24 07:41:07]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
drott@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2138613002/#ps420001
(title: "jochen review nit (add braces)")

[commit-bot: I haz the power, 2016-10-24 07:41:19]

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-24 10:03:31]

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

[commit-bot: I haz the power, 2016-10-24 10:03:33]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kojii, 2016-10-25 02:39:28]

The CQ bit was checked by kojii@chromium.org

[commit-bot: I haz the power, 2016-10-25 02:39:58]

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-25 03:21:12]

Committed patchset #22 (id:420001)

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

Description was changed from

==========
Set the font family for the "system-ui" generic font family on Linux

This patch sets the font family for the "system-ui" generic font family
added in [1] on Linux.

With this change, all platforms use the correct font. On Windows, the
browser already pass menu_font_family_name to the renderer. On Mac and
Android, the render can find the system font without needing the
browser to set.

On Linux and Chrome OS, gfx::Font() calls LinuxFontDelegate to figure
out the current system font. On the default Ubuntu, this resolves to
"sans", which Skia resolves to "DejaVu Sans", or "Arial" in
run-layout-test.

[1] http://crrev.com/2137483004

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

to

==========
Set the font family for the "system-ui" generic font family on Linux

This patch sets the font family for the "system-ui" generic font family
added in [1] on Linux.

With this change, all platforms use the correct font. On Windows, the
browser already pass menu_font_family_name to the renderer. On Mac and
Android, the render can find the system font without needing the
browser to set.

On Linux and Chrome OS, gfx::Font() calls LinuxFontDelegate to figure
out the current system font. On the default Ubuntu, this resolves to
"sans", which Skia resolves to "DejaVu Sans", or "Arial" in
run-layout-test.

[1] http://crrev.com/2137483004

BUG=654679

Committed: https://crrev.com/85fa241e3aa27a6c0c5ecb9c9ca4daa56ea4ac14
Cr-Commit-Position: refs/heads/master@{#427262}
==========

[commit-bot: I haz the power, 2016-10-25 03:26:29]

Patchset 22 (id:??) landed as
https://crrev.com/85fa241e3aa27a6c0c5ecb9c9ca4daa56ea4ac14
Cr-Commit-Position: refs/heads/master@{#427262}