[webkit-reviews] review denied: [Bug 37664] Chromium: Add --chromium option to new-run-webkit-websocketserver : [Attachment 54267] Patch

[bugzilla-daemon at webkit.org]

Chris Jerdonek <cjerdonek at webkit.org> has denied Fumitoshi Ukai
<ukai at chromium.org>'s request for review:
Bug 37664: Chromium: Add --chromium option to new-run-webkit-websocketserver
https://bugs.webkit.org/show_bug.cgi?id=37664

Attachment 54267: Patch
https://bugs.webkit.org/attachment.cgi?id=54267&action=review

------- Additional Comments from Chris Jerdonek <cjerdonek at webkit.org>
(In reply to comment #29)
> Created an attachment (id=54267) [details]

I notice several style improvements that can be made to this patch.  Some
of these improvements may be judgment calls while others are more clearly
spelled out by PEP8.  I'm including all of them for future guidance and
consideration, etc.

I will let another reviewer review the substantive elements of this patch, as I
haven't studied this particular area of code much.

> +++ b/WebKitTools/ChangeLog
> +	   env setup and setup_mount for cygwin is moved in
ChromiumWinPort.setup_environ_for_server.
> +
> +	   * Scripts/webkitpy/layout_tests/port/http_server.py:
> +	     remove register_cygwin parameter
> +	     call setup_environ_for_server()
> +	   * Scripts/webkitpy/layout_tests/port/websocket_server.py:
> +	     remove register_cygwin parameter
> +	     call setup_environ_for_server()

The sentences in the ChangeLog comments would be more readable if they
began with a capital letter and ended in a period as proper sentences do.

> +++ b/WebKitTools/Scripts/new-run-webkit-websocketserver
> @@ -55,6 +55,10 @@ def main():
>				default='', help='TLS private key file.')
>      option_parser.add_option('-c', '--certificate', dest='certificate',
>				default='', help='TLS certificate file.')
> +    option_parser.add_option('--chromium', action='store_true',
> +				dest='chromium',
> +				default=False,
> +				help='use the Chromium port')

The formatting of the help text should be consistent with the formatting
of the texts of the other help options for this command.  Does the prevailing
formatting for these options begin with a capital letter?  Does the
prevailing format give the texts an ending period or not?

> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
> @@ -419,7 +419,12 @@ class Port(object):
>  
>      def setup_test_run(self):
>	   """This routine can be overridden to perform any port-specific
> -	   work that shouuld be done at the beginning of a test run."""
> +	   work that should be done at the beginning of a test run."""
> +	   pass
> +
> +    def setup_environ_for_server(self):
> +	   """This routine can be overridden to perform any port-specific
> +	   work that should be done at the beginning of a server launch."""

PEP8 describes specific style rules that doc strings should follow.
In particular--

(1) The first sentence should fit on one line (under 80 characters).
(2) The first sentence should read as a command (e.g. "Set up a test run.").
(3) The closing triple quotes should be on a line by themselves and be
    preceded by a blank line.
    
For example--

>      def setup_test_run(self):
>	   """Perform port-specific work at the beginning of a test run.
>
>	   You can override this routine if necessary.
>
>	   """

> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_win.py
> +	   # Put the cygwin directory first in the path to find cygwin1.dll

PEP8 says that, generally speaking, Python comments should be written
as a complete sentence and end in a period.

> +	   # Put the cygwin directory so that pywebsocket finds proper python
> +	   # executable to run cgi program.

Do you mean to say, "Configure the cygwin directory so that..."?

> +	   if sys.platform == 'win32' and self._options and \
> +		   hasattr(self._options, 'register_cygwin') and \
> +		   self._options.register_cygwin:

PEP8 says that the preferred way to wrap long lines is to use implied
line continuation rather than a trailing backslash.  This will also
make it clearer where to indent on the second and subsequent lines, e.g.

> +	   if (sys.platform == 'win32' and self._options and
> +	       hasattr(self._options, 'register_cygwin') and
> +	       self._options.register_cygwin):


> +	       setup_mount = self.path_from_chromium_base(
> +		   'third_party', 'cygwin', 'setup_mount.bat')

This may read better as--

> +	       setup_mount = self.path_from_chromium_base('third_party',
> +							  'cygwin',
> +							  'setup_mount.bat')

> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/factory_unittest.py
> +import unittest
> +import chromium_linux
> +import chromium_mac
> +import chromium_win
> +import dryrun
> +import factory
> +import gtk
> +import mac
> +import qt
> +import sys
> +import test
> +import win

PEP8 says to group import statements in the following order and separate
the groups by a blank line: (1) standard library, (2) third party, and 
(3) local library.  For example--

import sys
import unittest

import chromium_...
...

> +class FactoryTest(unittest.TestCase):
> +    """Tests factory can create proper port object from port_name,
> +    sys.platform and options.
> +    """

Docstring styling as above.

> +    class WebKitOptions(object):
> +	   """Mimimum options for WebKit port."""

Minimum.  Also, you may want to say, "Represents the minimum options...
."

> +    class ChromiumOptions(WebKitOptions):
> +	   """Mimimum options for Chromium port."""

Minimum.