[webkit-reviews] review granted: [Bug 183265] [webkitpy, WinCairo] Launch Apache HTTPD for HTTP Tests. : [Attachment 335222] PATCH

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 183265: [webkitpy, WinCairo] Launch Apache HTTPD for HTTP Tests.
https://bugs.webkit.org/show_bug.cgi?id=183265

Attachment 335222: PATCH

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




--- Comment #16 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 335222
  --> https://bugs.webkit.org/attachment.cgi?id=335222
PATCH

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

> Tools/ChangeLog:7
> +

Please describe this change as the title of the bug is not descriptive enough.
The purpose of this change is to support running HTTP layout tests on Windows
without Cygwin. Currently we only support running HTTP layout tests on Windows
in Cygwin.

> Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:178
> +    @property
> +    def platform(self):
> +	   return self._port_obj.host.platform

Nice.

> Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:227
> +    def _server_error(self, message, err, exit_code):

I know that you named the second argument, err, based on the existing code. It
neither conforms to the naming conventions in the WebKit Code Style guidelines
nor is is very descriptive. I suggest we rename the argument err to
stderr_output (or can we come up with a better name) to describe what it is:
the standard error output.

> Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:229
> +	       err = 'Access is denied. Do you have an administrator
privilegde?'

"an administrator privilegde?" => "administrator privilege?"

> Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:230
> +

Please remove this empty line as I do not feel it improves the readability of
this function.

> Tools/Scripts/webkitpy/layout_tests/servers/apache_http_server.py:231
> +	   return http_server_base.ServerError('%s: %s (exit code=%s)' %
(message, err, exit_code))

We prefer using String.format() in new code as this was initially suggested as
the analog in Python 3. I am unclear when we will move to Python 3 (if ever) or
if String.format() is still preferred in Python 3. If you chose to use the %
operator then we should format the exit code as decimal (%d).

> Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py:168
> +

Please remove this empty line as I do not feel it improves the readability of
this function.

> Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py:171
> +    def _prepare_aliases(self, data):

Maybe a more descriptive name for this would be _build_alias_path_pairs?

> Tools/Scripts/webkitpy/layout_tests/servers/http_server_base.py:174
> +

Please remove this empty line as I do not feel it improves the readability of
this function.

> Tools/Scripts/webkitpy/layout_tests/servers/http_server_base_unittest.py:60
> +    def test_aliases(self):

I suggest we name this function test_prepare_aliases to coincide with the name
of the function it is testing _prepare_aliases. If you take my suggestion above
then we should rename this function to test_build_alias_path_pairs.

> Tools/Scripts/webkitpy/layout_tests/servers/http_server_base_unittest.py:68
> +	       ["/media-resources", "media"],
> +	       ["/modern-media-controls",
"../Source/WebCore/Modules/modern-media-controls"],
> +	       ["/resources/testharness.css", "resources/testharness.css"],

" (double quote) => ' (single quote)

> Tools/Scripts/webkitpy/layout_tests/servers/http_server_base_unittest.py:74
> +	       ('/media-resources', '/test.checkout/LayoutTests/media'),
> +	       ('/modern-media-controls',
'/test.checkout/LayoutTests/../Source/WebCore/Modules/modern-media-controls'),
> +	       ('/resources/testharness.css',
'/test.checkout/LayoutTests/resources/testharness.css'),

This output matches the output on Windows? I would have expected the slashes to
be different.

> Tools/Scripts/webkitpy/port/win.py:187
> +    def _expected_path_to_apache(self):
> +	   return self._filesystem.join('C:\\', 'xampp', 'apache', 'bin',
'httpd.exe')

I do not see the need for this function because it is referenced exactly once
from _path_to_apache() and its name raises more questions than answers
("expected"?). I suggest we inline the implementation of this function into
_path_to_apache() and remove this function.

> Tools/Scripts/webkitpy/port/win.py:193
> +

Please remove this empty line as I do not feel it improves the readability of
this function.

> Tools/Scripts/webkitpy/port/win.py:479
> +	   # To launch apache as a daemon, service installation is required.

apache => Apache
(it's a proper noun)

> Tools/Scripts/webkitpy/port/win.py:481
> +	   # 0=success, 2=already installed, 720005=permission error, etc

etc => "etc." (notice the period at the end since etc. is an abbreviation of et
cetera).