Bug 170144 – webkitpy: Standardize web-server port definitions

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

Currently, web-servers receive their ports through a mixture of global constants and hard-coded port values.  Make web-servers use global constants to determine default port values and provide a function listing all used ports so that tools can know which ports need to be managed.

[Radar WebKit Bug Importer]

<rdar://problem/31284026>

[Jonathan Bedard]

Created attachment 305518 [details]
Patch

[Daniel Bates]

Comment on attachment 305518 [details]
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?

[Jonathan Bedard]

Comment on attachment 305518 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/servers/http_server.py:81
>>                   '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?

I didn't see the port mapping code bellow.

I'm not sure what the self._root is being used for.  It should definitely be adopting the constants, though.

>> 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?

Actually, we should do the opposite.  PyWebSocket should own it's own constants.  In fact, it already does, they are just made private.

[Jonathan Bedard]

Created attachment 305596 [details]
Patch

[Daniel Bates]

Comment on attachment 305596 [details]
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

[Jonathan Bedard]

Created attachment 305749 [details]
Patch

[Daniel Bates]

Comment on attachment 305749 [details]
Patch

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

OK

> Tools/ChangeLog:9
> +        Default web-server ports should be declared in global variables.

Is this description up-to-date?

> Tools/Scripts/webkitpy/layout_tests/servers/websocket_server.py:-48
> -_DEFAULT_WSS_PORT = 9323

I am assuming that this was unused before his patch as I did not see any caller updated in this patch.

[Jonathan Bedard]

Created attachment 305767 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 305767 [details]
Patch for landing

Clearing flags on attachment: 305767

Committed r214558: <http://trac.webkit.org/changeset/214558>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.