[webkit-reviews] review denied: [Bug 38034] WebSocket: pywebsocket 0.5 : [Attachment 54396] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Fumitoshi Ukai
<ukai at chromium.org>'s request for review:
Bug 38034: WebSocket: pywebsocket 0.5
https://bugs.webkit.org/show_bug.cgi?id=38034

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

------- Additional Comments from David Levin <levin at chromium.org>
Looking good. Just a few things to address.

> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index
167c788bb4972264e4e014b5f76110728c73765b..5fc56a4717d538c358d0bac2768b4c6655bdb
91a 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,25 @@
> +2010-04-27  Fumitoshi Ukai  <ukai at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   WebSocket: pywebsocket 0.5
> +	   https://bugs.webkit.org/show_bug.cgi?id=38034
> +
> +	   Remove pywebsocket from webkitpy/thirdparty.
> +	   Make pywebsocket autoinstalled.
> +
> +	   * Scripts/new-run-webkit-websocketserver:
> +	     Add --output-dir option.
> +	   * Scripts/old-run-webkit-tests:
> +	     Use new-run-webkit-websocketserver, rather than directly run
pywebsocket's standalone.py.
> +	   * Scripts/run-webkit-websocketserver:
> +	     Use new-run-webkit-websocketserver, rather than directly run
pywebsocket's standalone.py.

"Ditto." works instead of repeating the exact same comment.


btw, can we just get rid of "run-webkit-websocketserver" and rename
"new-run-webkit-websocketserver" to "run-webkit-websocketserver"? *Not in this
patch* but a follow up patch.


> diff --git a/WebKitTools/Scripts/old-run-webkit-tests
b/WebKitTools/Scripts/old-run-webkit-tests

The following lines are repeated in "closeWebSocketServer" It feels like it
should be a common function that both of these places call.

>      my $webSocketServerPath = "/usr/bin/python";
> +    my $pid = open3(\*WEBSOCKETSERVER_IN, \*WEBSOCKETSERVER_OUT,
\*WEBSOCKETSERVER_ERR, $webSocketServerPath, @args);
> +    waitpid $pid, 0;
> +    close WEBSOCKETSERVER_IN;
> +    close WEBSOCKETSERVER_OUT;
> +    close WEBSOCKETSERVER_ERR;


> diff --git a/WebKitTools/Scripts/run-webkit-websocketserver
b/WebKitTools/Scripts/run-webkit-websocketserver

This has the same problem of repeating a bunch of lines related to running
webSocketServer