Bug 176351

I've been working on this and I've realized that after the 2.8 upgrade we have some failures that don't look correct. The problem is not new, though see bug #102374. That bug was closed because a patch was added for freetype 2.4.11, it was a cherry-pick of a freetype commit (http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=e0469372be3870a5ad60b2c4586e9c281357bd28), but that was reverted in freetype just a few revisions later (http://git.savannah.gnu.org/cgit/freetype/freetype2.git/commit/?id=87dc86d68c93c2b5b836501ece57583d1d6d1a2e), so when upgrading to 2.8 I decided not to include the patch. So, with 2.8 now we are getting: ascent: 15.000000, descent: 4.000000, height: 18.000000 while with previous patched version we got: ascent: 14.000000, descent: 3.000000, height: 17.000000 this means that now ascent + descent > height and we are getting a lineGap of -1. I don't want to keep using a patch that has been reverted upstream. This patch is also the reason why we get different results in many layout tests when not using the internal jhbuild. Also, I don't know why, but I tried to backport the patch to 2.8 just to check what happened and I was still getting -1, so that's no longer a solution. I'm not a fonts expert but the problem seems to be the rounding done by cairo when using hint metrics. When not using them what we get is: ascent: 14.257812, descent: 3.460938, height: 18.398438 where ascent + descent <= height and we get a line gap of 0.679688 I think we have several options here: a) Try to clamp the line gap to be 0 when we get a negative value b) Create a temporary cairo font with hint metrics disable only to get the metrics (this is the patch proposed by Dominik in bug #102374). c) Use freetype api directly to get the metrics instead of cairo. I don't know if these are valid solutions or if there's a better one. Thoughts?

Created attachment 320008 [details] Patch This patch upgrades to 2.8 and removes the patch we were using. Instead it uses Dominiks's patch (slightly modified just ot use smart pointers and a fe other cosmetic changes). With this I'm getting 5886 failures. The differences are changes in the text height in 1px. I'm preparing the rebaseline in blocks.

I have the rebaselines ready in 7 patches: $ ls -lh 000*.patch -rw-r--r-- 1 cgarcia cgarcia 36M sep 6 14:55 0001-rebaseline-1.patch -rw-r--r-- 1 cgarcia cgarcia 20M sep 6 14:55 0002-rebaseline-2.patch -rw-r--r-- 1 cgarcia cgarcia 27M sep 6 14:55 0003-rebaseline-3.patch -rw-r--r-- 1 cgarcia cgarcia 42M sep 6 14:55 0004-rebaseline-4.patch -rw-r--r-- 1 cgarcia cgarcia 32M sep 6 14:55 0005-rebaseline-5.patch -rw-r--r-- 1 cgarcia cgarcia 39M sep 6 14:56 0006-rebaseline-6.patch -rw-r--r-- 1 cgarcia cgarcia 42M sep 6 14:56 0007-rebaseline-7.patch Hopefully future freetype upgrades will not be so painful now that we don't patch it anymore.

Comment on attachment 320008 [details] Patch Thanks! getting rid of patches on our jhbuild is always a good thing.

Committed r221670: <http://trac.webkit.org/changeset/221670>

This killed the WPE bot :(

(In reply to Michael Catanzaro from comment #6) > This killed the WPE bot :( hmm, right, they are also using the freetype 2.4.11 patch. As a workaround we could ifdef the code introduced in this patch. Then I suggests someone bumps freetype to 2.8 in wpe as well, removing the patch and ifdefs and then rebaselining. Not fun at all :-(

(In reply to Carlos Garcia Campos from comment #7) > (In reply to Michael Catanzaro from comment #6) > > This killed the WPE bot :( > > hmm, right, they are also using the freetype 2.4.11 patch. As a workaround > we could ifdef the code introduced in this patch. Then I suggests someone > bumps freetype to 2.8 in wpe as well, removing the patch and ifdefs and then > rebaselining. Not fun at all :-( I think we can do this now, not need to ifdef the code.