Bug 126723 – [EFL] Change test font installed path to webkitgtk-font-tests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[ChangSeok Oh]

I'd like to bump up webkitgtk-font-tests to 0.0.4 and accordingly change the installed directory to 'webkitgtk-font-tests' like gtk port does.
I think it would help to reduce maintenance efforts for both gtk and efl ports.

[ChangSeok Oh]

Created attachment 220775 [details]
Patch

[Gyuyoung Kim]

Isn't there any regression on EFL layout test ? We SHOULD check if there is any layout test failure in EFL layout test before updating font.

[ChangSeok Oh]

(In reply to comment #2)
> Isn't there any regression on EFL layout test ? We SHOULD check if there is any layout test failure in EFL layout test before updating font.

I don't think this change makes anything worse and actually it did.

[Gyuyoung Kim]

(In reply to comment #3)
> (In reply to comment #2)
> > Isn't there any regression on EFL layout test ? We SHOULD check if there is any layout test failure in EFL layout test before updating font.
> 
> I don't think this change makes anything worse and actually it did.

I mean we need to make sure if there isn't any failure when using 0.4 ver. instead of 0.3. Did you run whole EFL layout test with 0.0.4 ver. ? Isn't there any additional failure between with 0.3 and 0.4 when you run whole EFL layout test ? As you may know, test font affects layout test result.

[ChangSeok Oh]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Isn't there any regression on EFL layout test ? We SHOULD check if there is any layout test failure in EFL layout test before updating font.
> > 
> > I don't think this change makes anything worse and actually it did.
> 
> I mean we need to make sure if there isn't any failure when using 0.4 ver. instead of 0.3. Did you run whole EFL layout test with 0.0.4 ver. ? Isn't there any additional failure between with 0.3 and 0.4 when you run whole EFL layout test ? As you may know, test font affects layout test result.

Doesn't efl bot check regressions? If this hurts something, it doesn't make sense that efl EWS bots show green. Bumping webkitgtk-test-fonts up doesn't mean changing the test font itself. Saying over, I believe nothing hurt. If you find something wrong, you can simply revert it.

BEFORE : 25705 tests ran as expected, 6832 didn't
AFTER : 25705 tests ran as expected, 6829 didn't

[ChangSeok Oh]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Isn't there any regression on EFL layout test ? We SHOULD check if there is any layout test failure in EFL layout test before updating font.
> > 
> > I don't think this change makes anything worse and actually it did.
> 
> I mean we need to make sure if there isn't any failure when using 0.4 ver. instead of 0.3. Did you run whole EFL layout test with 0.0.4 ver. ? Isn't there any additional failure between with 0.3 and 0.4 when you run whole EFL layout test ? As you may know, test font affects layout test result.

Well. if bumping the version makes you be concerned, I don't mind if this bug deals with only changing the installed path from webkitgtk-test-fonts-0.0.3 to webkitgtk-test-fonts. Different path name is really cumbersome when switching between gtk and efl...

[Gyuyoung Kim]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > Isn't there any regression on EFL layout test ? We SHOULD check if there is any layout test failure in EFL layout test before updating font.
> > > 
> > > I don't think this change makes anything worse and actually it did.
> > 
> > I mean we need to make sure if there isn't any failure when using 0.4 ver. instead of 0.3. Did you run whole EFL layout test with 0.0.4 ver. ? Isn't there any additional failure between with 0.3 and 0.4 when you run whole EFL layout test ? As you may know, test font affects layout test result.
> 
> Doesn't efl bot check regressions? If this hurts something, it doesn't make sense that efl EWS bots show green. Bumping webkitgtk-test-fonts up doesn't mean changing the test font itself. Saying over, I believe nothing hurt. If you find something wrong, you can simply revert it.

The layout test on buildbot exists to find regression whenever landing a commit. However, we SHOULD not land any patch if we find or know any possibility that can make any fault or regression. In this case, I think we should check if this test font can influence on EFL layout test. Because we're able to be enough to suspect this change can influence on EFL layout test. That's why I concerned this patch.


> BEFORE : 25705 tests ran as expected, 6832 didn't
> AFTER : 25705 tests ran as expected, 6829 didn't

Test result looks good.

[Gyuyoung Kim]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > Isn't there any regression on EFL layout test ? We SHOULD check if there is any layout test failure in EFL layout test before updating font.
> > > 
> > > I don't think this change makes anything worse and actually it did.
> > 
> > I mean we need to make sure if there isn't any failure when using 0.4 ver. instead of 0.3. Did you run whole EFL layout test with 0.0.4 ver. ? Isn't there any additional failure between with 0.3 and 0.4 when you run whole EFL layout test ? As you may know, test font affects layout test result.
> 
> Well. if bumping the version makes you be concerned, 

I didn't concern to bump test font. I just concerned that we bump the test font without testing whole EFL layout test.

[ChangSeok Oh]

Created attachment 221124 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 221124 [details]
Patch

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

> Tools/ChangeLog:9
> +        to 'webkitgtk-test-fonts' like gtk port. I believe it would help to reduce

I don't see why this can reduce maintenance cost. How ? 
I'm starting to get frustrated with these patch. You don't seem to understand what we're doing here.

[ChangSeok Oh]

Comment on attachment 221124 [details]
Patch

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

>> Tools/ChangeLog:9
>> +        to 'webkitgtk-test-fonts' like gtk port. I believe it would help to reduce
> 
> I don't see why this can reduce maintenance cost. How ? 
> I'm starting to get frustrated with these patch. You don't seem to understand what we're doing here.

Because we have to reset webkit test fonts path to fit that of each ports everytime when switching them, though efl and gtk use same source. Indeed, I think using webkitgtk-test-fonts-0.0.3 is wrong since DumpRenderTree and  efl is looking for the font set in "webkitgtk-test-fonts" which is hard coded. See https://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/efl/FontManagement.cpp#L105

[Gyuyoung Kim]

Comment on attachment 221124 [details]
Patch

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

>>> Tools/ChangeLog:9
>>> +        to 'webkitgtk-test-fonts' like gtk port. I believe it would help to reduce
>> 
>> I don't see why this can reduce maintenance cost. How ? 
>> I'm starting to get frustrated with these patch. You don't seem to understand what we're doing here.
> 
> Because we have to reset webkit test fonts path to fit that of each ports everytime when switching them, though efl and gtk use same source. Indeed, I think using webkitgtk-test-fonts-0.0.3 is wrong since DumpRenderTree and  efl is looking for the font set in "webkitgtk-test-fonts" which is hard coded. See https://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/efl/FontManagement.cpp#L105

My stance focuses on maintenance cost. I don't think we can share the test font when switching between efl and gtk. Because, IIRC, we can't build EFL(or GTK port) port by only using "--update-efl" option without removing "WebKitBuild/Dependencies of GTK". There were some conflicts. So, we need to download dependent libraries(the test font is also included the download list) again when switching from GTK port. That's why I think there is no benefit to reduce maintenance cost. (If it is possible, I wouldn't prefer to keep each port dependencies in same directory.) I don't mind to rename the test font directory. But, I don't agree there will be big benefit from the maintenance cost perspective.

[ChangSeok Oh]

Comment on attachment 221124 [details]
Patch

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

>>>> Tools/ChangeLog:9
>>>> +        to 'webkitgtk-test-fonts' like gtk port. I believe it would help to reduce
>>> 
>>> I don't see why this can reduce maintenance cost. How ? 
>>> I'm starting to get frustrated with these patch. You don't seem to understand what we're doing here.
>> 
>> Because we have to reset webkit test fonts path to fit that of each ports everytime when switching them, though efl and gtk use same source. Indeed, I think using webkitgtk-test-fonts-0.0.3 is wrong since DumpRenderTree and  efl is looking for the font set in "webkitgtk-test-fonts" which is hard coded. See https://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/efl/FontManagement.cpp#L105
> 
> My stance focuses on maintenance cost. I don't think we can share the test font when switching between efl and gtk. Because, IIRC, we can't build EFL(or GTK port) port by only using "--update-efl" option without removing "WebKitBuild/Dependencies of GTK". There were some conflicts. So, we need to download dependent libraries(the test font is also included the download list) again when switching from GTK port. That's why I think there is no benefit to reduce maintenance cost. (If it is possible, I wouldn't prefer to keep each port dependencies in same directory.) I don't mind to rename the test font directory. But, I don't agree there will be big benefit from the maintenance cost perspective.

Yes it's possible through extra jhbuild.module files! So we don't need to build whole dependencies duplicated twice and also don't need to rebuild. One remained thing is to modify test fonts path to meet each ports requirement whenever we should run layout test. I believe this is definitely wasting of time. One more thing to tell you is actually this patch is not intended to keep both ports dependencies in same directory. just intended to change the path to install test fonts and it would correct the path to fit hard coded one for efl port.

[ChangSeok Oh]

Created attachment 221235 [details]
Patch

[Gyuyoung Kim]

I agree to fix a wrong font path for DRT. However, I'm not sure we can say now we reduce maintenance cost with unofficial jhbuild file which isn't upstream.

[ChangSeok Oh]

(In reply to comment #15)
> I agree to fix a wrong font path for DRT. However, I'm not sure we can say now we reduce maintenance cost with unofficial jhbuild file which isn't upstream.

Thanks for the r+, this change saves my huge time, though you may not take any benefit for the maintenance const. ;)

[WebKit Commit Bot]

Comment on attachment 221235 [details]
Patch

Clearing flags on attachment: 221235

Committed r162060: <http://trac.webkit.org/changeset/162060>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.