Bug 34959 - [Qt] QtLauncher does not load the same set of fonts as the DRT
Summary: [Qt] QtLauncher does not load the same set of fonts as the DRT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P3 Normal
Assignee: Joe Wild
URL:
Keywords: LayoutTestFailure, Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2010-02-15 13:24 PST by Csaba Osztrogonác
Modified: 2011-03-14 18:04 PDT (History)
14 users (show)

See Also:


Attachments
expected png (18.08 KB, image/png)
2010-02-15 13:26 PST, Csaba Osztrogonác
no flags Details
actual png (9.17 KB, image/png)
2010-02-15 13:26 PST, Csaba Osztrogonác
no flags Details
actual png by QtLauncher (11.65 KB, image/png)
2010-02-15 13:27 PST, Csaba Osztrogonác
no flags Details
proposed patch - add Qt specific expected file (6.37 KB, patch)
2010-02-17 04:09 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
proposed patch - add Qt specific expected file (7.17 KB, patch)
2010-02-17 04:23 PST, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
This patch adds the option "-webkit-testfonts" to the QtTestBrowser. (4.94 KB, patch)
2011-03-05 08:31 PST, Joe Wild
no flags Details | Formatted Diff | Diff
Fix tab in source from previous patch.patch adds the option "-webkit-testfonts" to the QtTestBrowser. (4.94 KB, patch)
2011-03-05 09:04 PST, Joe Wild
no flags Details | Formatted Diff | Diff
Add the option "-use-test-fonts" to the QtTestBrowser to use same fonts as DumpRenderTree. (3.87 KB, patch)
2011-03-08 13:36 PST, Joe Wild
kenneth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Rebase and resubmit: Add the option "-use-test-fonts" to the QtTestBrowser to use same fonts as DumpRenderTree. (4.88 KB, patch)
2011-03-11 07:40 PST, Joe Wild
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2010-02-15 13:24:24 PST
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)
Comment 1 Csaba Osztrogonác 2010-02-15 13:26:25 PST
Created attachment 48773 [details]
expected png
Comment 2 Csaba Osztrogonác 2010-02-15 13:26:49 PST
Created attachment 48774 [details]
actual png
Comment 3 Csaba Osztrogonác 2010-02-15 13:27:15 PST
Created attachment 48775 [details]
actual png by QtLauncher
Comment 4 Csaba Osztrogonác 2010-02-15 13:39:10 PST
fast/multicol/client-rects.html skipped by http://trac.webkit.org/changeset/54793 until fix.
Comment 5 mitz 2010-02-16 07:32:13 PST
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.
Comment 6 Csaba Osztrogonác 2010-02-17 04:09:18 PST
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)
Comment 7 Csaba Osztrogonác 2010-02-17 04:23:39 PST
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.)
Comment 8 Ariya Hidayat 2010-02-17 10:01:54 PST
> 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?
Comment 9 Andras Becsi 2010-02-17 11:16:11 PST
(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.
Comment 10 Tor Arne Vestbø 2010-02-18 07:48:38 PST
(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.
Comment 11 Andras Becsi 2010-02-18 07:51:11 PST
(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.
Comment 12 Kenneth Rohde Christiansen 2010-02-26 04:57:47 PST
> > 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?
Comment 13 Csaba Osztrogonác 2010-02-26 07:26:10 PST
(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)
Comment 14 Csaba Osztrogonác 2010-02-26 07:26:36 PST
Comment on attachment 48891 [details]
proposed patch - add Qt specific expected file

Patch landed in http://trac.webkit.org/changeset/55281
Comment 15 Zoltan Horvath 2011-02-25 00:51:14 PST
(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?
Comment 16 Balazs Kelemen 2011-02-25 05:10:16 PST
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.
Comment 17 Balazs Kelemen 2011-02-25 05:11:32 PST
(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?
Comment 18 Csaba Osztrogonác 2011-02-25 05:19:09 PST
(In reply to comment #15)
> What is the status of the bug now?

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

Any volunteer?
Comment 19 Joe Wild 2011-03-02 15:23:17 PST
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.
Comment 20 Balazs Kelemen 2011-03-02 16:13:37 PST
(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.
Comment 21 Joe Wild 2011-03-05 08:31:08 PST
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.
Comment 22 WebKit Review Bot 2011-03-05 08:32:20 PST
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.
Comment 23 Joe Wild 2011-03-05 09:04:47 PST
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.
Comment 24 Kenneth Rohde Christiansen 2011-03-06 05:37:54 PST
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?
Comment 25 Kenneth Rohde Christiansen 2011-03-06 05:38:22 PST
Maybe -use-test-fonts
Comment 26 Joe Wild 2011-03-07 10:17:49 PST
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
Comment 27 Joe Wild 2011-03-08 13:36:10 PST
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.
Comment 28 Alexis Menard (darktears) 2011-03-10 11:35:47 PST
Comment on attachment 85086 [details]
Add the option "-use-test-fonts" to the QtTestBrowser to use same fonts as DumpRenderTree. 

LGTM
Comment 29 Laszlo Gombos 2011-03-10 15:21:48 PST
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.
Comment 30 WebKit Commit Bot 2011-03-10 16:02:11 PST
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
Comment 31 Joe Wild 2011-03-11 07:40:32 PST
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.
Comment 32 WebKit Commit Bot 2011-03-13 17:15:45 PDT
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>
Comment 33 WebKit Commit Bot 2011-03-13 17:15:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 WebKit Review Bot 2011-03-13 18:54:19 PDT
http://trac.webkit.org/changeset/80977 might have broken SnowLeopard Intel Release (Build)
Comment 35 Balazs Kelemen 2011-03-14 18:04:40 PDT
This breaks the "Qt ARMv7 Linux Release" bot: https://bugs.webkit.org/show_bug.cgi?id=56349