Bug 38758 - [gtk] web fonts not loaded properly in scribd html5 reader
Summary: [gtk] web fonts not loaded properly in scribd html5 reader
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://www.scribd.com/documents/30964...
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2010-05-07 09:37 PDT by Jonathon Jongsma (jonner)
Modified: 2010-06-23 11:55 PDT (History)
3 users (show)

See Also:


Attachments
a small test case I am tyring to use to reproduce the problem (445 bytes, text/html)
2010-05-11 10:34 PDT, Gustavo Noronha (kov)
no flags Details
proposed fix (3.43 KB, patch)
2010-05-21 13:51 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
second try (11.16 KB, patch)
2010-05-24 12:42 PDT, Gustavo Noronha (kov)
xan.lopez: review-
Details | Formatted Diff | Diff
third time is the charm (15.18 KB, patch)
2010-06-01 07:36 PDT, Gustavo Noronha (kov)
gns: commit-queue-
Details | Formatted Diff | Diff
special-case mono and sans as well (2.11 KB, patch)
2010-06-14 15:57 PDT, Gustavo Noronha (kov)
gns: commit-queue-
Details | Formatted Diff | Diff
remove restrictions from font matching (1.81 KB, patch)
2010-06-15 07:43 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathon Jongsma (jonner) 2010-05-07 09:37:21 PDT
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
Comment 1 Gustavo Noronha (kov) 2010-05-11 10:34:58 PDT
Created attachment 55715 [details]
a small test case I am tyring to use to reproduce the problem
Comment 2 Gustavo Noronha (kov) 2010-05-21 13:47:57 PDT
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.
Comment 3 Gustavo Noronha (kov) 2010-05-21 13:51:26 PDT
Created attachment 56741 [details]
proposed fix
Comment 4 WebKit Review Bot 2010-05-21 13:54:08 PDT
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.
Comment 5 Xan Lopez 2010-05-22 01:49:30 PDT
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.
Comment 6 Gustavo Noronha (kov) 2010-05-24 12:42:08 PDT
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.
Comment 7 Xan Lopez 2010-06-01 05:05:51 PDT
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.
Comment 8 Gustavo Noronha (kov) 2010-06-01 07:36:18 PDT
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.
Comment 9 Eric Seidel (no email) 2010-06-02 02:25:33 PDT
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.
Comment 10 Gustavo Noronha (kov) 2010-06-07 14:15:12 PDT
Landed as r60793.
Comment 11 Gustavo Noronha (kov) 2010-06-08 07:55:19 PDT
Oops, this bug should remain open - we only fixed the first problem.
Comment 12 Gustavo Noronha (kov) 2010-06-08 07:56:06 PDT
Comment on attachment 57542 [details]
third time is the charm

Clearing review flag.
Comment 13 Gustavo Noronha (kov) 2010-06-14 15:57:13 PDT
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.
Comment 14 Gustavo Noronha (kov) 2010-06-14 15:57:31 PDT
Comment on attachment 58720 [details]
special-case mono and sans as well

doh
Comment 15 Gustavo Noronha (kov) 2010-06-15 07:43:14 PDT
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.
Comment 16 Xan Lopez 2010-06-23 06:45:59 PDT
Comment on attachment 58720 [details]
special-case mono and sans as well

OK.
Comment 17 Xan Lopez 2010-06-23 06:53:33 PDT
Comment on attachment 58776 [details]
remove restrictions from font matching

OK, fc is weird.
Comment 18 Gustavo Noronha (kov) 2010-06-23 11:25:57 PDT
Comment on attachment 58720 [details]
special-case mono and sans as well

Landed as r61701.
Comment 19 Gustavo Noronha (kov) 2010-06-23 11:42:30 PDT
Comment on attachment 58776 [details]
remove restrictions from font matching

Clearing flags on attachment: 58776

Committed r61703: <http://trac.webkit.org/changeset/61703>
Comment 20 Gustavo Noronha (kov) 2010-06-23 11:42:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Gustavo Noronha (kov) 2010-06-23 11:47:26 PDT
I'll open a different bug to track the other problem, I'll post the URL here.
Comment 22 Gustavo Noronha (kov) 2010-06-23 11:55:31 PDT
https://bugs.webkit.org/show_bug.cgi?id=41091