I have two purpose with this: - remove code duplication (now we have the same code 3x) - fix use-test-fonts option of QtTestBrowser which is broken
Created attachment 122047 [details] Patch
Comment on attachment 122047 [details] Patch cq- because first we will need to update the fonts.conf file in the testfonts repository
Comment on attachment 122047 [details] Patch I'm a big fan of this cleanup (less duplicated code, no fonts.conf duplicated), but I'm not fond of linking WebKit against font-config by default. It's unnecessary linkage for production builds. Would it perhaps make sense to have a static helper library with DRT code that is shared between wk1 and wk2, that could be linked into the actual DRT programs?
Created attachment 122653 [details] Patch
(In reply to comment #3) > (From update of attachment 122047 [details]) > I'm a big fan of this cleanup (less duplicated code, no fonts.conf duplicated), but I'm not fond of linking WebKit against font-config by default. It's unnecessary linkage for production builds. > > Would it perhaps make sense to have a static helper library with DRT code that is shared between wk1 and wk2, that could be linked into the actual DRT programs? Since it' just one function and it should not grow in the future I just added the cpp into WebCore and reused the file in the particular projects.
Attachment 122653 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/qt/QtInitializeTestFonts.cpp:29: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #6) > Attachment 122653 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 > > Source/WebCore/platform/qt/QtInitializeTestFonts.cpp:29: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] > Total errors found: 1 in 11 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Unfortunately the new approach is still not compatible with the style because I could not use WebCore's config.h in the projects that use the file.
Comment on attachment 122653 [details] Patch I have a quick question: Does that solve the problem of shadow build of WebKit and running WebKitTestRunner that way : darktears@DarthVader:~/dev/troll/webkit2-trunk/Release/bin$ ./WebKitTestRunner --verbose /home/darktears/dev/troll/webkit/LayoutTests/fast/css/getComputedStyle/getComputedStyle-border-image.html He complains my font.conf file doesn't exist because it tries to look for `pwd`/Tools/DumpRenderTree/qt/fonts.conf which If I run it from the shadow build doesn't exist.
(In reply to comment #8) > (From update of attachment 122653 [details]) > I have a quick question: > Does that solve the problem of shadow build of WebKit and running WebKitTestRunner that way : > darktears@DarthVader:~/dev/troll/webkit2-trunk/Release/bin$ ./WebKitTestRunner --verbose /home/darktears/dev/troll/webkit/LayoutTests/fast/css/getComputedStyle/getComputedStyle-border-image.html > > He complains my font.conf file doesn't exist because it tries to look for `pwd`/Tools/DumpRenderTree/qt/fonts.conf which If I run it from the shadow build doesn't exist. Yes, it does :)
Comment on attachment 122653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122653&action=review > Tools/DumpRenderTree/qt/DumpRenderTree.pro:30 > + $${ROOT_WEBKIT_DIR}/Source/WebCore/platform/qt/QtInitializeTestFonts.h \ I would prefer this code to live in Tools/DumpRenderTree/Qt or in WTR, but not in WebCore. It doesn't have anything to do with WebCore AFAICS, being only testing related.
Created attachment 122903 [details] Patch added new files to Tools/DumpRenderTree/qt
Attachment 122903 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/DumpRenderTree/q..." exit_code: 1 Tools/DumpRenderTree/qt/QtInitializeTestFonts.cpp:29: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 122903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122903&action=review > Tools/DumpRenderTree/qt/QtInitializeTestFonts.h:32 > +namespace WebCore { I think WebKit would be a more appropriate namespace :) > Tools/QtTestBrowser/main.cpp:245 > -#if defined(Q_WS_X11) > +#if defined(HAVE_FONTCONFIG) && HAVE_FONTCONFIG Do you need the #ifdefs here? It looks to me that initializeTestFonts() has them in the function body already and the declaration is "fontconfig-free".
(In reply to comment #13) > (From update of attachment 122903 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122903&action=review > > > Tools/DumpRenderTree/qt/QtInitializeTestFonts.h:32 > > +namespace WebCore { > > I think WebKit would be a more appropriate namespace :) Fixed. > > > Tools/QtTestBrowser/main.cpp:245 > > -#if defined(Q_WS_X11) > > +#if defined(HAVE_FONTCONFIG) && HAVE_FONTCONFIG > > Do you need the #ifdefs here? It looks to me that initializeTestFonts() has them in the function body already and the declaration is "fontconfig-free". Fixed. Landed in http://trac.webkit.org/changeset/105876