Bug 73474 – [WinCairo] Correct SimpleFontData for Font Height

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

Several layout tests under WinCairo fail to match the core Apple results because of differences in font height.  This was tracked down to the "shouldApplyMacAscentHack()" case in the CG font handling, which was not properly replicated in the WinCairo branch.

This change brings the WinCairo font handling more closely in alignment with the Apple implementation, and allows us to produce identical results for several test cases.

[Brent Fulgham]

Created attachment 117233 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 117233 [details]
Patch

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

> Source/WebCore/platform/graphics/win/SimpleFontDataCairoWin.cpp:91
> +    if (shouldApplyMacAscentHack()) {
> +        // This code comes from FontDataMac.mm. We only ever do this when running regression tests so that our metrics will match Mac.
> +
> +        // We need to adjust Times, Helvetica, and Courier to closely match the
> +        // vertical metrics of their Microsoft counterparts that are the de facto
> +        // web standard. The AppKit adjustment of 20% is too big and is
> +        // incorrectly added to line spacing, so we use a 15% adjustment instead
> +        // and add it to the ascent.
> +        if (!wcscmp(faceName.data(), L"Times") || !wcscmp(faceName.data(), L"Helvetica") || !wcscmp(faceName.data(), L"Courier"))
> +            ascent += floorf(((ascent + descent) * 0.15f) + 0.5f);
> +    }

Maybe we should factor this out into a separate function in SimpleFontDataWin.cpp?

[Brent Fulgham]

(In reply to comment #2)
> (From update of attachment 117233 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117233&action=review
> 
> > Source/WebCore/platform/graphics/win/SimpleFontDataCairoWin.cpp:91
> > +    if (shouldApplyMacAscentHack()) {
> > +        // This code comes from FontDataMac.mm. We only ever do this when running regression tests so that our metrics will match Mac.
> > +
> > +        // We need to adjust Times, Helvetica, and Courier to closely match the
> > +        // vertical metrics of their Microsoft counterparts that are the de facto
> > +        // web standard. The AppKit adjustment of 20% is too big and is
> > +        // incorrectly added to line spacing, so we use a 15% adjustment instead
> > +        // and add it to the ascent.
> > +        if (!wcscmp(faceName.data(), L"Times") || !wcscmp(faceName.data(), L"Helvetica") || !wcscmp(faceName.data(), L"Courier"))
> > +            ascent += floorf(((ascent + descent) * 0.15f) + 0.5f);
> > +    }
> 
> Maybe we should factor this out into a separate function in SimpleFontDataWin.cpp?

We would need to pass the face name, ascent and descent values by reference so they could be modified. Would it be better to have a "modifyFontForMacAscent(fontName, ascent, descent)" call after the test, or an all-in-one method that checks for the MacAscentHack state and does the work as well?

I.e., is it better to do:
if (shouldApplyMacAscentHack())
    modifyFontForMacAscent(fontName, ascent, descent);

... or:
checkAndmodifyFontForMacAscent(fontName, ascent, descent);

Also, should we encapsulate the "m_isSystemFont = !wcscmp(faceName.data(), L"Lucida Grande");" test in a function?

[Adam Roben (:aroben)]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 117233 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=117233&action=review
> > 
> > > Source/WebCore/platform/graphics/win/SimpleFontDataCairoWin.cpp:91
> > > +    if (shouldApplyMacAscentHack()) {
> > > +        // This code comes from FontDataMac.mm. We only ever do this when running regression tests so that our metrics will match Mac.
> > > +
> > > +        // We need to adjust Times, Helvetica, and Courier to closely match the
> > > +        // vertical metrics of their Microsoft counterparts that are the de facto
> > > +        // web standard. The AppKit adjustment of 20% is too big and is
> > > +        // incorrectly added to line spacing, so we use a 15% adjustment instead
> > > +        // and add it to the ascent.
> > > +        if (!wcscmp(faceName.data(), L"Times") || !wcscmp(faceName.data(), L"Helvetica") || !wcscmp(faceName.data(), L"Courier"))
> > > +            ascent += floorf(((ascent + descent) * 0.15f) + 0.5f);
> > > +    }
> > 
> > Maybe we should factor this out into a separate function in SimpleFontDataWin.cpp?
> 
> We would need to pass the face name, ascent and descent values by reference so they could be modified. Would it be better to have a "modifyFontForMacAscent(fontName, ascent, descent)" call after the test, or an all-in-one method that checks for the MacAscentHack state and does the work as well?
> 
> I.e., is it better to do:
> if (shouldApplyMacAscentHack())
>     modifyFontForMacAscent(fontName, ascent, descent);
> 
> ... or:
> checkAndmodifyFontForMacAscent(fontName, ascent, descent);

Only the ascent gets modified. I'd do something like this:

ascent = ascentConsideringMacAscentHack(fontName.data(), ascent, descent);

> Also, should we encapsulate the "m_isSystemFont = !wcscmp(faceName.data(), L"Lucida Grande");" test in a function?

Maybe, but now we're getting down the rabbit hole of why more code isn't shared between SimpleFontDataCGWin and SimpleFontDataCairoWin. The more we can share the better, obviously.

[Brent Fulgham]

Created attachment 117254 [details]
Patch

[Brent Fulgham]

Created attachment 117257 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 117257 [details]
Patch

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

> Source/WebCore/platform/graphics/win/SimpleFontDataWin.cpp:80
> +    if (!wcscmp(faceName, L"Times") || !wcscmp(faceName, L"Helvetica") || !wcscmp(faceName, L"Courier"))
> +        ascent += floorf(((ascent + descent) * 0.15f) + 0.5f);
> +
> +    return ascent;

You could use an early return instead, something like:

if (wcscmp(...) || wcscmp(...) || wcscmp(...))
    return ascent;

return ascent + floorf(...);

[Brent Fulgham]

Landed in r101554.