Bug 55036 – [Qt] FontCache::createFontPlatformData() is broken, a default font is returned even if the family does not match

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Pierre Rossi]

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/

[Benjamin Poulain]

Assigning to Pierre since he is already working on a fix.

[Pierre Rossi]

Created attachment 83471 [details]
proposed fix (depends on another patch yet to land in Qt master)

[Eric Seidel (no email)]

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?

[Pierre Rossi]

(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.

[Pierre Rossi]

(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.

[Benjamin Poulain]

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.

[Pierre Rossi]

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.

[Pierre Rossi]

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.

[Benjamin Poulain]

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.

[Benjamin Poulain]

Andreas, Robert, any comment?

[Robert Hogan]

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!

[Robert Hogan]

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

[Pierre Rossi]

(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 ?

[Robert Hogan]

(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.

[Pierre Rossi]

Created attachment 103235 [details]
Patch

Ok then, just changed the ChangeLog about tests.

[Andreas Kling]

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?

[Andrew Wason]

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.

[Andrew Wason]

*** Bug 66186 has been marked as a duplicate of this bug. ***

[Andrew Wason]

Comment on attachment 103862 [details]
fix using QFont::exactMatch

Never mind, just read the discussion in bug 36351 on why exactMatch() won't work.

[Pierre Rossi]

(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 ?

[Benjamin Poulain]

(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.

[Robert Hogan]

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.

[Pierre Rossi]

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.

[Pierre Rossi]

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.

[Benjamin Poulain]

(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.

[Andreas Kling]

Comment on attachment 104041 [details]
Patch

r=me
Do exercise caution (with regard to the bots) when landing this.

[Csaba Osztrogonác]

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

[Ademar Reis]

Check also this upstream Qt bug:
https://bugreports.qt.nokia.com/browse/QTBUG-21036

[Csaba Osztrogonác]

Patch landed in:
http://trac.webkit.org/changeset/97990

~6000 rebased expected files landed in:
http://trac.webkit.org/changeset/97994
http://trac.webkit.org/changeset/97995
http://trac.webkit.org/changeset/98015
http://trac.webkit.org/changeset/98018
http://trac.webkit.org/changeset/98072
http://trac.webkit.org/changeset/98078