Bug 55036 - [Qt] FontCache::createFontPlatformData() is broken, a default font is returned even if the family does not match
Summary: [Qt] FontCache::createFontPlatformData() is broken, a default font is returne...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Major
Assignee: Pierre Rossi
URL:
Keywords: Qt, QtTriaged
: 66186 (view as bug list)
Depends on:
Blocks: 36351 67131
  Show dependency treegraph
 
Reported: 2011-02-23 04:26 PST by Pierre Rossi
Modified: 2011-11-12 04:58 PST (History)
12 users (show)

See Also:


Attachments
proposed fix (depends on another patch yet to land in Qt master) (1.62 KB, patch)
2011-02-23 04:38 PST, Pierre Rossi
eric: review-
Details | Formatted Diff | Diff
patch v2 (2.32 KB, patch)
2011-07-22 04:58 PDT, Pierre Rossi
no flags Details | Formatted Diff | Diff
Patch (1.96 KB, patch)
2011-08-08 05:46 PDT, Pierre Rossi
no flags Details | Formatted Diff | Diff
fix using QFont::exactMatch (1.61 KB, patch)
2011-08-13 11:15 PDT, Andrew Wason
no flags Details | Formatted Diff | Diff
Patch (1.86 KB, patch)
2011-08-16 07:58 PDT, Pierre Rossi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre Rossi 2011-02-23 04:26:51 PST
Since QFont always falls back to something, QtWebKit currently only tries the first font family from a given CSS.

Depending on the fonts installed, this is something that can be reproduced (for me at least) on websites such as:

http://whatthecommit.com/
http://code.google.com/more/table/
Comment 1 Benjamin Poulain 2011-02-23 04:30:20 PST
Assigning to Pierre since he is already working on a fix.
Comment 2 Pierre Rossi 2011-02-23 04:38:00 PST
Created attachment 83471 [details]
proposed fix (depends on another patch yet to land in Qt master)
Comment 3 Eric Seidel (no email) 2011-02-24 02:57:24 PST
Comment on attachment 83471 [details]
proposed fix (depends on another patch yet to land in Qt master)

