Summary: | Add support for webkit-test-runner jscOptions in DumpRenderTree and WebKitTestRunner. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
Component: | JavaScriptCore | Assignee: | 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
Mark Lam
2018-06-08 17:07:14 PDT
Created attachment 342350 [details]
proposed patch.
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 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(). Created attachment 342469 [details]
proposed patch.
Thanks for the review. Landed in r232733: <http://trac.webkit.org/r232733>. |