Use underlining metrics from the font file
Created attachment 241645 [details] Patch
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.
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?
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
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
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
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
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.
Created attachment 241722 [details] Patch
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. :)
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.
Created attachment 241755 [details] Patch
rdar://problem/18948247
http://trac.webkit.org/changeset/176263
Re-opened since this is blocked by bug 138854
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?
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