Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(59)

Issue 2138613002: Set the font family for the "system-ui" generic font family on Linux (Closed)

Created:
4 years, 5 months ago by kojii
Modified:
4 years, 1 month ago
CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, creis+watch_chromium.org, danakj+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jam, jbroman, Justin Novosad, kinuko+watch, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Patch Set 2 : WIP #

Patch Set 3 : WIP #

Patch Set 4 : WIP #

Patch Set 5 : WIP #

Patch Set 6 : WIP #

Patch Set 7 : #

Patch Set 8 : test failures for cros #

Patch Set 9 : WIP #

Patch Set 10 : WIP #

Patch Set 11 : WIP #

Total comments: 1

Patch Set 12 : Cleanup, change Blink first #

Patch Set 13 : Rebase after clang-format #

Patch Set 14 : Rebase #

Patch Set 15 : Skip setting if empty() #

Patch Set 16 : Switch to gfx::Font().GetFontName() #

Patch Set 17 : Rebase #

Patch Set 18 : Move to render_view_host_impl.cc #

Patch Set 19 : Windows fix #

Patch Set 20 : Avoid #if in Source/web #

Total comments: 2

Patch Set 21 : Changed char* to WebString #

Total comments: 3

Patch Set 22 : jochen review nit (add braces) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -14 lines) Patch
M content/browser/renderer_host/render_view_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 4 chunks +6 lines, -8 lines 0 comments Download
M content/common/view_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +3 lines, -0 lines 0 comments Download
M content/public/common/renderer_preferences.h View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download
M content/renderer/render_view_linux.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/fonts/FontCache.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/fonts/android/FontCacheAndroid.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/web/linux/WebFontRendering.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/linux/WebFontRendering.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 92 (74 generated)
drott
https://codereview.chromium.org/2138613002/diff/200001/third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right): https://codereview.chromium.org/2138613002/diff/200001/third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm#newcode62 third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm:62: DEFINE_STATIC_LOCAL(AtomicString, systemFontFamilyName, ("system-ui")); Does this work on all OS ...
4 years, 2 months ago (2016-09-29 13:48:54 UTC) #21
kojii
On 2016/09/29 at 13:48:54, drott wrote: > https://codereview.chromium.org/2138613002/diff/200001/third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm > File third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm (right): > > https://codereview.chromium.org/2138613002/diff/200001/third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm#newcode62 ...
4 years, 2 months ago (2016-09-29 15:06:20 UTC) #22
kojii
drott@: content_shell can see the correct font by moving the set-code from renderer_preferences_util.cc to render_view_host_impl.cc ...
4 years, 2 months ago (2016-10-19 07:03:22 UTC) #57
kojii
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
4 years, 2 months ago (2016-10-20 09:08:59 UTC) #67
dcheng
ipc lgtm https://codereview.chromium.org/2138613002/diff/380001/third_party/WebKit/public/web/linux/WebFontRendering.h File third_party/WebKit/public/web/linux/WebFontRendering.h (right): https://codereview.chromium.org/2138613002/diff/380001/third_party/WebKit/public/web/linux/WebFontRendering.h#newcode52 third_party/WebKit/public/web/linux/WebFontRendering.h:52: BLINK_EXPORT static void setSystemFontFamily(const char*); Out of ...
4 years, 2 months ago (2016-10-20 09:16:03 UTC) #68
kojii
https://codereview.chromium.org/2138613002/diff/380001/third_party/WebKit/public/web/linux/WebFontRendering.h File third_party/WebKit/public/web/linux/WebFontRendering.h (right): https://codereview.chromium.org/2138613002/diff/380001/third_party/WebKit/public/web/linux/WebFontRendering.h#newcode52 third_party/WebKit/public/web/linux/WebFontRendering.h:52: BLINK_EXPORT static void setSystemFontFamily(const char*); On 2016/10/20 at 09:16:03, ...
4 years, 2 months ago (2016-10-20 11:08:13 UTC) #71
drott
https://codereview.chromium.org/2138613002/diff/400001/third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt 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/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt#newcode3 third_party/WebKit/LayoutTests/platform/linux/inspector-protocol/layout-fonts/generic-system-ui-expected.txt:3: "Arial" : 37 Could you explain why this ends ...
4 years, 2 months ago (2016-10-20 15:18:40 UTC) #74
drott
So, LGTM with the above request of adding an explanation for the layout test result. ...
4 years, 2 months ago (2016-10-20 15:20:31 UTC) #75
kojii
setSystemFontFamily is set in content_shell and in layout tests by moving the code to render_view_host_impl.cpp, ...
4 years, 2 months ago (2016-10-21 03:25:45 UTC) #77
drott
On 2016/10/21 at 03:25:45, kojii wrote: > setSystemFontFamily is set in content_shell and in layout ...
4 years, 2 months ago (2016-10-21 13:42:48 UTC) #78
kojii
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
4 years, 2 months ago (2016-10-21 17:00:16 UTC) #80
jochen (gone - plz use gerrit)
lgtm with nit https://codereview.chromium.org/2138613002/diff/400001/content/renderer/render_view_linux.cc File content/renderer/render_view_linux.cc (right): https://codereview.chromium.org/2138613002/diff/400001/content/renderer/render_view_linux.cc#newcode73 content/renderer/render_view_linux.cc:73: if (!prefs.system_font_family_name.empty()) please add { } ...
4 years, 1 month ago (2016-10-24 07:24:26 UTC) #81
kojii
https://codereview.chromium.org/2138613002/diff/400001/content/renderer/render_view_linux.cc File content/renderer/render_view_linux.cc (right): https://codereview.chromium.org/2138613002/diff/400001/content/renderer/render_view_linux.cc#newcode73 content/renderer/render_view_linux.cc:73: if (!prefs.system_font_family_name.empty()) On 2016/10/24 at 07:24:26, jochen wrote: > ...
4 years, 1 month ago (2016-10-24 07:41:07 UTC) #83
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2138613002/420001
4 years, 1 month ago (2016-10-24 07:41:19 UTC) #85
commit-bot: I haz the power
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_chromeos_rel_ng/builds/301180)
4 years, 1 month ago (2016-10-24 10:03:33 UTC) #87
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2138613002/420001
4 years, 1 month ago (2016-10-25 02:39:58 UTC) #89
commit-bot: I haz the power
Committed patchset #22 (id:420001)
4 years, 1 month ago (2016-10-25 03:21:12 UTC) #90
commit-bot: I haz the power
4 years, 1 month ago (2016-10-25 03:26:29 UTC) #92
Message was sent while issue was closed.
Patchset 22 (id:??) landed as
https://crrev.com/85fa241e3aa27a6c0c5ecb9c9ca4daa56ea4ac14
Cr-Commit-Position: refs/heads/master@{#427262}

Powered by Google App Engine
This is Rietveld 408576698