Bug 34959 – [Qt] QtLauncher does not load the same set of fonts as the DRT

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

fast/multicol/client-rects.html is a new test introduced in http://trac.webkit.org/changeset/54784 .

It seems that this test passes, because the pixel result is similar to 
expected result. But surprisingly fast/multicol/client-rects.html is 
displayed incorrectly in QtLauncher. (png files attached)

[Csaba Osztrogonác]

Created attachment 48773 [details]
expected png

[Csaba Osztrogonác]

Created attachment 48774 [details]
actual png

[Csaba Osztrogonác]

Created attachment 48775 [details]
actual png by QtLauncher

[Csaba Osztrogonác]

fast/multicol/client-rects.html skipped by http://trac.webkit.org/changeset/54793 until fix.

The actual PNG from QtLauncher shows that you don’t have the Ahem font in QtLauncher. The actual PNG from DumpRenderTree shows that it is using a different standard font (for the description text). I guess this is what makes Qt require platform-specific results for so many tests. Seems like this is one of those tests.

[Csaba Osztrogonác]

Created attachment 48888 [details]
proposed patch - add Qt specific expected file

I installed Ahem font for QtLauncher, now it works correctly,
I get same result by QtLauncher as by DumpRenderTree.

I compared actual rendertree dump with expected file, they 
are same except metrics. (run-webkit-tests --ignore-metrics)

[Csaba Osztrogonác]

Created attachment 48891 [details]
proposed patch - add Qt specific expected file

Remove fast/multicol/client-rects.html from skiplist. (I missed it in my previous patch.)

[Ariya Hidayat]

> I installed Ahem font for QtLauncher, now it works correctly,
> I get same result by QtLauncher as by DumpRenderTree.

Don't we need to add this info the wiki page, too?

[Andras Becsi]

