Bug 186451 - Add support for webkit-test-runner jscOptions in DumpRenderTree and WebKitTestRunner.
Summary: Add support for webkit-test-runner jscOptions in DumpRenderTree and WebKitTes...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks: 186452
  Show dependency treegraph
 
Reported: 2018-06-08 17:07 PDT by Mark Lam
Modified: 2018-06-11 15:16 PDT (History)
8 users (show)

See Also:


Attachments
proposed patch. (19.46 KB, patch)
2018-06-08 17:21 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (19.27 KB, patch)
2018-06-11 15:07 PDT, Mark Lam
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>.