Bug 72513 - [Qt] Test fonts are not used with Qt5
Summary: [Qt] Test fonts are not used with Qt5
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Balazs Kelemen
URL:
Keywords: LayoutTestFailure, Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-11-16 09:13 PST by Balazs Kelemen
Modified: 2011-12-14 15:43 PST (History)
5 users (show)

See Also:


Attachments
Patch (10.32 KB, patch)
2011-11-17 04:16 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (10.43 KB, patch)
2011-11-24 06:21 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (10.14 KB, patch)
2011-11-24 08:53 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Balazs Kelemen 2011-11-16 09:13:03 PST
The fontconfig code in DRT and WTR is guarded with Q_WS_X11 which is not defined with Qt5. Blame that it has missed our attention... Likely we would have to rebaseline the world.
Comment 1 Balazs Kelemen 2011-11-17 04:16:53 PST
Created attachment 115557 [details]
Patch

Patch for feedback. Not dealing with rebaselining results which is definitely necessary.
Comment 2 Csaba Osztrogonác 2011-11-17 08:34:35 PST
Comment on attachment 115557 [details]
Patch

cq-, because we should check its impact to the the test results before landing.
Comment 3 Simon Hausmann 2011-11-17 12:25:10 PST
Comment on attachment 115557 [details]
Patch

Looks good, but I'm pretty sure this patch is going to break with Qt 4.8, because Qt 4 doesn't support the configure tests. (r-)

However I think the solution is easy: For Qt 4 we make Qt DRT linux/x11 only and just _assume_ (thus require) the presence of font-config, i.e. set the define and pkgconfig if haveQt(4) in the .pro file (or the test passes with qt5).
Comment 4 Balazs Kelemen 2011-11-24 06:21:14 PST
Created attachment 116502 [details]
Patch
Comment 5 Simon Hausmann 2011-11-24 07:15:43 PST
Comment on attachment 116502 [details]
Patch

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

> Tools/DumpRenderTree/qt/main.cpp:-256
> -#ifdef Q_WS_X11
> -    FcConfigSetCurrent(0);
> -#endif

Why are you removing this function call?

> Tools/WebKitTestRunner/Target.pri:38
> -!embedded: PKGCONFIG += fontconfig
> +contains(config_test_fontconfig, yes) {
> +    PKGCONFIG += fontconfig
> +}

