Bug 96116 – [EFL] Use same default minimum logical font size in DRT and WTR

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

EFL, Qt and GTK ports use a default minimum logical font size of 0 for WebKit1, while Mac and Windows use 9.
On WebKit2, the default value is 9 for all platforms.

This results in tests having different output with WK1 and WK2 due to text being bigger with WK2 than with WK1 (for EFL, GTK and Qt ports). At least the following test cases are affected:
fast/ruby/nested-ruby.html
fast/ruby/ruby-beforeafter.html
fast/ruby/ruby-empty-rt.html
fast/ruby/ruby-length.html
fast/ruby/ruby-run-break.html
fast/ruby/ruby-runs-spans.html
fast/ruby/ruby-runs.html
fast/ruby/ruby-simple-rp.html
fast/ruby/ruby-simple.html
fast/ruby/ruby-trailing.html
fast/ruby/rubyDOM-insert-rt.html
fast/ruby/rubyDOM-insert-text1.html
fast/ruby/rubyDOM-insert-text2.html
fast/ruby/rubyDOM-insert-text3.html
fast/ruby/rubyDOM-remove-rt1.html
fast/ruby/rubyDOM-remove-rt2.html
fast/ruby/rubyDOM-remove-text1.html
fast/ruby/rubyDOM-remove-text2.html
http/tests/misc/object-embedding-svg-delayed-size-negotiation-2.htm

We need to fix this to have consistent values for all ports between WK1 and WK2.

[Chris Dumez]

Created attachment 162791 [details]
Patch

[Chris Dumez]

Adding Ossy in CC since I haven't unskipped the tests for Qt port.

I did not dare unskip those tests for Qt ports because they are skipped in the global Skipped list, instead of the wk2 one. Moreover, I don't have a local Qt port build to verify if the tests are now passing.

[Chris Dumez]

Created attachment 162793 [details]
Patch

Rebase on master.

[Martin Robinson]

Comment on attachment 162791 [details]
Patch

Wouldn't it make sense to set this explicitly in the test runner instead of changing the default? It would just be a matter of updating both DumpRenderTree and WebKitTestRunner.

[Chris Dumez]

(In reply to comment #4)
> (From update of attachment 162791 [details])
> Wouldn't it make sense to set this explicitly in the test runner instead of changing the default? It would just be a matter of updating both DumpRenderTree and WebKitTestRunner.

It is not just about layout tests. I don't see why we would use a different default value for the same setting in WebKit1 and WebKit2.

Sure, we can set them explicitly in DRT / WKTR but it does not change the fact that the default value should be consistent between WK1 and WK2 for each port IMHO.

There are already other settings in the file I edited that are customized for each port using #if statements, for e.g.:

#if PLATFORM(GTK)
#define DEFAULT_WEBKIT_TABSTOLINKS_ENABLED true
#else
#define DEFAULT_WEBKIT_TABSTOLINKS_ENABLED false
#endif

[Martin Robinson]

(In reply to comment #5)

> Sure, we can set them explicitly in DRT / WKTR but it does not change the fact that the default value should be consistent between WK1 and WK2 for each port IMHO.

Since the WebKit2 C API is designed to be cross-platform, wouldn't it make more sense to value the consistency among the WebKit2 C APIs higher than the consistency of particular port's WebKit1 and WebKit2 ports?

Note that what we are talking about it only for the C API. Ports may choose to adjust the settings for their user-exposed APIs (for GTK+ this is the GObject one).

Consider that reducing the differences between the C APIs reduces the amount of platform-dependent code in WebKitTestRunner. In the past, what we've done is to bring DumpRenderTree in line with the output of WebKitTestRunner, which brings all results closer to each other. This change seems to only solidify those differences.
 
> There are already other settings in the file I edited that are customized for each port using #if statements, for e.g.:
> 
> #if PLATFORM(GTK)
> #define DEFAULT_WEBKIT_TABSTOLINKS_ENABLED true
> #else
> #define DEFAULT_WEBKIT_TABSTOLINKS_ENABLED false
> #endif

I think in this case especially, we should probably move this to the GObject API layer.

[Chris Dumez]

Created attachment 162839 [details]
Patch

Take Martin Robinson's feedback into consideration.
Dropped support for other ports since this approach requires rebaselining.

[WebKit Review Bot]

Comment on attachment 162839 [details]
Patch

Clearing flags on attachment: 162839

Committed r127909: <http://trac.webkit.org/changeset/127909>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.