Bug 150464 – [WinCairo] Improve test results for fast layouttests.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

There are many failures on the WinCairo fast layout tests. Many of these seems to be caused by differences in font metrics.
WinCairo is currently applying the Mac ascent hack. It seems this should only be used for platforms using CoreGraphics.
Also, a device scale factor different from 1.0, is causing differences from the expected result.

Created attachment 263848 [details]
Patch

This patch does not fix many tests (if any), but it brings the result closer to the expected result. The font metrics are not in line with the expected results yet, but they are closer :)

[Csaba Osztrogonác]

run-webkit-tests doesn't know --wincairo platform at all.

Do you plan to fix it and make the WinCairo bot run tests?

(In reply to comment #3)
> run-webkit-tests doesn't know --wincairo platform at all.
> 
> Do you plan to fix it and make the WinCairo bot run tests?

It would be very nice to get this fixed, I will start with looking into the font metrics, and see how far it gets us :)

[Alex Christensen]

Comment on attachment 263848 [details]
Patch

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

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:1237
> +#if !USE(CAIRO)
>      viewPrivate->setShouldApplyMacFontAscentHack(TRUE);
> +#endif

This seems good, or maybe this should be inside the definition of setShouldApplyMacFontAscentHack

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:1241
> +#if USE(CAIRO)
> +    viewPrivate->setCustomBackingScaleFactor(1.0);
> +#endif

Why?

Created attachment 263918 [details]
Patch

(In reply to comment #5)
> Comment on attachment 263848 [details]
> Patch
>

Thanks for reviewing :)
 
> 
> > Tools/DumpRenderTree/win/DumpRenderTree.cpp:1241
> > +#if USE(CAIRO)
> > +    viewPrivate->setCustomBackingScaleFactor(1.0);
> > +#endif
> 
> Why?

The render tree text output is in logical coordinates.
If the logical coordinate space is different from the pixel coordinate space, we will not get the expected output.
Alternatively, we could also scale the size of the web view with the device scale factor.
It seems Mac is doing the same thing.

[Alex Christensen]

Comment on attachment 263918 [details]
Patch

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

> Tools/DumpRenderTree/win/DumpRenderTree.cpp:1239
> +#if USE(CAIRO)
> +    viewPrivate->setCustomBackingScaleFactor(1.0);
> +#endif

Now that you mention it, I've run into this problem on the AppleWin port, too, when trying to test on high dpi screens.  This should be done for all Windows testing.  Please remove the #if USE(CAIRO)

[Alex Christensen]

Comment on attachment 263918 [details]
Patch

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

> Source/WebCore/platform/graphics/win/SimpleFontDataCairoWin.cpp:-84
> -    ascent = ascentConsideringMacAscentHack(faceName.data(), ascent, descent);

On second thought, removing this would make the Windows ports more different, which would make them harder to test, wouldn't it?

(In reply to comment #9)
> Comment on attachment 263918 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263918&action=review
> 
> > Source/WebCore/platform/graphics/win/SimpleFontDataCairoWin.cpp:-84
> > -    ascent = ascentConsideringMacAscentHack(faceName.data(), ascent, descent);
> 
> On second thought, removing this would make the Windows ports more
> different, which would make them harder to test, wouldn't it?

Sorry, I don't quite understand what you mean :) This is only for WinCairo, and should make the font metrics less different than before. I think we should only apply the Mac ascent adjustment for CoreGraphics platforms.

[Alex Christensen]

Doesn't applying the mac ascent adjustment on WinCairo make WinCairo layout text more similar to the AppleWin port?  Ideally the two ports would lay everything out the same.

(In reply to comment #11)
> Doesn't applying the mac ascent adjustment on WinCairo make WinCairo layout
> text more similar to the AppleWin port?  Ideally the two ports would lay
> everything out the same.

I believe it makes it more different :) As I have understood it, the Mac adjustment hack is applied to make the Mac font metrics more similar to certain Windows font metrics (Times, for example), because some Windows font metrics had become a de facto standard on the web. When getting font metrics on Windows with the native API, we already have the "correct" metrics, and there is no need to apply the Mac ascent adjustment, except for AppleWin, which is using CoreGraphics, and thereby behaving in the same way as Mac. Debugging also shows that if we apply the ascent adjustment, the layout elements are too big compared to what is expected.

[Alex Christensen]

Comment on attachment 263918 [details]
Patch

Sounds good.  Commit it without the #if USE(CAIRO) in DumpRenderTree.cpp.

(In reply to comment #13)
> Comment on attachment 263918 [details]
> Patch
> 
> Sounds good.  Commit it without the #if USE(CAIRO) in DumpRenderTree.cpp.

Thanks! Will do.

Committed r191507: <http://trac.webkit.org/changeset/191507>.