Bug 186451 – Add support for webkit-test-runner jscOptions in DumpRenderTree and WebKitTestRunner.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

This option can be used by a layout test to specify some JSC runtime options needed by the test e.g. by adding something like this to the top of the html file after the DOCTYPE tag:
    <!-- webkit-test-runner [ jscOptions=--useIntlPluralRules=true ] -->

If more than one option is needed, the options can be specified as a comma separated string e.g.
    <!-- webkit-test-runner [ jscOptions=--useIntlPluralRules=true,--useZombieMode=true ] -->

This only works with JSC options that can be changed at runtime.  Not all JSC options can be changed this way as some options are only checked and set once at VM / process initialization time: changing these types of options may have no effect.  It's the test writer's responsibility to determine which options are appropriate for with this webkit-test-runner jscOptions option.

<rdar://problem/40875792>

[Mark Lam]

Created attachment 342350 [details]
proposed patch.

[Tim Horton]

Comment on attachment 342350 [details]
proposed patch.

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

> Tools/DumpRenderTree/TestOptions.cpp:115
> +        && other.jscOptions == jscOptions;

It looks to me like you can set these dynamically. Do you actually need to re-create the web view? You might be able to avoid this.

> Tools/WebKitTestRunner/TestOptions.h:95
> +            || jscOptions != options.jscOptions)

It looks to me like you can set these dynamically. Do you actually need to re-create the web view? You might be able to avoid this.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:232
> +            size_t stringLength = WKStringGetLength(jscOptionsString.get());
> +            Vector<char> optionsVector;
> +            optionsVector.resize(stringLength + 1);
> +            WKStringGetUTF8CString(jscOptionsString.get(), optionsVector.data(), stringLength + 1);
> +            String options(optionsVector.data(), stringLength);

How about you just use toWTFString instead of all this madness.

[Mark Lam]

Comment on attachment 342350 [details]
proposed patch.

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

>> Tools/DumpRenderTree/TestOptions.cpp:115
>> +        && other.jscOptions == jscOptions;
> 
> It looks to me like you can set these dynamically. Do you actually need to re-create the web view? You might be able to avoid this.

Unlike the WKTR case below, DRT's TestOptions::webViewIsCompatibleWithOptions() does not block resetWebViewToConsistentStateBeforeTesting() from being called.  So, we can argue that this change is not needed for the intl JSC options used in this patch.

However, there are some JSC options that only take effect when the JSGlobalObject is constructed.  It is better for the test harness to enable to use of those options as well.  Hence, I would prefer to keep this test condition.

>> Tools/WebKitTestRunner/TestOptions.h:95
>> +            || jscOptions != options.jscOptions)
> 
> It looks to me like you can set these dynamically. Do you actually need to re-create the web view? You might be able to avoid this.

This is needed.  Here's why:
1. hasSameInitializationOptions(0 is called by viewSupportsOptions().
2. viewSupportsOptions() is called by TestController::ensureViewSupportsOptionsForTest().
3. TestController::ensureViewSupportsOptionsForTest() returns early if viewSupportsOptions() returns true.  This results in TestController::resetStateToConsistentValues() not being called.  As a result, the TestRunner will never apply a different set of jscOptions.

I first observed the need for this when I ran the layout tests with my changes and found that my change did not take effect, and it was due to omitting this change.

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:232
>> +            String options(optionsVector.data(), stringLength);
> 
> How about you just use toWTFString instead of all this madness.

Thanks.  I looked for such a conversion utility function but wasn't able to find it.  I've changed the code to use toWTFString().

[Mark Lam]

Created attachment 342469 [details]
proposed patch.

[Mark Lam]

Thanks for the review.  Landed in r232733: <http://trac.webkit.org/r232733>.