RESOLVED FIXED 38758
[gtk] web fonts not loaded properly in scribd html5 reader
https://bugs.webkit.org/show_bug.cgi?id=38758
Summary [gtk] web fonts not loaded properly in scribd html5 reader
Jonathon Jongsma (jonner)
Reported 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
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
proposed fix (3.43 KB, patch)
2010-05-21 13:51 PDT, Gustavo Noronha (kov)
no flags
second try (11.16 KB, patch)
2010-05-24 12:42 PDT, Gustavo Noronha (kov)
xan.lopez: review-
third time is the charm (15.18 KB, patch)
2010-06-01 07:36 PDT, Gustavo Noronha (kov)
gustavo: commit-queue-
special-case mono and sans as well (2.11 KB, patch)
2010-06-14 15:57 PDT, Gustavo Noronha (kov)
gustavo: commit-queue-
remove restrictions from font matching (1.81 KB, patch)
2010-06-15 07:43 PDT, Gustavo Noronha (kov)
no flags
Gustavo Noronha (kov)
Comment 1 2010-05-11 10:34:58 PDT
Created attachment 55715 [details] a small test case I am tyring to use to reproduce the problem
Gustavo Noronha (kov)
Comment 2 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.
Gustavo Noronha (kov)
Comment 3 2010-05-21 13:51:26 PDT
Created attachment 56741 [details] proposed fix
WebKit Review Bot
Comment 4 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.
Xan Lopez
Comment 5 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.
Gustavo Noronha (kov)
Comment 6 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.
Xan Lopez
Comment 7 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.
Gustavo Noronha (kov)
Comment 8 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.
Eric Seidel (no email)
Comment 9 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.
Gustavo Noronha (kov)
Comment 10 2010-06-07 14:15:12 PDT
Landed as r60793.
Gustavo Noronha (kov)
Comment 11 2010-06-08 07:55:19 PDT
Oops, this bug should remain open - we only fixed the first problem.
Gustavo Noronha (kov)
Comment 12 2010-06-08 07:56:06 PDT
Comment on attachment 57542 [details] third time is the charm Clearing review flag.
Gustavo Noronha (kov)
Comment 13 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.
Gustavo Noronha (kov)
Comment 14 2010-06-14 15:57:31 PDT
Comment on attachment 58720 [details] special-case mono and sans as well doh
Gustavo Noronha (kov)
Comment 15 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.
Xan Lopez
Comment 16 2010-06-23 06:45:59 PDT
Comment on attachment 58720 [details] special-case mono and sans as well OK.
Xan Lopez
Comment 17 2010-06-23 06:53:33 PDT
Comment on attachment 58776 [details] remove restrictions from font matching OK, fc is weird.
Gustavo Noronha (kov)
Comment 18 2010-06-23 11:25:57 PDT
Comment on attachment 58720 [details] special-case mono and sans as well Landed as r61701.
Gustavo Noronha (kov)
Comment 19 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>
Gustavo Noronha (kov)
Comment 20 2010-06-23 11:42:41 PDT
All reviewed patches have been landed. Closing bug.
Gustavo Noronha (kov)
Comment 21 2010-06-23 11:47:26 PDT
I'll open a different bug to track the other problem, I'll post the URL here.
Gustavo Noronha (kov)
Comment 22 2010-06-23 11:55:31 PDT
Note You need to log in before you can comment on or make changes to this bug.