150464 2015-10-22 12:55:50 -0700 [WinCairo] Improve test results for fast layouttests. 2015-10-23 13:24:38 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 peavo webkit-unassigned achristensen bfulgham commit-queue lforschler mmaxfield ossy oldest_to_newest 1135784 0 peavo 2015-10-22 12:55:50 -0700 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. 1135786 1 263848 peavo 2015-10-22 13:04:48 -0700 Created attachment 263848 Patch 1135788 2 peavo 2015-10-22 13:09:01 -0700 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 :) 1135824 3 ossy 2015-10-22 14:00:57 -0700 run-webkit-tests doesn't know --wincairo platform at all. Do you plan to fix it and make the WinCairo bot run tests? 1135843 4 peavo 2015-10-22 14:16:00 -0700 (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 :) 1135861 5 263848 achristensen 2015-10-22 15:03:53 -0700 Comment on attachment 263848 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? 1136024 6 263918 peavo 2015-10-23 05:47:20 -0700 Created attachment 263918 Patch 1136026 7 peavo 2015-10-23 05:52:12 -0700 (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. 1136080 8 263918 achristensen 2015-10-23 11:14:32 -0700 Comment on attachment 263918 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) 1136105 9 263918 achristensen 2015-10-23 12:17:27 -0700 Comment on attachment 263918 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? 1136107 10 peavo 2015-10-23 12:30:03 -0700 (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. 1136108 11 achristensen 2015-10-23 12:31:40 -0700 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. 1136115 12 peavo 2015-10-23 12:46:58 -0700 (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. 1136116 13 263918 achristensen 2015-10-23 12:48:51 -0700 Comment on attachment 263918 Patch Sounds good. Commit it without the #if USE(CAIRO) in DumpRenderTree.cpp. 1136117 14 peavo 2015-10-23 12:50:30 -0700 (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. 1136123 15 peavo 2015-10-23 13:24:38 -0700 Committed r191507: <http://trac.webkit.org/changeset/191507>. 263848 2015-10-22 13:04:48 -0700 2015-10-23 05:47:12 -0700 Patch bug-150464-20151022220418.patch text/plain 1512 peavo SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDE5MTQ2NykKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE2IEBACisyMDE1LTEwLTIyICBQZXIgQXJuZSBWb2xsYW4gIDxwZWF2b0BvdXRsb29rLmNv bT4KKworICAgICAgICBbV2luQ2Fpcm9dIEltcHJvdmUgdGVzdCByZXN1bHRzIGZvciBmYXN0IGxh eW91dHRlc3RzLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MTUwNDY0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgU2V0IGN1c3RvbSBkZXZpY2Ugc2NhbGUgZmFjdG9yIHRvIDEuMC4KKyAgICAgICAgRGlzYWJs ZSB0aGUgTWFjIGFzY2VudCBoYWNrIGZvciBXaW5DYWlyby4KKworICAgICAgICAqIER1bXBSZW5k ZXJUcmVlL3dpbi9EdW1wUmVuZGVyVHJlZS5jcHA6CisgICAgICAgIChjcmVhdGVXZWJWaWV3QW5k T2Zmc2NyZWVuV2luZG93KToKKwogMjAxNS0xMC0yMiAgUnlvc3VrZSBOaXdhICA8cm5pd2FAd2Vi a2l0Lm9yZz4KIAogICAgICAgICBSRUdSRVNTSU9OIChyMTgxOTcyKTogU2Nyb2xsIHBvc2l0aW9u IGNoYW5nZXMgdG8gdG9wIG9mIHlvdXR1YmUgcGFnZSB3aGVuIHN3aXRjaGluZyB0YWJzCkluZGV4 OiBUb29scy9EdW1wUmVuZGVyVHJlZS93aW4vRHVtcFJlbmRlclRyZWUuY3BwCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFRvb2xzL0R1bXBSZW5kZXJUcmVlL3dpbi9EdW1wUmVuZGVyVHJlZS5jcHAJKHJldmlzaW9u IDE5MTM5NikKKysrIFRvb2xzL0R1bXBSZW5kZXJUcmVlL3dpbi9EdW1wUmVuZGVyVHJlZS5jcHAJ KHdvcmtpbmcgY29weSkKQEAgLTEyMzIsOCArMTIzMiwxMyBAQCBJV2ViVmlldyogY3JlYXRlV2Vi Vmlld0FuZE9mZnNjcmVlbldpbmRvCiAgICAgaWYgKEZBSUxFRCh3ZWJWaWV3LT5RdWVyeUludGVy ZmFjZSgmdmlld1ByaXZhdGUpKSkKICAgICAgICAgcmV0dXJuIG51bGxwdHI7CiAKKyNpZiAhVVNF KENBSVJPKQogICAgIHZpZXdQcml2YXRlLT5zZXRTaG91bGRBcHBseU1hY0ZvbnRBc2NlbnRIYWNr KFRSVUUpOworI2VuZGlmCiAgICAgdmlld1ByaXZhdGUtPnNldEFsd2F5c1VzZXNDb21wbGV4VGV4 dENvZGVQYXRoKGZvcmNlQ29tcGxleFRleHQpOworI2lmIFVTRShDQUlSTykKKyAgICB2aWV3UHJp dmF0ZS0+c2V0Q3VzdG9tQmFja2luZ1NjYWxlRmFjdG9yKDEuMCk7CisjZW5kaWYKIAogICAgIF9i c3RyX3QgcGx1Z2luUGF0aCA9IF9ic3RyX3QoZXhlUGF0aCgpLmRhdGEoKSkgKyBUZXN0UGx1Z2lu RGlyOwogICAgIGlmIChGQUlMRUQodmlld1ByaXZhdGUtPmFkZEFkZGl0aW9uYWxQbHVnaW5EaXJl Y3RvcnkocGx1Z2luUGF0aC5HZXRCU1RSKCkpKSkK 263918 2015-10-23 05:47:20 -0700 2015-10-23 12:48:51 -0700 Patch bug-150464-20151023144651.patch text/plain 2722 peavo SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE5MTQ5NCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDE1LTEwLTIzICBQZXIgQXJu ZSBWb2xsYW4gIDxwZWF2b0BvdXRsb29rLmNvbT4KKworICAgICAgICBbV2luQ2Fpcm9dIEltcHJv dmUgdGVzdCByZXN1bHRzIGZvciBmYXN0IGxheW91dHRlc3RzLgorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTUwNDY0CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgRGlzYWJsZSB0aGUgTWFjIGFzY2VudCBoYWNr IGZvciBXaW5DYWlyby4KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL3dpbi9TaW1wbGVG b250RGF0YUNhaXJvV2luLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkZvbnQ6OnBsYXRmb3JtSW5p dCk6CisKIDIwMTUtMTAtMjMgIEFudG9pbmUgUXVpbnQgIDxncmFvdXRzQGFwcGxlLmNvbT4KIAog ICAgICAgICBTdXBwb3J0IGZvciBTVkcgYGJlZ2luRXZlbnRgIGV2ZW50IGFuZCBgb25iZWdpbmAg YXR0cmlidXRlCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy93aW4vU2lt cGxlRm9udERhdGFDYWlyb1dpbi5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcGxh dGZvcm0vZ3JhcGhpY3Mvd2luL1NpbXBsZUZvbnREYXRhQ2Fpcm9XaW4uY3BwCShyZXZpc2lvbiAx OTEzOTYpCisrKyBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy93aW4vU2ltcGxlRm9u dERhdGFDYWlyb1dpbi5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTgxLDggKzgxLDYgQEAgdm9pZCBG b250OjpwbGF0Zm9ybUluaXQoKQogICAgIDo6R2V0VGV4dEZhY2UoZGMsIGZhY2VMZW5ndGgsIGZh Y2VOYW1lLmRhdGEoKSk7CiAgICAgbV9pc1N5c3RlbUZvbnQgPSAhd2NzY21wKGZhY2VOYW1lLmRh dGEoKSwgTCJMdWNpZGEgR3JhbmRlIik7CiAgCi0gICAgYXNjZW50ID0gYXNjZW50Q29uc2lkZXJp bmdNYWNBc2NlbnRIYWNrKGZhY2VOYW1lLmRhdGEoKSwgYXNjZW50LCBkZXNjZW50KTsKLQogICAg IG1fZm9udE1ldHJpY3Muc2V0QXNjZW50KGFzY2VudCk7CiAgICAgbV9mb250TWV0cmljcy5zZXRE ZXNjZW50KGRlc2NlbnQpOwogICAgIG1fZm9udE1ldHJpY3Muc2V0TGluZUdhcChsaW5lR2FwKTsK SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDE5MTQ2NykKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE1IEBACisyMDE1LTEwLTIyICBQZXIgQXJuZSBWb2xsYW4gIDxwZWF2b0BvdXRsb29rLmNv bT4KKworICAgICAgICBbV2luQ2Fpcm9dIEltcHJvdmUgdGVzdCByZXN1bHRzIGZvciBmYXN0IGxh eW91dHRlc3RzLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MTUwNDY0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgU2V0IGN1c3RvbSBkZXZpY2Ugc2NhbGUgZmFjdG9yIHRvIDEuMC4KKworICAgICAgICAqIER1 bXBSZW5kZXJUcmVlL3dpbi9EdW1wUmVuZGVyVHJlZS5jcHA6CisgICAgICAgIChjcmVhdGVXZWJW aWV3QW5kT2Zmc2NyZWVuV2luZG93KToKKwogMjAxNS0xMC0yMiAgUnlvc3VrZSBOaXdhICA8cm5p d2FAd2Via2l0Lm9yZz4KIAogICAgICAgICBSRUdSRVNTSU9OIChyMTgxOTcyKTogU2Nyb2xsIHBv c2l0aW9uIGNoYW5nZXMgdG8gdG9wIG9mIHlvdXR1YmUgcGFnZSB3aGVuIHN3aXRjaGluZyB0YWJz CkluZGV4OiBUb29scy9EdW1wUmVuZGVyVHJlZS93aW4vRHVtcFJlbmRlclRyZWUuY3BwCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIFRvb2xzL0R1bXBSZW5kZXJUcmVlL3dpbi9EdW1wUmVuZGVyVHJlZS5jcHAJKHJl dmlzaW9uIDE5MTM5NikKKysrIFRvb2xzL0R1bXBSZW5kZXJUcmVlL3dpbi9EdW1wUmVuZGVyVHJl ZS5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTEyMzQsNiArMTIzNCw5IEBAIElXZWJWaWV3KiBjcmVh dGVXZWJWaWV3QW5kT2Zmc2NyZWVuV2luZG8KIAogICAgIHZpZXdQcml2YXRlLT5zZXRTaG91bGRB cHBseU1hY0ZvbnRBc2NlbnRIYWNrKFRSVUUpOwogICAgIHZpZXdQcml2YXRlLT5zZXRBbHdheXNV c2VzQ29tcGxleFRleHRDb2RlUGF0aChmb3JjZUNvbXBsZXhUZXh0KTsKKyNpZiBVU0UoQ0FJUk8p CisgICAgdmlld1ByaXZhdGUtPnNldEN1c3RvbUJhY2tpbmdTY2FsZUZhY3RvcigxLjApOworI2Vu ZGlmCiAKICAgICBfYnN0cl90IHBsdWdpblBhdGggPSBfYnN0cl90KGV4ZVBhdGgoKS5kYXRhKCkp ICsgVGVzdFBsdWdpbkRpcjsKICAgICBpZiAoRkFJTEVEKHZpZXdQcml2YXRlLT5hZGRBZGRpdGlv bmFsUGx1Z2luRGlyZWN0b3J5KHBsdWdpblBhdGguR2V0QlNUUigpKSkpCg==