New issue
Advanced search Search tips

Issue 808221 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Cleanup font configuration in blink

Project Member Reported by sergeyu@chromium.org, Feb 1 2018

Issue description

Font rendering configuration in Blink has a lot of platform-specific logic, which makes it hard to add new platforms and keep behavior consistent across different platform. There is also a lot of complexity that doesn't need to be there.
Specifically the following can be done to improve this code:

1. Move FontRenderStyle class so it can be used on platforms other than Linux & Android. It's mostly generic code that should be usable on all platforms and particularly those that that use FreeType for font rendering (e.g. Fuchsia). Pending CL also renames this class to WebFontRenderStyle, see https://chromium-review.googlesource.com/c/chromium/src/+/896839

2. Use FontRenderStyle on Fuchsia & Windows. Remove some OS_WIN ifdefs from FontCache and FontPlatformData.

3. Remove WebFontRendering class. Currently content layer calls WebFontRendering to pass system font configuration. It redirects all calls to other classes in the blink platform layer. It would be easier to the content layer to talk to blink platform directly.

4. Don't use FontCache class to store system font configuration. Currently FontCache is used on various font configuration options, which makes little sense. Move this logic somewhere else. Specifically font sizing/styling info can be stored in WebThemeEngine. Font rendering options - in FontRenderStyle.

5. Related to (4). Separate font rendering parameters (such as hinting level, subpixel rendering, etc) from system font styling configuration (default fonts size, menu font, etc.). Currently both categories are controlled via WebFontRendering interface, but they are applied at different layers. Tt doesn't make sense to mix them together. Default font sizing can be moved to blink::WebThemeEngine.

6. On Linux font rendering configuration is passed between content and blink platform in two steps: (1) renderer sets default configuration in RenderViewImpl::UpdateFontRenderingFromRendererPrefs() based on RendererPreferences, (2) blink requests rendering parameters for each font via WebSandboxSupport::GetWebFontRenderStyleForStrike() callback. There is no reason we need both. It would be better to add blink::Platform::GetWebFontRenderStyleForStrike() and use it on all platforms. Then (1) won't be needed. 
 

Comment 1 by drott@chromium.org, Feb 2 2018

Cc: kojii@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0034884cda16a0371d0f80b865278218205312ef

commit 0034884cda16a0371d0f80b865278218205312ef
Author: Sergey Ulanov <sergeyu@google.com>
Date: Wed Feb 07 21:07:35 2018

[Blink] Simplify font render style handling on Linux, Android & Fuchsia

Previously font render styler logic was mostly platform specific,
which made it hard to ensure that font rendering is consistent across
supported platforms. This CL moves code previously used on Linux and
Android, so it can be shared with Fuchsia.
 1. FontRenderStyle class out of linux directory and renames it to
    WebFontRenderStyle.
 2. Removed WebFontRendering class, which allows to remove one layer
    between content and font config stored in FontRenderStyle.
 3. Default font size now is stored in FontCache instead of
    LayoutThemeFontProvider. Mainly for consistency, since FontCache
    is responsible for handling all other font-related settings.

With these changes font rendering on Fuchsia is consistent with Linux,
which will allow to reuse layout test expectations.

In future WebFontRenderStyle can be used on Windows as well.

Bug: 778467, 808221
Change-Id: I6a7ef7e87f4c51e9d78d077c6cb796f3cbe5c8e1
Reviewed-on: https://chromium-review.googlesource.com/896839
Commit-Queue: Sergey Ulanov <sergeyu@chromium.org>
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Pavel Feldman <pfeldman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535140}
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/content/child/child_process_sandbox_support_impl_linux.cc
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/content/renderer/render_view_linux.cc
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/content/shell/test_runner/test_runner.cc
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/content/shell/test_runner/test_runner_for_specific_view.cc
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/content/zygote/zygote_main_linux.cc
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/core/css/CSSFontFaceSourceTest.cpp
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/core/layout/BUILD.gn
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/core/layout/LayoutThemeDefault.cpp
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/core/layout/LayoutThemeDefault.h
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/core/layout/LayoutThemeFontProvider.cpp
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/core/layout/LayoutThemeFontProvider.h
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/core/layout/LayoutThemeFontProviderDefault.cpp
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/core/layout/LayoutThemeFontProviderWin.cpp
[delete] https://crrev.com/c8a484b5f69327d05a95c0298b1ef2a18f192532/third_party/WebKit/Source/core/layout/WebFontRenderingAndroid.cpp
[delete] https://crrev.com/c8a484b5f69327d05a95c0298b1ef2a18f192532/third_party/WebKit/Source/core/layout/WebFontRenderingLinux.cpp
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/platform/BUILD.gn
[delete] https://crrev.com/c8a484b5f69327d05a95c0298b1ef2a18f192532/third_party/WebKit/Source/platform/exported/linux/WebFontRenderStyle.cpp
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/platform/fonts/FontCache.cpp
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/platform/fonts/FontCache.h
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/platform/fonts/FontCustomPlatformData.cpp
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/platform/fonts/FontMetrics.cpp
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/platform/fonts/FontPlatformData.cpp
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/platform/fonts/FontPlatformData.h
[add] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/platform/fonts/WebFontRenderStyle.cpp
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/platform/fonts/fuchsia/FontCacheFuchsia.cpp
[delete] https://crrev.com/c8a484b5f69327d05a95c0298b1ef2a18f192532/third_party/WebKit/Source/platform/fonts/fuchsia/FontPlatformDataFuchsia.cpp
[delete] https://crrev.com/c8a484b5f69327d05a95c0298b1ef2a18f192532/third_party/WebKit/Source/platform/fonts/linux/FontPlatformDataLinux.cpp
[delete] https://crrev.com/c8a484b5f69327d05a95c0298b1ef2a18f192532/third_party/WebKit/Source/platform/fonts/linux/FontRenderStyle.cpp
[delete] https://crrev.com/c8a484b5f69327d05a95c0298b1ef2a18f192532/third_party/WebKit/Source/platform/fonts/linux/FontRenderStyle.h
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/platform/fonts/shaping/ShapeResultBloberizerTest.cpp
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp
[modify] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/public/BUILD.gn
[add] https://crrev.com/0034884cda16a0371d0f80b865278218205312ef/third_party/WebKit/public/platform/WebFontRenderStyle.h
[delete] https://crrev.com/c8a484b5f69327d05a95c0298b1ef2a18f192532/third_party/WebKit/public/platform/linux/WebFontRenderStyle.h
[delete] https://crrev.com/c8a484b5f69327d05a95c0298b1ef2a18f192532/third_party/WebKit/public/web/linux/WebFontRendering.h

