Bug 73474 - [WinCairo] Correct SimpleFontData for Font Height
Summary: [WinCairo] Correct SimpleFontData for Font Height
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-30 11:14 PST by Brent Fulgham
Modified: 2011-11-30 14:02 PST (History)
1 user (show)

See Also:


Attachments
Patch (5.74 KB, patch)
2011-11-30 11:22 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2011-11-30 12:48 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (7.87 KB, patch)
2011-11-30 12:58 PST, Brent Fulgham
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2011-11-30 11:14:06 PST
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.
Comment 1 Brent Fulgham 2011-11-30 11:22:10 PST
Created attachment 117233 [details]
Patch
Comment 2 Adam Roben (:aroben) 2011-11-30 11:26:38 PST
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?
Comment 3 Brent Fulgham 2011-11-30 11:34:13 PST
(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?
Comment 4 Adam Roben (:aroben) 2011-11-30 11:37:36 PST
(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.
Comment 5 Brent Fulgham 2011-11-30 12:48:57 PST
Created attachment 117254 [details]
Patch
Comment 6 Brent Fulgham 2011-11-30 12:58:03 PST
Created attachment 117257 [details]
Patch
Comment 7 Adam Roben (:aroben) 2011-11-30 13:04:51 PST
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(...);
Comment 8 Brent Fulgham 2011-11-30 14:02:18 PST
Landed in r101554.