Bug 38758 – [gtk] web fonts not loaded properly in scribd html5 reader

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Jonathon Jongsma (jonner)]

visit http://www.scribd.com/documents/30964170/Scribd-in-HTML5, and read the document in the html5 reader.  the fonts are all mis-placed and not being displayed correctly due to not loading the web fonts appropriately

[Gustavo Noronha (kov)]

Created attachment 55715 [details]
a small test case I am tyring to use to reproduce the problem

[Gustavo Noronha (kov)]

There are two issues here. First, WebKitGTK+ does not even try to download the fonts - it always thinks it has the 'â' font that is specified by scribd as the prefferred local font. I have a fix for this. Even after getting WebKitGTK+ to actually download the fonts, they are not being applied, though.

[Gustavo Noronha (kov)]

Created attachment 56741 [details]
proposed fix

[WebKit Review Bot]

Attachment 56741 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/cairo/FontCacheCairo.cpp:106:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xan Lopez]

Comment on attachment 56741 [details]
proposed fix

I suppose this is very difficult to test without pixel tests? Looks good to me in any case, assuming we don't regress somewhere else :)

r=me with the style nit fixed.

[Gustavo Noronha (kov)]

Created attachment 56913 [details]
second try

After running all the tests I found a number of cases where we would have to regenerate results, and even a few failures caused by changes in the sizes of fonts (the new code would fallback all the way to Times New Roman, instead of matching with FC immediately, as was done before). What this means is essentially adding more special cases, for well-known font names the tests expect. I decided to not do that for the japanese font names, though, since the test result is a passing result, I decided to just generate a new result.

[Xan Lopez]

Comment on attachment 56913 [details]
second try

>+    // Handle generic family types specially, because fontconfig does not know them, but we have
>+    // code to fallback correctly in our platform data implementation. Times New Roman is treated
>+    // specially, as it is used as last fallback.
>+    if (!family.length() || family.startsWith("-webkit-")
>+        || (fontDescription.genericFamily() != FontDescription::NoFamily)
>+        || isWellKnownFontName(family) || (family == "Times New Roman"))
>+        return new FontPlatformData(fontDescription, family);

Not sure if it makes sense to not have Time News Roman in the well known list at this point.

>+
>+    // First check the font exists.
>+    CString familyNameString = family.string().utf8();
>+    const char* fcfamily = familyNameString.data();
>+
>+    FcPattern* pattern = FcPatternCreate();
>+    if (!FcPatternAddString(pattern, FC_FAMILY, reinterpret_cast<const FcChar8*>(fcfamily))) {
>+        FcPatternDestroy(pattern);
>+        return 0;
>+    }
>+
>+    FcConfigSubstitute(0, pattern, FcMatchPattern);
>+    FcDefaultSubstitute(pattern);
>+
>+    FcObjectSet* objectSet = FcObjectSetCreate();
>+    if (!FcObjectSetAdd(objectSet, FC_FAMILY)) {
>+        FcPatternDestroy(pattern);
>+        FcObjectSetDestroy(objectSet);
>+        return 0;
>+    }
>+
>+    FcFontSet* fontSet = FcFontList(0, pattern, objectSet);
>+    FcPatternDestroy(pattern);
>+    FcObjectSetDestroy(objectSet);
>+
>+    if (!fontSet)
>+        return 0;
>+
>+    if (!fontSet->fonts) {
>+        FcFontSetDestroy(fontSet);
>+        return 0;
>+    }
>+
>+    FcFontSetDestroy(fontSet);
>+
>     return new FontPlatformData(fontDescription, family);

Most of the code here is just freeing a couple of data structures all the time, guess we should just use GOwnPtr.

Looks good other than that.

[Gustavo Noronha (kov)]

Created attachment 57542 [details]
third time is the charm

Address Xan's comments - added GOwnPtr infrastructure for Fc types, and moved Times New Roman to the well known font name handling.

[Eric Seidel (no email)]

Comment on attachment 56741 [details]
proposed fix

Cleared Xan Lopez's review+ from obsolete attachment 56741 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Gustavo Noronha (kov)]

Landed as r60793.

[Gustavo Noronha (kov)]

Oops, this bug should remain open - we only fixed the first problem.

[Gustavo Noronha (kov)]

Comment on attachment 57542 [details]
third time is the charm

Clearing review flag.

[Gustavo Noronha (kov)]

Created attachment 58720 [details]
special-case mono and sans as well

Shaun complained that yelp started using a serif font even though he was specifying 'sans'. The problem is this alias is not being special-cased.

[Gustavo Noronha (kov)]

Comment on attachment 58720 [details]
special-case mono and sans as well

doh

[Gustavo Noronha (kov)]

Created attachment 58776 [details]
remove restrictions from font matching

Trying the new code with a few new fonts, I noticed it was not matching them. Playing around with a test program I found that the default substitutions end up being too restrictive, and are not required to exclude fonts that do not exist, so this patch removes those.

[Xan Lopez]

Comment on attachment 58720 [details]
special-case mono and sans as well

OK.

[Xan Lopez]

Comment on attachment 58776 [details]
remove restrictions from font matching

OK, fc is weird.

[Gustavo Noronha (kov)]

Comment on attachment 58720 [details]
special-case mono and sans as well

Landed as r61701.

[Gustavo Noronha (kov)]

Comment on attachment 58776 [details]
remove restrictions from font matching

Clearing flags on attachment: 58776

Committed r61703: <http://trac.webkit.org/changeset/61703>

[Gustavo Noronha (kov)]

All reviewed patches have been landed.  Closing bug.

[Gustavo Noronha (kov)]

I'll open a different bug to track the other problem, I'll post the URL here.

[Gustavo Noronha (kov)]

https://bugs.webkit.org/show_bug.cgi?id=41091