Project Member

Comment 3 by bugdroid1@chromium.org, Feb 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b4b446cf10be778d49db81a9a68bdc202c692f0b

commit b4b446cf10be778d49db81a9a68bdc202c692f0b
Author: Sergey Ulanov <sergeyu@chromium.org>
Date: Tue Feb 13 20:36:40 2018

Remove default_font_size from RendererPreferences.

default_font_size was added for Linux, but it's never set, so
we don't need it.

Bug: 808221
Change-Id: I678e65522acbdb127137b65635420b6081c9c7fb
Reviewed-on: https://chromium-review.googlesource.com/911880
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Takayoshi Kochi <kochi@chromium.org>
Reviewed-by: Dominik Röttsches <drott@chromium.org>
Commit-Queue: Sergey Ulanov <sergeyu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536455}
[modify] https://crrev.com/b4b446cf10be778d49db81a9a68bdc202c692f0b/content/common/view_messages.h
[modify] https://crrev.com/b4b446cf10be778d49db81a9a68bdc202c692f0b/content/public/common/renderer_preferences.cc
[modify] https://crrev.com/b4b446cf10be778d49db81a9a68bdc202c692f0b/content/public/common/renderer_preferences.h
[modify] https://crrev.com/b4b446cf10be778d49db81a9a68bdc202c692f0b/content/renderer/render_view_linux.cc
[modify] https://crrev.com/b4b446cf10be778d49db81a9a68bdc202c692f0b/third_party/WebKit/Source/core/layout/LayoutThemeFontProviderDefault.cpp
[modify] https://crrev.com/b4b446cf10be778d49db81a9a68bdc202c692f0b/third_party/WebKit/Source/core/layout/LayoutThemeFontProviderWin.cpp
[modify] https://crrev.com/b4b446cf10be778d49db81a9a68bdc202c692f0b/third_party/WebKit/Source/platform/fonts/FontCache.cpp
[modify] https://crrev.com/b4b446cf10be778d49db81a9a68bdc202c692f0b/third_party/WebKit/Source/platform/fonts/FontCache.h
[modify] https://crrev.com/b4b446cf10be778d49db81a9a68bdc202c692f0b/third_party/WebKit/Source/platform/fonts/WebFontRenderStyle.cpp
[modify] https://crrev.com/b4b446cf10be778d49db81a9a68bdc202c692f0b/third_party/WebKit/public/platform/WebFontRenderStyle.h

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 19

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6e10cbab14b7b8d58ecdc9d34493819888e505e3

commit 6e10cbab14b7b8d58ecdc9d34493819888e505e3
Author: Daniel Bratell <bratell@opera.com>
Date: Fri Oct 19 07:41:44 2018

Remove references from blink/public to non-existing headers

An android specific block in BUILD.gn kept some references to
files that no longer existed after a cleanup in
https://chromium-review.googlesource.com/896839

This caused some local issues with trybots so it was time to
clean up the build files. Bye unnecessary code block.

Bug: 808221
Change-Id: I29ff4fc149fe408bd2a97517fbc338b408d13e7d
Reviewed-on: https://chromium-review.googlesource.com/c/1290272
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#601073}
[modify] https://crrev.com/6e10cbab14b7b8d58ecdc9d34493819888e505e3/third_party/blink/public/BUILD.gn

Sign in to add a comment