[FreeType] Font matching fails to consider all family names

[Timothy Hatcher]

Right now the FreeType implementation of FontCache::createFontPlatformData only checks the first match from FontConfig when matching fonts. Some fonts can have multiple matches and not always have the first match being the one that is needed. This was preventing matches of the Gotham font we use at Tesla.

[Timothy Hatcher]

Created attachment 315132 [details]
Patch

[Michael Catanzaro]

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.

[Michael Catanzaro]

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.

[Timothy Hatcher]

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.

[Build Bot]

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

[Build Bot]

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

[Timothy Hatcher]

Created attachment 315172 [details]
Gotham FcPatternPrint

[Timothy Hatcher]

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

[Timothy Hatcher]

Created attachment 315173 [details]
Patch

[Timothy Hatcher]

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.

[Michael Catanzaro]

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.

[Timothy Hatcher]

Created attachment 315250 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 315250 [details]
Patch

Clearing flags on attachment: 315250

Committed r219418: <http://trac.webkit.org/changeset/219418>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.