[webkit-reviews] review denied: [Bug 170144] webkitpy: Standardize web-server port definitions : [Attachment 305518] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 170144: webkitpy: Standardize web-server port definitions
https://bugs.webkit.org/show_bug.cgi?id=170144

Attachment 305518: Patch

https://bugs.webkit.org/attachment.cgi?id=305518&action=review




--- Comment #3 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 305518
  --> https://bugs.webkit.org/attachment.cgi?id=305518
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305518&action=review

> Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:61
> +	       self._mappings = [{'port': self.DEFAULT_PORTS[0]},
> +				 {'port': self.DEFAULT_PORTS[1]},
> +				 {'port': self.DEFAULT_PORTS[2], 'sslcert':
True}]

It seems error prone to use an array to hold these ports numbers because the
ordering of the array elements can affect the port used for the SSL port. I
suggest we use named constants, say HTTP_SERVER_PORT,
ALTERNATIVE_HTTP_SERVER_PORT and HTTPS_SERVER_PORT for 8000, 8080, and 8443,
respectively.

> Tools/Scripts/webkitpy/layout_tests/servers/http_server.py:81
> +		   {'port': self.DEFAULT_PORTS[2], 'docroot':
self._webkit_tests,
>		    'sslcert': self._pem_file}])

Similarly, this is error prone because the ordering of self.DEFAULT_PORTS[2]
can affect which port we chose to use as the SSL port.

On another note, I do not understand what the purpose of self._root is in this
class? We have similar code below to setup the port mapping that was not
updated and hence still hardcodes the port numbers 8000, 8080, and 8443. Did
you intend to keep such code as-is?

> Tools/Scripts/webkitpy/port/base.py:942
> +    def webserver_ports(self):

webserver => web_server

Web server is two words.

> Tools/Scripts/webkitpy/port/base.py:992
> +				   use_tls=True, port=Port.WEBSOCKET_PORT,
private_key=private_key_file, certificate=certificate_file)

Maybe Port.WEBSOCKET_SERVER_PORT would be more descriptive?

Would it make sense to move the named constants for the HTTP server ports to
the base port class given that the base port class has a named constant for the
WebSocket server port?