[webkit-reviews] review denied: [Bug 37664] Chromium: Add --chromium option to new-run-webkit-websocketserver : [Attachment 55520] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Fumitoshi Ukai <ukai at chromium.org>'s
request for review:
Bug 37664: Chromium: Add --chromium option to new-run-webkit-websocketserver
https://bugs.webkit.org/show_bug.cgi?id=37664

Attachment 55520: Patch
https://bugs.webkit.org/attachment.cgi?id=55520&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:52
 +	    chromium.ChromiumPort.setup_environ_for_server(self)
This is wrong.	I know it doesn't do anyting yet, but I Think you meant env =
chromium.ChromiumPort.setup_environ_for_server, no?

WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:54
 +	    env = os.environ
This line wouldn't be needed if the above was fixed.

WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:55
 +	    env['PATH'] = '%s;%s' % (
Not a big deal at all, but I think the conciseness was to standardize on " over
'.   I don't really care which we use so long as we try to pick one. :)

WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:62
 +	    if (sys.platform == 'win32' and self._options and
We need to move away from this hasattr pattern and towards one where the
options are closer to the code at hand.  A genric "options" element is a bad
design.  Code which uses certain optiosn should always require those to be
passed.

WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py:68
 +		subprocess.Popen(setup_mount).wait()
Why not Executive.run_command()?

WebKitTools/Scripts/webkitpy/layout_tests/port/factory_unittest.py:51
 +	class WebKitOptions(object):
The ports themselves should expose what options they require.  But we'll fix
that globally at some later date.

WebKitTools/Scripts/webkitpy/layout_tests/port/factory_unittest.py:139
 +  if __name__ == '__main__':
I don't think these are generally used in our unittest setup.

Certainly looks better than the code that was there before.  r- for the env =
chromium.ChromiumPort.setup_environ_for_server() issue, all the rest are nits.