Add a new option '--show-webview' to run-webkit-tests
Created attachment 435805 [details] Patch
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"
Created attachment 435809 [details] Revise the patch based on Simon's comment
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!
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'
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.
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".
Created attachment 435880 [details] Revise the patch based on Alexey's comment
Created attachment 435889 [details] Fix build failures on the win bot
(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.
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].
<rdar://problem/82255475>