Bug 76068 – [Qt] Cleanup testfonts handling

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Balazs Kelemen]

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

[Balazs Kelemen]

Created attachment 122047 [details]
Patch

[Balazs Kelemen]

Comment on attachment 122047 [details]
Patch

cq- because first we will need to update the fonts.conf file in the testfonts repository

[Simon Hausmann]

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?

[Balazs Kelemen]

Created attachment 122653 [details]
Patch

[Balazs Kelemen]

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

[WebKit Review Bot]

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.

[Balazs Kelemen]

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

[Alexis Menard (darktears)]

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.

[Balazs Kelemen]

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

[Simon Hausmann]

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.

[Balazs Kelemen]

Created attachment 122903 [details]
Patch

added new files to Tools/DumpRenderTree/qt

[WebKit Review Bot]

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.

[Simon Hausmann]

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

[Balazs Kelemen]

(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