Bug 97457

Summary:

[WK2] TestRunner does not support overridePreference with value "0"

Product:

WebKit

Reporter:

Dominik Röttsches (drott) <d-r>

Component:

WebKit2

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, cdumez, cmarcelo, kenneth, mihnea, mitz, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

83897

Bug Blocks:

Attachments:

Description	Flags
Patch	none

Description Dominik Röttsches (drott) 2012-09-24 08:29:16 PDT

fast/regions/css-regions-disabled.html disables CSS Regions with an overridePreference call, that doesn't seem to have any effect on EFL WebKit2.

Comment 1 Chris Dumez 2012-09-24 09:53:28 PDT

There seems to be support for this in WebKitTestRunner already. I will debug this once the dependency patch lands.

Comment 2 Chris Dumez 2012-09-24 11:47:01 PDT

The issue is that the current WKTR overridePreference() takes a boolean in argument even though the tests are passing a string. The issue is that "0" and "1" are both evaluated to true boolean value. As a consequence, CSS regions do not get disabled despite the fact that the test calls:
testRunner.overridePreference("WebKitCSSRegionsEnabled", "0");

I will upload a patch soon.

Comment 3 Tony Chang 2012-09-24 12:01:41 PDT

I had the same problem when adding an overridePreference value for CSS Grid Layout.

Then I noticed that some features are set in internals.settings instead of overridePreference.  This is cross platform code rather than needing to modify testRunner.overridePreference in every port.

I think we should try to move more runtime flags into internals.settings.

Comment 4 Chris Dumez 2012-09-24 12:03:05 PDT

Created attachment 165422 [details]
Patch

Comment 5 Chris Dumez 2012-09-24 12:15:17 PDT

(In reply to comment #3)
> I had the same problem when adding an overridePreference value for CSS Grid Layout.
> 
> Then I noticed that some features are set in internals.settings instead of overridePreference.  This is cross platform code rather than needing to modify testRunner.overridePreference in every port.
> 
> I think we should try to move more runtime flags into internals.settings.

In this case, it is a preference that is already present (I'm not adding it). I'm merely fixing an issue with the current implementation of testRunner.overridePreference() for WebKit2. Also note that on WebKit2, we use the same testRunner.overridePreference() implementation for all ports.

Anyway, I think my patch is still valid since it fixes an issue with the existing implementation.

Comment 6 Alexey Proskuryakov 2012-09-24 12:22:40 PDT

Hmm, this is an interesting case. Mac preferences also support "YES", but I don't know how exactly the conversion to boolean works.

Comment 7 Alexey Proskuryakov 2012-09-24 12:25:05 PDT

Comment on attachment 165422 [details]
Patch

Certainly looks like an improvement to me.

Comment 8 Chris Dumez 2012-09-24 12:27:50 PDT

(In reply to comment #6)
> Hmm, this is an interesting case. Mac preferences also support "YES", but I don't know how exactly the conversion to boolean works.

I don't mind adding support for "YES" but there does not seem to be any test case using it. Do you think it is worth adding?

Comment 9 WebKit Review Bot 2012-09-24 17:04:45 PDT

Comment on attachment 165422 [details]
Patch

Clearing flags on attachment: 165422

Committed r129430: <http://trac.webkit.org/changeset/129430>

Comment 10 WebKit Review Bot 2012-09-24 17:04:51 PDT

All reviewed patches have been landed.  Closing bug.