[webkit-reviews] review denied: [Bug 48892] Rebaseline server: initial framework : [Attachment 72780] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Mihai Parparita
<mihaip at chromium.org>'s request for review:
Bug 48892: Rebaseline server: initial framework
https://bugs.webkit.org/show_bug.cgi?id=48892

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

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72780&action=review

Mostly small style nits.

> WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/main.js:37
>  \ No newline at end of file

Nit: New line

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:46
> +class RebaselineHTTPServer(BaseHTTPServer.HTTPServer):
> +    def __init__(self, httpd_port, results_directory):

Nit: Can you add a module level docstring explaining what is in this file and
what this file is for?

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:53
> +    STATIC_FILE_NAMES = [
> +	   'index.html',

Nit: Make this a set.

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:69
> +	       (path, query_string) = self.path.split('?', 1)

Nit: Don't need the () on the left side

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:89
> +    def serve_static_file(self, static_path):

Nit: Can this method be protected?

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:107
> +	   static_file = file(file_path, 'rb')

The preferred way in webkit to open files seems to be:
  with codecs.open(file_path, 'rb') as static_file:
The 'with' automatically closes the file at the end of the scope (which
includes exceptions being raised).

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:111
> +	   (mime_type, encoding) = mimetypes.guess_type(file_path)

Nit: no () on the left

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:117
> +	       expires_time = \
> +		   datetime.datetime.now() + datetime.timedelta(0,
cacheable_seconds)

Nit: I prefer () for implicit line continuation instead of \, but I realize
that's not in PEP8.  I think Eric just prefers to have long lines :)

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:129
> +    help_text = 'Given a test results directory generated by
(new-)run-webkit-tests, starts a local HTTP server that displays unexpected
failures and allows rebaselining.'

Tears, we could have just used the doc string for this, but I guess that boat
has sailed.

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:145
> +	   print 'Starting server at http://localhost:%d/' % options.httpd_port

> +

Nit: Maybe add text saying to browse to /quitquitquit to shutdown the server? 
I guess you can also use ctrl+c to shutdown, right?