Bug 138762 – Use underlining metrics from the font file

• Status: REOPENED
• Component: New Bugs

[Myles C. Maxfield]

Use underlining metrics from the font file

[Myles C. Maxfield]

Created attachment 241645 [details]
Patch

[WebKit Commit Bot]

Attachment 241645 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jon Lee]

Comment on attachment 241645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241645&action=review

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:69
> +    float underlinePosition = std::max(1.0f, ceilf(decorationThickness / 2.0));

This calculation reoccurs per port. Can we consolidate this?

[Build Bot]

Comment on attachment 241645 [details]
Patch

Attachment 241645 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4829837717929984

New failing tests:
fast/css3-text/css3-text-decoration/text-decoration-thickness.html

[Build Bot]

Created attachment 241651 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 241645 [details]
Patch

Attachment 241645 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6706599913586688

New failing tests:
fast/css3-text/css3-text-decoration/text-decoration-thickness.html

[Build Bot]

Created attachment 241655 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Myles C. Maxfield]

Comment on attachment 241645 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241645&action=review

>> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:69
>> +    float underlinePosition = std::max(1.0f, ceilf(decorationThickness / 2.0));
> 
> This calculation reoccurs per port. Can we consolidate this?

I actually can't because the font size is considered platform-dependent (and the size is an input to these calculations). "Font"s have sizes because of the font-size CSS property, and "PlatformFontData"s have sizes because it's dealing with the platform directly, but SimpleFontData (nor its constituent FontMetrics) knows anything about sizes.

[Myles C. Maxfield]

Created attachment 241722 [details]
Patch

[Dave Hyatt]

Comment on attachment 241722 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241722&action=review

r=me

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:69
> +    float decorationThickness = m_platformData.m_size / 16.0f;
> +    float underlinePosition = std::max(1.0f, ceilf(decorationThickness / 2.0));

Comments might be nice here explaining what you're doing.

> Source/WebCore/platform/graphics/ios/SimpleFontDataIOS.mm:109
> +        decorationThickness = m_platformData.size() / 16.0f;
> +        underlinePosition = std::max(1.0f, ceilf(decorationThickness / 2.0));

Ditto.

> Source/WebCore/platform/graphics/win/SimpleFontDataCGWin.cpp:73
> +    float decorationThickness = m_platformData.size() / 16.0f;
> +    float underlinePosition = std::max(1.0f, ceilf(decorationThickness / 2.0));

And again!

> Source/WebCore/svg/SVGFontData.cpp:83
> +    float decorationThickness = fontSize / 16.0f;
> +    float underlinePosition = std::max(1.0f, ceilf(decorationThickness / 2.0));

This is coming up enough that maybe you want a helper somewhere that takes two float references and sets their values. That way if we ever decide to change behavior, you'd only have to patch one spot. :)

[Myles C. Maxfield]

Comment on attachment 241722 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=241722&action=review

>> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:69
>> +    float underlinePosition = std::max(1.0f, ceilf(decorationThickness / 2.0));
> 
> Comments might be nice here explaining what you're doing.

Done.

>> Source/WebCore/platform/graphics/ios/SimpleFontDataIOS.mm:109
>> +        underlinePosition = std::max(1.0f, ceilf(decorationThickness / 2.0));
> 
> Ditto.

Done.

>> Source/WebCore/platform/graphics/win/SimpleFontDataCGWin.cpp:73
>> +    float underlinePosition = std::max(1.0f, ceilf(decorationThickness / 2.0));
> 
> And again!

Done.

>> Source/WebCore/svg/SVGFontData.cpp:83
>> +    float underlinePosition = std::max(1.0f, ceilf(decorationThickness / 2.0));
> 
> This is coming up enough that maybe you want a helper somewhere that takes two float references and sets their values. That way if we ever decide to change behavior, you'd only have to patch one spot. :)

Done.

[Myles C. Maxfield]

Created attachment 241755 [details]
Patch

[Jon Lee]

rdar://problem/18948247

[Myles C. Maxfield]

http://trac.webkit.org/changeset/176263

[WebKit Commit Bot]

Re-opened since this is blocked by bug 138854

[Sam Weinig]

It seems the patch was rolled out due to "underlines looking hideous". What exactly was hideous about them? Do we want to abandon the idea of getting the metrics from fonts altogether (and close this bug), or does the patch just need fixing?

[Myles C. Maxfield]

This patch needs investigation. I think we are moving the underline after we place it according to the font file, moving it up too high, so it was intersecting each letter, causing the skipping logic to be triggered, meaning underlines turned into periods between each glyph