[webkit-reviews] review denied: [Bug 62180] nrwt: fix http, websocket server startup, shutdown : [Attachment 96359] ready for review

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 62180: nrwt: fix http, websocket server startup, shutdown
https://bugs.webkit.org/show_bug.cgi?id=62180

Attachment 96359: ready for review
https://bugs.webkit.org/attachment.cgi?id=96359&action=review

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

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:48
> +    mappings = {}

It doesn't look like this dict should be shared by all instances that don't
override it (e.g., it would be easy for a subclass to accidentally insert a
value into this global dict).  I would just make it a member variable to make
it more obvious that sub classes should be providing their own instance of it.

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:66
> +	   except OSError, e:
> +	       _log.warning('Failed to remove old %s log files' % self.name)

Should we just move this try/except into remove_log_files()?  It seems a bit
weird that remove_stale_logs() needs to know what type of errors it can raise.

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:75
> +	   _log.debug("%s started. Testing ports" % self.name)
> +	   server_started =
self._wait_for_action(self._is_server_running_on_all_ports)

It looks like we used to retry for a few seconds for the python server in case
it was slow to start.  Should we do something similar here?

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:90
> +	   elif self.pid_file and self._filesystem.exists(self.pid_file):
> +	       pid = int(self._filesystem.read_text_file(self.pid_file))

What cases would this be true?	Do we need to have a similar check in
_is_server_running_on_all_ports()?

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:107
> +    def prepare_config(self):
> +	   pass

Should these interface methods be protected?

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:144
> +	       s = socket.socket()

Nit: s -> test_socket or something

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:147
> +		   s.connect(('localhost', port))

This isn't quite the same check right?	The old code would make sure the server
understood HTTP.  Maybe that's OK?

> Tools/Scripts/webkitpy/layout_tests/port/http_server_base.py:160
> +	       s = socket.socket()

Nit: s-> test_socket or something