I don't understand what this is doing.  How do we test this?
Comment 4 Pierre Rossi 2011-02-24 13:10:11 PST
(In reply to comment #3)
> (From update of attachment 83471 [details])
> I don't understand what this is doing.  How do we test this?

The issue in QtWebKit (and subsequently Rekonq) is that QFont always falls back to something, and eventually it might be the default font, and not what we want at all.
In the examples I cited, take whatthecommit.com for instance: on my system, the request for "Lucida Console" defaults to "DejaVu Sans", so it never even tries "Courier New" and the rendering looks very different from what I get in Chromium or Firefox.
As for the testing part, well I believe there are tests already, and I suspect a bunch of them might break because of this, but that would be because the "expected" results were not quite correct in the first place IMHO.
Comment 5 Pierre Rossi 2011-02-24 13:11:58 PST
(In reply to comment #3)
> (From update of attachment 83471 [details])
> I don't understand what this is doing.  How do we test this?

Oh, I forgot to point there:
http://bugreports.qt.nokia.com/browse/QTBUG-15575

to find the patches required on top of Qt for this to work.
Comment 6 Benjamin Poulain 2011-04-13 06:13:12 PDT
Adding as nice to have for QtWebKit 2.2 release. If the API is in Qt, it would be stupid not to fix the bug.
Comment 7 Pierre Rossi 2011-06-06 06:32:19 PDT
Unfortunately that's not something that can be fixed easily for 2.2 at least since 4.8 is feature frozen and I didn't push to add the required function because of the new raw font approach that should have made that useless.
Comment 8 Pierre Rossi 2011-07-22 04:58:38 PDT
Created attachment 101713 [details]
patch v2

This might require synchronizing for the the bots since the #ifdef will break the build until Qt is bumped to have QFontDatabase::hasFamily().
And an obscene number of layout tests results are going to need to be updated as a result, not sure how to do that properly.
Comment 9 Benjamin Poulain 2011-08-07 05:15:45 PDT
Comment on attachment 101713 [details]
patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=101713&action=review

> Source/WebCore/ChangeLog:16
> +        No new tests. (OOPS!)

Remove this line. Instead, you should write this is covered by existing tests.
Comment 10 Benjamin Poulain 2011-08-07 05:23:41 PDT
Andreas, Robert, any comment?
Comment 11 Robert Hogan 2011-08-07 05:50:19 PDT
Hi Pierre,
This is the same as 36351, I think you should add the test from there or some other suitable test. Otherwise looks fine to me!
Comment 12 Robert Hogan 2011-08-07 05:52:26 PDT
Comment on attachment 101713 [details]
patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=101713&action=review

> Source/WebCore/ChangeLog:9
> +        the requestfor a given font family can be

s/requestfor/request for
Comment 13 Pierre Rossi 2011-08-07 08:28:53 PDT
(In reply to comment #11)
> Hi Pierre,
> This is the same as 36351, I think you should add the test from there or some other suitable test. Otherwise looks fine to me!

I'm afraid that test won't work with just the font fallback getting fixed, but is going to with another patch I have in store (essentially it just requires loading the data we get with QRawFont). Can I steal it for that other change then ?
Comment 14 Robert Hogan 2011-08-07 14:11:05 PDT
(In reply to comment #13)
> (In reply to comment #11)
> > Hi Pierre,
> > This is the same as 36351, I think you should add the test from there or some other suitable test. Otherwise looks fine to me!
> 
> I'm afraid that test won't work with just the font fallback getting fixed, but is going to with another patch I have in store (essentially it just requires loading the data we get with QRawFont). Can I steal it for that other change then ?

Oh OK. The test I'm referring to is fast/css/font-face-invalid-local-source.html in the patch, btw. I would have expected this change to make it pass, but it's been a while.
Comment 15 Pierre Rossi 2011-08-08 05:46:10 PDT
Created attachment 103235 [details]
Patch

Ok then, just changed the ChangeLog about tests.
Comment 16 Andreas Kling 2011-08-09 06:33:31 PDT
Comment on attachment 103235 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103235&action=review

r- to get some more polish up in here. :)

> Source/WebCore/ChangeLog:3
> +        Fix the QFont fallback issue.

That could mean a lot of things. The bug title would fit very nicely here instead.

> Source/WebCore/ChangeLog:12
> +        This is a showstopper to get webfonts working
> +        properly in QtWebKit.

This is bug tracker talk, and can be excluded from the ChangeLog.

> Source/WebCore/ChangeLog:18
> +        This change should be covered by existing tests,
> +        but there's a good chance some of these tests
> +        will in turn need to be fixed.

We should strive to be more certain in ChangeLog entries.
Run the tests with your change and find out what breaks, then (if applicable) include the updated qt-4.8 baselines in the patch.

> Source/WebCore/platform/graphics/qt/FontCacheQt.cpp:114
> +#if QT_VERSION >= QT_VERSION_CHECK(4, 8, 0)
> +    QFontDatabase db;
> +    if (!db.hasFamily(familyName))
> +        return 0;
> +#endif

This looks good to me. Is the API available on the Qt-4.8 used by our bots though?
Comment 17 Andrew Wason 2011-08-13 11:15:27 PDT
Created attachment 103862 [details]
fix using QFont::exactMatch

I think my bug 66186 is a duplicate of this, I really need a fix that works with Qt 4.7 for that.

Attached patch uses QFont::exactMatch() on the FontPlatformData::font() to check if it is the font we asked for, and return 0 if not.
Comment 18 Andrew Wason 2011-08-13 11:16:26 PDT
*** Bug 66186 has been marked as a duplicate of this bug. ***
Comment 19 Andrew Wason 2011-08-13 14:57:55 PDT
Comment on attachment 103862 [details]
fix using QFont::exactMatch

Never mind, just read the discussion in bug 36351 on why exactMatch() won't work.
Comment 20 Pierre Rossi 2011-08-15 13:33:31 PDT
(In reply to comment #16)

> > Source/WebCore/ChangeLog:18
> > +        This change should be covered by existing tests,
> > +        but there's a good chance some of these tests
> > +        will in turn need to be fixed.
> 
> We should strive to be more certain in ChangeLog entries.
> Run the tests with your change and find out what breaks, then (if applicable) include the updated qt-4.8 baselines in the patch.

Agreed, I just remembered it made the layout tests unhappy. Now I re-ran the tests and it breaks more than four thousand of them. Is there a nice way to update all of that ?
Comment 21 Benjamin Poulain 2011-08-15 14:29:01 PDT
(In reply to comment #20)
> (In reply to comment #16)
> 
> > > Source/WebCore/ChangeLog:18
> > > +        This change should be covered by existing tests,
> > > +        but there's a good chance some of these tests
> > > +        will in turn need to be fixed.
> > 
> > We should strive to be more certain in ChangeLog entries.
> > Run the tests with your change and find out what breaks, then (if applicable) include the updated qt-4.8 baselines in the patch.
> 
> Agreed, I just remembered it made the layout tests unhappy. Now I re-ran the tests and it breaks more than four thousand of them. Is there a nice way to update all of that ?

You can ask Ossy for help, he has a talent at keeping bots happy ;)

But I think the comment here is more that the Changelog is not good enough. It is your job to make sure a patch is tested correctly. Then you detail it in the Changelog. Here the Changelog reads like you have no clue if the patch is tested properly.
Comment 22 Robert Hogan 2011-08-15 14:49:21 PDT
4000 sounds like too many to be right - the tests must be rendering with a different (possibly more accurate) font now. The best approach may be to patch DumpRenderTree to ensure the same fonts as before are used (for now).

Normally you would use the QBat-VM in a case like this:

http://webkit.sed.hu/blog/20101028/qtwebkit-builder-and-tester-virtual-machine

But it doesn't have a 4.8 environment. You should post a link to some of the failures maybe, in case there's something else wrong.
Comment 23 Pierre Rossi 2011-08-16 07:58:21 PDT
Created attachment 104041 [details]
Patch

From looking at the results, I think there are so many tests effected by a change in font matching and hence font metrics, 4000 might not be that shocking after all.
Comment 24 Pierre Rossi 2011-08-16 08:16:16 PDT
Comment on attachment 104041 [details]
Patch

The 4.8 version on the bot is too old for now, so I guess this will have to wait a little longer.
Comment 25 Benjamin Poulain 2011-08-19 04:45:47 PDT
(In reply to comment #24)
> (From update of attachment 104041 [details])
> The 4.8 version on the bot is too old for now, so I guess this will have to wait a little longer.

You can land something now and update the results when the bots get updated.
Comment 26 Andreas Kling 2011-08-22 04:12:26 PDT
Comment on attachment 104041 [details]
Patch

r=me
Do exercise caution (with regard to the bots) when landing this.
Comment 27 Csaba Osztrogonác 2011-09-07 07:12:52 PDT
Unfortunately it is more complex problem that I thought previously. :((

I sent a mail to webkit-qt mailing list about it: 
https://lists.webkit.org/pipermail/webkit-qt/2011-September/001864.html
Comment 28 Ademar Reis 2011-09-08 06:03:55 PDT
Check also this upstream Qt bug:
https://bugreports.qt.nokia.com/browse/QTBUG-21036