Bug 229253 – Add a new option '--show-window' to run-webkit-tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Peng Liu]

Add a new option '--show-webview' to run-webkit-tests

[Peng Liu]

Created attachment 435805 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 435805 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:343
> +        optparse.make_option('--show-window', action="store_true", default=False, help="If set, a window (with webview) will show up. Helpful for debugging some tests."),

I think the help should say "Make the test runner window visible during testing"

[Peng Liu]

Created attachment 435809 [details]
Revise the patch based on Simon's comment

[Peng Liu]

Comment on attachment 435805 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:343
>> +        optparse.make_option('--show-window', action="store_true", default=False, help="If set, a window (with webview) will show up. Helpful for debugging some tests."),
> 
> I think the help should say "Make the test runner window visible during testing"

Fixed. Thanks!

[Jonathan Bedard]

Comment on attachment 435809 [details]
Revise the patch based on Simon's comment

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

> Tools/WebKitTestRunner/Options.cpp:169
> +    optionList.append(Option("--show-window", "Show the WebView during test runs (for debugging)", handleOptionShowWebView));

Why add the new option here instead of just passing --show-webview in the options? If we wanted to keep the option and name aligned, when creating the option, we could do this:

optparse.make_option('--show-window', '--show-webview', dest='show_webview', ...)

which would make the option inside webkitpy 'show_webview', but the option '--show-window' with an alias '--show-webview'

[Peng Liu]

Comment on attachment 435809 [details]
Revise the patch based on Simon's comment

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

>> Tools/WebKitTestRunner/Options.cpp:169
>> +    optionList.append(Option("--show-window", "Show the WebView during test runs (for debugging)", handleOptionShowWebView));
> 
> Why add the new option here instead of just passing --show-webview in the options? If we wanted to keep the option and name aligned, when creating the option, we could do this:
> 
> optparse.make_option('--show-window', '--show-webview', dest='show_webview', ...)
> 
> which would make the option inside webkitpy 'show_webview', but the option '--show-window' with an alias '--show-webview'

The original plan was to use `--show-window` instead of `--show-webview` in both run-webkit-tests and WebKitTestRunner , and Simon suggested to add an alias.

[Alexey Proskuryakov]

I don't have a string opinion about which name to use, but having two options with identical descriptions is super confusing.

If you do this, please change the description to something like "DEPRECATED. Same as --show-window".

[Peng Liu]

Created attachment 435880 [details]
Revise the patch based on Alexey's comment

[Peng Liu]

Created attachment 435889 [details]
Fix build failures on the win bot

[Peng Liu]

(In reply to Alexey Proskuryakov from comment #7)
> I don't have a string opinion about which name to use, but having two
> options with identical descriptions is super confusing.
> 
> If you do this, please change the description to something like "DEPRECATED.
> Same as --show-window".

Agree! Fixed.

[EWS]

Committed r281461 (240840@main): <https://commits.webkit.org/240840@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 435889 [details].

[Radar WebKit Bug Importer]

<rdar://problem/82255475>