Bug 76068 - [Qt] Cleanup testfonts handling
Summary: [Qt] Cleanup testfonts handling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P3 Normal
Assignee: Balazs Kelemen
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2012-01-11 09:21 PST by Balazs Kelemen
Modified: 2012-01-25 07:06 PST (History)
2 users (show)

See Also:


Attachments
Patch (27.85 KB, patch)
2012-01-11 10:27 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (33.33 KB, patch)
2012-01-16 09:17 PST, Balazs Kelemen
no flags Details | Formatted Diff | Diff
Patch (31.88 KB, patch)
2012-01-18 05:26 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 2012-01-11 09:21:37 PST
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
Comment 1 Balazs Kelemen 2012-01-11 10:27:59 PST
Created attachment 122047 [details]
Patch
Comment 2 Balazs Kelemen 2012-01-11 10:29:56 PST
Comment on attachment 122047 [details]
Patch

cq- because first we will need to update the fonts.conf file in the testfonts repository
Comment 3 Simon Hausmann 2012-01-16 01:18:36 PST
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?
Comment 4 Balazs Kelemen 2012-01-16 09:17:32 PST
Created attachment 122653 [details]
Patch
Comment 5 Balazs Kelemen 2012-01-16 09:19:47 PST
(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.
Comment 6 WebKit Review Bot 2012-01-16 09:20:31 PST
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.
Comment 7 Balazs Kelemen 2012-01-16 09:24:01 PST
(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 8 Alexis Menard (darktears) 2012-01-16 10:30:04 PST
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.
Comment 9 Balazs Kelemen 2012-01-16 10:32:03 PST
(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 10 Simon Hausmann 2012-01-17 11:13:09 PST
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.
Comment 11 Balazs Kelemen 2012-01-18 05:26:26 PST
Created attachment 122903 [details]
Patch

added new files to Tools/DumpRenderTree/qt
Comment 12 WebKit Review Bot 2012-01-18 05:29:41 PST
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 13 Simon Hausmann 2012-01-23 12:53:09 PST
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".
Comment 14 Balazs Kelemen 2012-01-25 07:04:04 PST
(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