Commit d3045ebd authored by Etienne Bergeron's avatar Etienne Bergeron Committed by Commit Bot

Fix font-height off-by one due to rounding

This CL is fixes the unit tests for the PlatformFontSkiaOnWindows experiment.
  1) It fixes the off-by one due to rounding (e.g. font height computation).
  2) It uses the DirectWrite Skia font manager by default; otherwise the default
     is the GDI font manager which is not giving the appropriate metrics.

1) Off-by-one due to rounding:

PlatformFontWin is computing the font metrics with Skia in the
function PlatformFontWin::CreateHFontRefFromSkia.

See: https://cs.chromium.org/chromium/src/ui/gfx/platform_font_win.cc?l=515

    SkFontMetrics skia_metrics;
    font.getMetrics(&skia_metrics);
    const int height = std::ceil(skia_metrics.fDescent - skia_metrics.fAscent);

PlatformFontSkia is computing it this way (ComputeMetricsIfNecessary):

See: https://cs.chromium.org/chromium/src/ui/gfx/platform_font_skia.cc?l=342

    SkFontMetrics skia_metrics;
    font.getMetrics(&skia_metrics);
    ascent_pixels_ = SkScalarCeilToInt(-metrics.fAscent);
    height_pixels_ = ascent_pixels_ + SkScalarCeilToInt(metrics.fDescent);

The height computation is also not consistent with the way it's computed on Mac(platform_font_mac).

See: https://cs.chromium.org/chromium/src/ui/gfx/platform_font_mac.mm?q=platformfont+mac&dr=C&l=355

     height_ = ceil(ascent_ + std::abs([font descender]) + [font leading]);

This difference is making the font height on linux/windows off by one pixel.
Unittests were adjusted for these off-by-one on each platform.

The migration of PlatformFontWin to PlatformFontSkia is making these tests
to fail. Ideally, we should choose one implementation and fix the code.
To ease the migration, this CL implements on Windows the same computation
used in PlatformFontWin to minimize the visible changes in Chrome.

2) DirectWrite Font Manager

The unittests are not calling InitializeDirectWrite(..) to force an override of the
Skia font manager. The browser code currently fails if the font manager is not set to
DirectWrite. Unfortunately, the unittests are not calling the initialization function
and were using by default the GDI adapter.

The simple way to fix this issue is to ensures that Chrome code (browser and tests) are
only using the DirectWrite version. This is fine since WinXP is no longer maintained
and DirectWrite is always available on Win7+.

Bug: 944227
R=robliao@chromium.org, asvitkine@chromium.org

Change-Id: Ide54b96d576c5e4ec862dabb5aef6c735a3ffdc5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1721453
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: default avatarRobert Liao <robliao@chromium.org>
Reviewed-by: default avatarAlexei Svitkine <asvitkine@chromium.org>
Reviewed-by: default avatarScott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688715}
parent 882891f7
...@@ -10,7 +10,7 @@ ...@@ -10,7 +10,7 @@
namespace skia { namespace skia {
SK_API sk_sp<SkFontMgr> CreateDefaultSkFontMgr() { SK_API sk_sp<SkFontMgr> CreateDefaultSkFontMgr() {
return SkFontMgr_New_GDI(); return SkFontMgr_New_DirectWrite();
} }
} // namespace skia } // namespace skia
\ No newline at end of file
...@@ -363,9 +363,21 @@ void PlatformFontSkia::ComputeMetricsIfNecessary() { ...@@ -363,9 +363,21 @@ void PlatformFontSkia::ComputeMetricsIfNecessary() {
SkFontMetrics metrics; SkFontMetrics metrics;
font.getMetrics(&metrics); font.getMetrics(&metrics);
ascent_pixels_ = SkScalarCeilToInt(-metrics.fAscent); ascent_pixels_ = SkScalarCeilToInt(-metrics.fAscent);
height_pixels_ = ascent_pixels_ + SkScalarCeilToInt(metrics.fDescent);
cap_height_pixels_ = SkScalarCeilToInt(metrics.fCapHeight); cap_height_pixels_ = SkScalarCeilToInt(metrics.fCapHeight);
// There is a mismatch between the way the PlatformFontWin was computing the
// font height in pixel. The font height may vary by one pixel due to
// decimal rounding.
// Windows Skia implements : ceil(descent - ascent)
// Linux Skia implements : ceil(-ascent) + ceil(descent)
// TODO(etienneb): Make both implementation consistent and fix the broken
// unittests.
#if defined(OS_WIN)
height_pixels_ = SkScalarCeilToInt(metrics.fDescent - metrics.fAscent);
#else
height_pixels_ = ascent_pixels_ + SkScalarCeilToInt(metrics.fDescent);
#endif
if (metrics.fAvgCharWidth) { if (metrics.fAvgCharWidth) {
average_width_pixels_ = SkScalarToDouble(metrics.fAvgCharWidth); average_width_pixels_ = SkScalarToDouble(metrics.fAvgCharWidth);
} else { } else {
......
...@@ -87,8 +87,10 @@ void InitializeDirectWrite() { ...@@ -87,8 +87,10 @@ void InitializeDirectWrite() {
base::UmaHistogramSparse("DirectWrite.Fonts.Gfx.InitializeLoopCount", base::UmaHistogramSparse("DirectWrite.Fonts.Gfx.InitializeLoopCount",
iteration); iteration);
// TODO(crbug.com/956064): Move to a CHECK when the cause of the crash is // TODO(crbug.com/956064): Move to a CHECK when the cause of the crash is
// fixed. // fixed and remove the if statement that fallback to GDI font manager.
DCHECK(!!direct_write_font_mgr); DCHECK(!!direct_write_font_mgr);
if (!direct_write_font_mgr)
direct_write_font_mgr = SkFontMgr_New_GDI();
// Override the default skia font manager. This must be called before any // Override the default skia font manager. This must be called before any
// use of the skia font manager is done (e.g. before any call to // use of the skia font manager is done (e.g. before any call to
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment