Bug 29689 - [Layout tests] [Gtk] Gtk DumpRenderTree should use WebKit test fonts
Summary: [Layout tests] [Gtk] Gtk DumpRenderTree should use WebKit test fonts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-23 12:15 PDT by Zan Dobersek
Modified: 2009-10-07 09:24 PDT (History)
2 users (show)

See Also:


Attachments
Final patch, first attempt (3.93 KB, patch)
2009-09-23 12:38 PDT, Zan Dobersek
gns: review-
Details | Formatted Diff | Diff
Final patch, second attempt (12.91 KB, patch)
2009-09-26 07:44 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff
Build fix (948 bytes, patch)
2009-10-06 22:20 PDT, Shinichiro Hamaji
jmalonzo: review+
hamaji: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2009-09-23 12:15:15 PDT
DumpRenderTree should load test fonts through fontconfig at the start and before each test to insure proper fonts are used for testing. This insures a more proper rendering results.
Comment 1 Zan Dobersek 2009-09-23 12:38:41 PDT
Created attachment 40012 [details]
Final patch, first attempt

First attempt at final patch.

Mimics Qt's way of doing it.
Comment 2 Gustavo Noronha (kov) 2009-09-23 15:42:46 PDT
Comment on attachment 40012 [details]
Final patch, first attempt

> +    FcFontSet* appFontSet = FcConfigGetFonts(0, FcSetApplication);
> +    if (appFontSet && numFonts >= 0 && appFontSet->nfont == numFonts)
> +        return;

So, before reading the configuration this will return NULL or something? What if the user has a configuration already at their home?

>  dumprendertree_cppflags += \
> -	-DTEST_PLUGIN_DIR=\"${shell pwd}/${top_builddir}/TestNetscapePlugin/.libs\"
> +	-DTEST_PLUGIN_DIR=\"${shell pwd}/${top_builddir}/TestNetscapePlugin/.libs\" \
> +	-DFONTS_CONF_FILE=\"${shell pwd}/${srcdir}/WebKitTools/DumpRenderTree/qt/fonts.conf\"

I think we should have a copy of that configuration file in gtk/, instead of refering from the qt one, to avoid being hit by any changes they need to do.

Did you test if render tree dumps are matching or more closely matching the expected ones in Mac? =) I'll say r- till we get these two issues sorted out.
Comment 3 Zan Dobersek 2009-09-26 04:33:58 PDT
(In reply to comment #2)
> (From update of attachment 40012 [details])
> > +    FcFontSet* appFontSet = FcConfigGetFonts(0, FcSetApplication);
> > +    if (appFontSet && numFonts >= 0 && appFontSet->nfont == numFonts)
> > +        return;
> 
> So, before reading the configuration this will return NULL or something? What
> if the user has a configuration already at their home?

This sees if any fonts were added or removed via CSS @font-face rule. It checks if the number of fonts that the current config (which was created and made default, 'current', when initializeFonts() was called for the first time) contains the exact same number as when it was created.
Should've added a comment explaining why that part of code is there.

> Did you test if render tree dumps are matching or more closely matching the
> expected ones in Mac? =)

Ignoring all the fonts problems, I'll start creating bugs for each of the directories that contain tests with no generated results. Patches with these results and diff between mac's and gtk's expected results will be uploaded to clarify things. If there are any large differences between expected results, the generated results will not be added to the patch, a bug will be created about the problem and the latter will be examined thoroughly to see what's wrong.

I'm also planning to add support for pixel tests, which will bring a better overview to changes and help out with examining rendering differences.

Regards,
Zan Dobersek
Comment 4 Zan Dobersek 2009-09-26 07:44:23 PDT
Created attachment 40172 [details]
Final patch, second attempt

Adds a comment about why we check the number of fonts, adds our own copy of fonts.conf.
Comment 5 WebKit Commit Bot 2009-09-26 10:51:24 PDT
Comment on attachment 40172 [details]
Final patch, second attempt

Clearing flags on attachment: 40172

Committed r48791: <http://trac.webkit.org/changeset/48791>
Comment 6 WebKit Commit Bot 2009-09-26 10:51:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Shinichiro Hamaji 2009-10-06 22:20:44 PDT
Created attachment 40763 [details]
Build fix
Comment 8 Shinichiro Hamaji 2009-10-06 22:21:07 PDT
(In reply to comment #7)
> Created an attachment (id=40763) [details]
> Build fix

It seems we need to add -lfontconfig ?
Comment 9 Jan Alonzo 2009-10-07 04:09:09 PDT
Comment on attachment 40763 [details]
Build fix

r=me.
Comment 10 Shinichiro Hamaji 2009-10-07 09:24:18 PDT
Committed r49246: <http://trac.webkit.org/changeset/49246>