Bug 97457 - [WK2] TestRunner does not support overridePreference with value "0"
Summary: [WK2] TestRunner does not support overridePreference with value "0"
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: 83897
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-24 08:29 PDT by Dominik Röttsches (drott)
Modified: 2012-09-24 17:04 PDT (History)
8 users (show)

See Also:


Attachments
Patch (7.35 KB, patch)
2012-09-24 12:03 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 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.