Bug 96116 - [EFL] Use same default minimum logical font size in DRT and WTR
Summary: [EFL] Use same default minimum logical font size in DRT and WTR
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-07 09:36 PDT by Chris Dumez
Modified: 2012-09-07 13:04 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.36 KB, patch)
2012-09-07 09:43 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (6.48 KB, patch)
2012-09-07 09:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (742.55 KB, patch)
2012-09-07 12:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2012-09-07 09:36:04 PDT
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.
Comment 1 Chris Dumez 2012-09-07 09:43:04 PDT
Created attachment 162791 [details]
Patch
Comment 2 Chris Dumez 2012-09-07 09:46:10 PDT
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.
Comment 3 Chris Dumez 2012-09-07 09:48:24 PDT
Created attachment 162793 [details]
Patch

Rebase on master.
Comment 4 Martin Robinson 2012-09-07 09:49:40 PDT
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.
Comment 5 Chris Dumez 2012-09-07 10:07:13 PDT
(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
Comment 6 Martin Robinson 2012-09-07 10:33:06 PDT
(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.
Comment 7 Chris Dumez 2012-09-07 12:04:22 PDT
Created attachment 162839 [details]
Patch

Take Martin Robinson's feedback into consideration.
Dropped support for other ports since this approach requires rebaselining.
Comment 8 WebKit Review Bot 2012-09-07 13:04:06 PDT
Comment on attachment 162839 [details]
Patch

Clearing flags on attachment: 162839

Committed r127909: <http://trac.webkit.org/changeset/127909>
Comment 9 WebKit Review Bot 2012-09-07 13:04:12 PDT
All reviewed patches have been landed.  Closing bug.