Bug 72513

Created attachment 115557 [details] Patch Patch for feedback. Not dealing with rebaselining results which is definitely necessary.

Comment on attachment 115557 [details] Patch cq-, because we should check its impact to the the test results before landing.

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

Created attachment 116502 [details] Patch

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?

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

Created attachment 116518 [details] Patch

(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 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() :-)

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

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

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

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?

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

Fix landed in http://trac.webkit.org/changeset/102806

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

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

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