(In reply to comment #8)
> > I installed Ahem font for QtLauncher, now it works correctly,
> > I get same result by QtLauncher as by DumpRenderTree.
> 
> Don't we need to add this info the wiki page, too?
It probably would be a good idea, maybe even implement a similar loading mechanism for WEBKIT_TEST_FONTS in QtLauncher which DRT uses wouldn't be a bad idea either.

[Tor Arne Vestbø]

(In reply to comment #9)
> (In reply to comment #8)
> > > I installed Ahem font for QtLauncher, now it works correctly,
> > > I get same result by QtLauncher as by DumpRenderTree.
> > 
> > Don't we need to add this info the wiki page, too?
> It probably would be a good idea, maybe even implement a similar loading
> mechanism for WEBKIT_TEST_FONTS in QtLauncher which DRT uses wouldn't be a bad
> idea either.

I'm working on this as part of the SVG-fonts, basically you run the QtLauncher with --like-drt

But I've hit a few bumbs when trying to rebase the SVG-fonts patch, I'm working o that now, let's see how it goes.

[Andras Becsi]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > > I installed Ahem font for QtLauncher, now it works correctly,
> > > > I get same result by QtLauncher as by DumpRenderTree.
> > > 
> > > Don't we need to add this info the wiki page, too?
> > It probably would be a good idea, maybe even implement a similar loading
> > mechanism for WEBKIT_TEST_FONTS in QtLauncher which DRT uses wouldn't be a bad
> > idea either.
> 
> I'm working on this as part of the SVG-fonts, basically you run the QtLauncher
> with --like-drt
> 
> But I've hit a few bumbs when trying to rebase the SVG-fonts patch, I'm working
> o that now, let's see how it goes.

That's really cool. Thanks.

[Kenneth Rohde Christiansen]

> > I'm working on this as part of the SVG-fonts, basically you run the QtLauncher
> > with --like-drt

As this is not going forward, I guess we should all install that font as well. Could it be part of our font package?

I will review the patch assuming that you have this font on the buildbot.

Could you please update with info on how to get the font and email our webkit-qt mailing list with teh info?

[Csaba Osztrogonác]

(In reply to comment #12)
> As this is not going forward, I guess we should all install that font as well.
> Could it be part of our font package?
> 
> I will review the patch assuming that you have this font on the buildbot.
> 
> Could you please update with info on how to get the font and email our
> webkit-qt mailing list with teh info?

Ahem font is part of our test font package. ( git://gitorious.org/qtwebkit/testfonts.git ) The problem is that DRT uses these fonts, but QtLauncher doesn't do it. I agree with Comment #9 of Andras, we should make QtLauncher to be able to use WEBKIT_TEST_FONTS environment variable. (Or everyone should install testfonts into ~/.fonts too)

[Csaba Osztrogonác]

Comment on attachment 48891 [details]
proposed patch - add Qt specific expected file

Patch landed in http://trac.webkit.org/changeset/55281

[Zoltan Horvath]

(In reply to comment #14)
> (From update of attachment 48891 [details])
> Patch landed in http://trac.webkit.org/changeset/55281

What is the status of the bug now?

[Balazs Kelemen]

Maybe I'm not familiar enough with this but I do not see what is the big deal.
We should simply copy-paste the font configuration hack from drt to 
QtTestBrowser, or make it acceccible from it. That would work on linux at least.

[Balazs Kelemen]

(In reply to comment #16)
> Maybe I'm not familiar enough with this but I do not see what is the big deal.
> We should simply copy-paste the font configuration hack from drt to 
> QtTestBrowser, or make it acceccible from it. That would work on linux at least.

Take this as a question. Is this just that easy?

[Csaba Osztrogonác]

(In reply to comment #15)
> What is the status of the bug now?

It seems nobody fixed it in the previous year ... :(

Any volunteer?

[Joe Wild]

I'll try my hand at this one.  You can assign it to me if you like.

If I understand this correctly, I just need to move/copy the font
loading code from DumpRenderTreeQt.cpp that reads in the fonts from
WEBKIT_TESTFONTS into the QtTestBrowser.  This code should be
projected with an option.  I would suggest -webkit-testfonts or
-load-webkit-testfonts which seem consistent with the other QtTestBrowser options.

One thing I need to investigate too is that DumpRenderTree loads different
font.conf files for different platforms.  This file seems to specify font name mappings.  Anyone know if this would be also needed in the QtTestBrowser.

Comments and suggestions welcome.

[Balazs Kelemen]

(In reply to comment #19)
> I'll try my hand at this one.  You can assign it to me if you like.
> 
> If I understand this correctly, I just need to move/copy the font
> loading code from DumpRenderTreeQt.cpp that reads in the fonts from
> WEBKIT_TESTFONTS into the QtTestBrowser.  This code should be
> projected with an option.  I would suggest -webkit-testfonts or
> -load-webkit-testfonts which seem consistent with the other QtTestBrowser options.

Yep, it's just that. -webkit-testfonts seems to be fine.

> 
> One thing I need to investigate too is that DumpRenderTree loads different
> font.conf files for different platforms.  This file seems to specify font name mappings.  Anyone know if this would be also needed in the QtTestBrowser.
> 

It loads the same checked in config file for every platform: configFile += "/Tools/DumpRenderTree/qt/fonts.conf";
I do not know the role of that file but I guess it is also needed in QTB to get correct visual
result for every test. By the way the approach we use in DRT depends on fontconfig so it is not working
on every platform. You should guard the whole thing with an appropriate ifdef condition that is true on platforms
where fontconfig is presented.

[Joe Wild]

Created attachment 84867 [details]
This patch adds the option "-webkit-testfonts" to the QtTestBrowser.


This patch adds the option "-webkit-testfonts" to the QtTestBrowser.
When this option is used the webkit fonts are loaded the same
as they are in DumpRenderTree.  This option can be used on
QtTestBrowser and run-launcher.  It can only be used
on Linux systems with FcInit and is configured as such.

I have a couple of concerns with my changes.  Since I am reusing
the code from DumpRenderTree, I'm not sure of the best way to share
it.

I basically duplicated WebCore:DumpRenderTree::initializeFonts() from
in the QtTestBrowser since I did not want to reference files across
these separate components.

However, when initalizing the fonts, I am reading fonts.conf
from Tools/DumpRenderTree/qt/ since I did not want us to have
to maintain 2 copies of this file.

Finally, I changed the lookup of the fonts.conf to be relative from
the location of the QtTestBrowser executable instead of from the
WebKit top level directory as it does DumpRenderTree.  The main reason
for this is that run-launcher does not do a chdirWebKit() to the top
level directory, like run-webkit-tests does.  I'm not sure if this was
the right decision, but I did not want to change the behavior of the
run-launcher script.

I did not see any test or docs (other than QtTestBrowser -help) that
I needed to update.

Input welcome.

[WebKit Review Bot]

Attachment 84867 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/QtTestBrowser/la..." exit_code: 1

Tools/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joe Wild]

Created attachment 84868 [details]
Fix tab in source from previous patch.patch adds the option "-webkit-testfonts" to the QtTestBrowser.

My mistake.  Didn't run check-webkit-style again after creating the change log.
Fixed.

[Kenneth Rohde Christiansen]

Comment on attachment 84868 [details]
Fix tab in source from previous patch.patch adds the option "-webkit-testfonts" to the QtTestBrowser.

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

> Tools/ChangeLog:8
> +        This patch adds the option "-webkit-testfonts" to the QtTestBrowser.

-test-fonts would do

> Tools/QtTestBrowser/main.cpp:54
> +#if defined(Q_WS_X11)
> +// Very similar to WebCore::DumpRenderTree::initializeFonts();
> +// Duplicated here so that QtTestBrowser would display contents
> +// with the same fonts as run-webkit-tests/DumpRenderTree.
> +static void initWebKitTestFonts()

Can't this be in DumpRenderTreeSupport or what the class is called?

[Kenneth Rohde Christiansen]

Maybe -use-test-fonts

[Joe Wild]

On Comment #24 + #25.

Kenneth,

Thank you for you comments.

As far as option name, I chose -webkit-testfonts because it seemed
reasonably consistent the other QtTestBrower options and has been
referred to as "WebKit Test Fonts" already in DumpRenderTree.  Environment
variable of where to load these fonts in named "WEBKIT_TESTFONTS". 
I can change the option name to anything we agree on.
"-use-test-fonts" is also file with me.

As far as DumpRenderTreeSupport under
Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.h/cpp, it
looks like this exposes WebKit info to DumpRenderTree.  I don't see a
way to tunnel to Tools/DumpRenderTree/qt/DumpRenderTreeQt.h/cpp
initalizeFonts() from QtTestBrowser through DumpRenderTreeeSupportQt.
Please feel free to correct me if I'm wrong.

So I think we have to choose between duplicating initilizeFonts() and
referencing froms from DumpRenderTree in QtTestBrowser.

Joe

[Joe Wild]

Created attachment 85086 [details]
Add the option "-use-test-fonts" to the QtTestBrowser to use same fonts as DumpRenderTree. 

I changed the option name to -use-test-fonts.

However, I left the duplication of 
WebCore:DumpRenderTree::initializeFonts() in the QtTestBrowser 
since I did not see how to share it through DumpRenderTreeSupportQt. 
I am open to suggestions.

[Alexis Menard (darktears)]

Comment on attachment 85086 [details]
Add the option "-use-test-fonts" to the QtTestBrowser to use same fonts as DumpRenderTree. 

LGTM

[Laszlo Gombos]

Comment on attachment 85086 [details]
Add the option "-use-test-fonts" to the QtTestBrowser to use same fonts as DumpRenderTree. 

Joe, if you'd like your patch committed you should set the commit-queue to "?" as well.

I cq+ this for Joe.

[WebKit Commit Bot]

Comment on attachment 85086 [details]
Add the option "-use-test-fonts" to the QtTestBrowser to use same fonts as DumpRenderTree. 

Rejecting attachment 85086 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'la..." exit_code: 2

Last 500 characters of output:
://git.webkit.org/WebKit
   2d7275a..8f26e14  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 80779 = 2d7275ac45644916eec7b5d4c746648445ce37d5
r80780 = 8f26e14cd3bfd8c4428e938ae8c245038f69fc88
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://queues.webkit.org/results/8139063

[Joe Wild]

Created attachment 85475 [details]
Rebase and resubmit: Add the option "-use-test-fonts" to the QtTestBrowser to use same fonts as DumpRenderTree. 

This patch passed review and then failed to apply. Rebased the patch and resubmitted.

[WebKit Commit Bot]

Comment on attachment 85475 [details]
Rebase and resubmit: Add the option "-use-test-fonts" to the QtTestBrowser to use same fonts as DumpRenderTree. 

Clearing flags on attachment: 85475

Committed r80977: <http://trac.webkit.org/changeset/80977>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/80977 might have broken SnowLeopard Intel Release (Build)

[Balazs Kelemen]

This breaks the "Qt ARMv7 Linux Release" bot: https://bugs.webkit.org/show_bug.cgi?id=56349