[webkit-reviews] review denied: [Bug 62256] webkitpy: clean up code prior to functional changes for server startup/shutdown : [Attachment 96357] add missing ServerError

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 62256: webkitpy: clean up code prior to functional changes for server
startup/shutdown
https://bugs.webkit.org/show_bug.cgi?id=62256

Attachment 96357: add missing ServerError
https://bugs.webkit.org/attachment.cgi?id=96357&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=96357&action=review

Looks fine in general.	Just a few small questions.

> Tools/Scripts/webkitpy/layout_tests/port/apache_http_server.py:44
> +    name = 'apache'

Nit: I would give this a ALL_CAPS name since it's a static constant.

> Tools/Scripts/webkitpy/layout_tests/port/http_server.py:44
> +    name = 'lighttpd'

Same here.  Maybe SERVER_NAME or something?

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:48
> +    pid_file = None

Should this be in __init__?  It's not clear to me what makes it different from
self._process.

> Tools/Scripts/webkitpy/layout_tests/port/websocket_server.py:40
>  import http_server
> +import http_server_base

Should we switch these to full imports while we're here?

> Tools/Scripts/webkitpy/layout_tests/port/websocket_server.py:201
> +	   if self.pid_file:

Do we need this check?	It looks like self.pid_file should always be set in
__init__.

> Tools/Scripts/webkitpy/layout_tests/port/websocket_server.py:211
> +	   elif self.pid_file:

Same question as above.