Bug 174374

Created attachment 315132 [details] Patch

Comment on attachment 315132 [details] Patch Oh wow! I think this patch is correct. Thanks! I wonder if you just fixed bug #115634? Do you think so? I think it might be a bit cleaner if you tried writing it as a for loop instead, using i instead of matchIndex. Could you try that and see how it turns out? Since changes related to Fontconfig are inherently tricky, I'd love to see some example debug output taken by calling FcPatternPrint() once on resultPattern. I presume the result is something along the lines of "Family: SomethingNotGotham,Gotham", is that right? So fonts can have multiple family names? Next step: I think this really needs a layout test. Do you think that can be achieved just by playing with our test fonts.conf Tools/WebKitTestRunner/gtk/fonts/fonts.conf, without requiring adding a new font to our test fonts repository? I'm not sure. I guess it might require that we have some font for which Fontconfig returns multiple language names. I don't know how hard it would be to create such a test font. We could use Gotham if it's freely redistributable, but if not, then perhaps it is not practical to add a test for this.

Note: our tests are currently under LayoutTests/platform/gtk/fonts (though most of them should probably move elsewhere to be shared with WPE), and they depend heavily on that test fonts.conf under Tools/WebKitTestRunner/gtk/fonts. The fonts themselves are at https://github.com/mrobinson/webkitgtk-test-fonts. I wonder if any of the fonts already there could be used to test this. If it's not practical to add a test, them I'm willing to give r+ without one, but it would be great if it is possible.

It very well could fix bug #115634. I will re-write this as a for loop, and capture some debug output for you to see too. I'm not sure how hard a test would be to write yet, I'll look into it.

Comment on attachment 315132 [details] Patch Attachment 315132 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4102459 New failing tests: storage/websql/execute-sql-rowsAffected.html

Created attachment 315151 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5

Created attachment 315172 [details] Gotham FcPatternPrint

In this case the font has "Gotham" and "Gotham Book" as the family names. The content was using font-family: "Gotham Book".

Created attachment 315173 [details] Patch

I looked through the test fonts, and didn't see any with multiple names. I also don't know how to simulate it with fonts.conf. Right now I wouldn't be able to test it against a Gtk build since I don't have one readily set up. Attached is a revised patch.

Comment on attachment 315173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315173&action=review > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:381 > + if (!equalIgnoringASCIICase(familyNameAfterConfiguration, familyNameAfterMatching) && !isCommonlyUsedGenericFamily(familyNameString) && !areStronglyAliased(familyNameAfterConfiguration, familyNameAfterMatching)) > + continue; > + > + matchedFontFamily = true; > + break; You can simplify this significantly by inverting the condition: if (equalIgnoringASCIICase(...) || isCommonlyUsedGenericFamily(...) || areStronglyAliased(...)) { matchedFontFamily = true; break; } Then you don't need a continue statement at all.

Created attachment 315250 [details] Patch

Comment on attachment 315250 [details] Patch Clearing flags on attachment: 315250 Committed r219418: <http://trac.webkit.org/changeset/219418>

All reviewed patches have been landed. Closing bug.