Is this actually needed? Isn't it only the injected bundle that's using fontconfig? Perhaps we can remove the font-config linkage for WTR (the app) altogether?
Comment 6 Balazs Kelemen 2011-11-24 08:37:26 PST
(In reply to comment #5)
> (From update of attachment 116502 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116502&action=review
> 
> > Tools/DumpRenderTree/qt/main.cpp:-256
> > -#ifdef Q_WS_X11
> > -    FcConfigSetCurrent(0);
> > -#endif

That is just at the end of main. I don't think that has any purpose.
BTW, the WTR version don't has this.

> 
> Why are you removing this function call?
> 
> > Tools/WebKitTestRunner/Target.pri:38
> > -!embedded: PKGCONFIG += fontconfig
> > +contains(config_test_fontconfig, yes) {
> > +    PKGCONFIG += fontconfig
> > +}
> 
> Is this actually needed? Isn't it only the injected bundle that's using fontconfig? Perhaps we can remove the font-config linkage for WTR (the app) altogether?

You are right it's not needed for WTR, I'm going to remove it.
Comment 7 Balazs Kelemen 2011-11-24 08:53:21 PST
Created attachment 116518 [details]
Patch
Comment 8 Simon Hausmann 2011-11-25 00:20:51 PST
(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 116502 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=116502&action=review
> > 
> > > Tools/DumpRenderTree/qt/main.cpp:-256
> > > -#ifdef Q_WS_X11
> > > -    FcConfigSetCurrent(0);
> > > -#endif
> 
> That is just at the end of main. I don't think that has any purpose.
> BTW, the WTR version don't has this.

It was added to avoid a crash on exit. If that crash isn't reproducible anymore, then I'm fine with removing it. Have you experienced any (new?) crashes when running all the layout tests with that change?
Comment 9 Simon Hausmann 2011-11-25 04:10:06 PST
Comment on attachment 116518 [details]
Patch

r=me

(My concern about the FcSetCurrent(0) doesn't apply as you pointed out that the line comes after return app.exec() :-)
Comment 10 Csaba Osztrogonác 2011-11-25 06:29:41 PST
I tested this patch, it works fine on the qt-4.8 and qt-5.0 bots. 
But I got this strange crasher for all tests on my Ubuntu:

(gdb) bt
#0  0x031e0cc6 in ?? () from /lib/i386-linux-gnu/libc.so.6
#1  0x02d9e548 in QString::fromUtf8(char const*, int) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtCore.so.5
#2  0x04d5d468 in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/plugins/platforms/libxcb.so
#3  0x04d5d31a in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/plugins/platforms/libxcb.so
#4  0x0296509c in QGuiApplication::font() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtGui.so.5
#5  0x029f0efa in QFont::QFont() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtGui.so.5
#6  0x021a574f in QWidgetPrivate::QWidgetPrivate(int) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtWidgets.so.5
#7  0x021b508c in QWidget::QWidget(QWidget*, QFlags<Qt::WindowType>) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtWidgets.so.5
#8  0x08054576 in main ()

Have you got any idea what is it? (Unfortunately I have 32 bit Ubuntu and debug build is impossible on 32 bit.)
Comment 11 Simon Hausmann 2011-11-25 06:38:00 PST
(In reply to comment #10)
> I tested this patch, it works fine on the qt-4.8 and qt-5.0 bots. 
> But I got this strange crasher for all tests on my Ubuntu:
> 
> (gdb) bt
> #0  0x031e0cc6 in ?? () from /lib/i386-linux-gnu/libc.so.6
> #1  0x02d9e548 in QString::fromUtf8(char const*, int) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtCore.so.5
> #2  0x04d5d468 in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/plugins/platforms/libxcb.so
> #3  0x04d5d31a in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/plugins/platforms/libxcb.so
> #4  0x0296509c in QGuiApplication::font() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtGui.so.5
> #5  0x029f0efa in QFont::QFont() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtGui.so.5
> #6  0x021a574f in QWidgetPrivate::QWidgetPrivate(int) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtWidgets.so.5
> #7  0x021b508c in QWidget::QWidget(QWidget*, QFlags<Qt::WindowType>) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtWidgets.so.5
> #8  0x08054576 in main ()
> 
> Have you got any idea what is it? (Unfortunately I have 32 bit Ubuntu and debug build is impossible on 32 bit.)

Darn, that almost sounds like a widget being created before QApplication.
Comment 12 Csaba Osztrogonác 2011-11-25 06:52:07 PST
Simon suggested me to comment out the line "QApplication::setFont(QWidget().font());", but I still get similar crash:

(gdb) bt
#0  0x031e0cc6 in ?? () from /lib/i386-linux-gnu/libc.so.6
#1  0x02d9e548 in QString::fromUtf8(char const*, int) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtCore.so.5
#2  0x04d5d468 in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/plugins/platforms/libxcb.so
#3  0x04d5d31a in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/plugins/platforms/libxcb.so
#4  0x0296509c in QGuiApplication::font() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtGui.so.5
#5  0x029f0efa in QFont::QFont() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r13/lib/libQtGui.so.5
#6  0x002ecd41 in QWebSettings::QWebSettings() () from /storage/WebKit/WebKitBuild/Release/lib/libQtWebKit.so.4
#7  0x002ef0a6 in QWebSettings::globalSettings() () from /storage/WebKit/WebKitBuild/Release/lib/libQtWebKit.so.4
#8  0x002f1fd7 in QWebSettings::enablePersistentStorage(QString const&) ()
   from /storage/WebKit/WebKitBuild/Release/lib/libQtWebKit.so.4
#9  0x08057962 in WebCore::DumpRenderTree::DumpRenderTree() ()
#10 0x08054573 in main ()
Comment 13 Csaba Osztrogonác 2011-12-14 07:57:40 PST
Comment on attachment 116518 [details]
Patch

Landed in http://trac.webkit.org/changeset/102776.

It works fine on the buildbot, on Ubuntus in real hardware, on the Amazon image. It fails only my VirtualBox Ubuntu VM, but it doesn't a big problem now.
Comment 14 Csaba Osztrogonác 2011-12-14 10:40:10 PST
Unfortunately it broke the MIPS and SH4 build. :-/

I have no idea how to fix MIPS and SH4 ... I tried it,
but it wasn't good: http://trac.webkit.org/changeset/102795

I don't want to roll out this patch, because I unskipped thousends of tests for the Qt5 platform which is very important.

Holger, could you check it, please?
Comment 15 Simon Hausmann 2011-12-14 11:30:04 PST
(In reply to comment #14)
> Unfortunately it broke the MIPS and SH4 build. :-/
> 
> I have no idea how to fix MIPS and SH4 ... I tried it,
> but it wasn't good: http://trac.webkit.org/changeset/102795
> 
> I don't want to roll out this patch, because I unskipped thousends of tests for the Qt5 platform which is very important.
> 
> Holger, could you check it, please?

It probably just requires a clean rebuild due to the change in DEFINES.
Comment 16 Csaba Osztrogonác 2011-12-14 11:44:32 PST
Fix landed in http://trac.webkit.org/changeset/102806
Comment 17 Balazs Kelemen 2011-12-14 15:06:01 PST
(In reply to comment #16)
> Fix landed in http://trac.webkit.org/changeset/102806

Thanks for handling the build Ossy. Btw, I wonder why was this last one necessary. Is it about precedence? (I assumed AND is stronger than OR but maybe qmake is crazy enough to believe in different mathematics.)
Comment 18 Holger Freyther 2011-12-14 15:39:37 PST
(In reply to comment #17)
> (In reply to comment #16)
> > Fix landed in http://trac.webkit.org/changeset/102806
> 
> Thanks for handling the build Ossy. Btw, I wonder why was this last one necessary. Is it about precedence? (I assumed AND is stronger than OR but maybe qmake is crazy enough to believe in different mathematics.)

The motivation was that fontconfig has nothing to do with SH4/MIPS but is more about the configuration of these two bots (both target QWS and not X11). And AFAIR the default configuration of an embedded build will not use fontconfig?

Without looking much at the context, how will DRT directly link to -lfontconfig? Who manipulates the LIBS?
Comment 19 Balazs Kelemen 2011-12-14 15:43:56 PST
(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Fix landed in http://trac.webkit.org/changeset/102806
> > 
> > Thanks for handling the build Ossy. Btw, I wonder why was this last one necessary. Is it about precedence? (I assumed AND is stronger than OR but maybe qmake is crazy enough to believe in different mathematics.)
> 
> The motivation was that fontconfig has nothing to do with SH4/MIPS but is more about the configuration of these two bots (both target QWS and not X11). And AFAIR the default configuration of an embedded build will not use fontconfig?
> 
> Without looking much at the context, how will DRT directly link to -lfontconfig? Who manipulates the LIBS?

I was thinking about http://trac.webkit.org/changeset/102824, the very last one :) Sorry for the confusion.