Bug 29689

Summary: [Layout tests] [Gtk] Gtk DumpRenderTree should use WebKit test fonts
Product: WebKit Reporter: Zan Dobersek <zan>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hamaji
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Final patch, first attempt
gustavo: review-
Final patch, second attempt
none
Build fix jmalonzo: review+, hamaji: commit-queue+

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>