Bug 186451

Summary: Add support for webkit-test-runner jscOptions in DumpRenderTree and WebKitTestRunner.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, keith_miller, msaboff, rmorisset, saam, thorton, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 186452    
Attachments:
Description Flags
proposed patch.
none
proposed patch. thorton: review+

Description Mark Lam 2018-06-08 17:07:14 PDT
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>
Comment 1 Mark Lam 2018-06-08 17:21:31 PDT
Created attachment 342350 [details]
proposed patch.
Comment 2 Tim Horton 2018-06-08 17:45:42 PDT
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.
Comment 3 Mark Lam 2018-06-11 15:03:37 PDT
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().
Comment 4 Mark Lam 2018-06-11 15:07:01 PDT
Created attachment 342469 [details]
proposed patch.
Comment 5 Mark Lam 2018-06-11 15:16:01 PDT
Thanks for the review.  Landed in r232733: <http://trac.webkit.org/r232733>.