[webkit-reviews] review denied: [Bug 170144] webkitpy: Standardize web-server port definitions : [Attachment 305596] 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 305596: Patch

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




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

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

> Tools/Scripts/webkitpy/layout_tests/servers/web_platform_test_server.py:104
> +	   result = []
> +	   for dict in self._mappings:
> +	       result.append(dict['port'])
> +	   return result

We can use list comprehension to simplify this to:

return [dict['port'] for dict in self._mappings]

> Tools/Scripts/webkitpy/port/base.py:940
> +    def web_server_ports(self):

Maybe a better name for this function would be ports_to_forward()?

> Tools/Scripts/webkitpy/port/base.py:955
> +	   ports = []
> +	   port_from_options = self.get_option('http_port')
> +	   if port_from_options is not None:
> +	       ports.append(int(port_from_options))
> +	   else:
> +	       ports.extend([
> +		   http_server_base.HttpServerBase.HTTP_SERVER_PORT,
> +		  
http_server_base.HttpServerBase.ALTERNATIVE_HTTP_SERVER_PORT,
> +		   http_server_base.HttpServerBase.HTTPS_SERVER_PORT,
> +	       ])
> +	   ports.extend([
> +	       websocket_server.PyWebSocket.DEFAULT_WS_PORT,
> +	       websocket_server.PyWebSocket.DEFAULT_WSS_PORT,
> +	   ])
> +

This does not seem like the correct approach to use because it is fragile and
duplicates logic for determining the ports that will be opened. The PyWebSocket
class and HttpServerBase class or derived classes of it are the most
knowledgeable about the ports they actually chose to open. I suggest that we
expose a method on PyWebSocket and HttpServerBase called ports_to_forward() (I
am open to name suggestions) that returns an array of the ports that were
opened. In HttpServerBase, ports_to_forward() should raise a not implemented
exception and the derived classes Lighttpd and LayoutTestApacheHttpd should
override the base implementation. Then we can implement this function as:

ports = []
if self._http_server:
    ports.extend(self._http_server.ports_to_forward())
if self._websocket_server:
    ports.extend(self._websocket_server.ports_to_forward())
if self._websocket_secure_server:
    ports.extend(self._websocket_secure_server.ports_to_forward())
if self._web_platform_test_server:
    ports.extend(self._web_platform_test_server.ports_to_forward())
return ports