WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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)
gustavo
: 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)
gustavo
: 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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
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
https://bugs.webkit.org/show_bug.cgi?id=41091
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug