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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.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 72998: Patch
https://bugs.webkit.org/attachment.cgi?id=72998&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72998&action=review

Thanks for posting this in small pieces.  Also, can we test this code?	For
example, we could test the command by having a mock HTTPServer so we didn't
have to spin up a real server.

>
WebKitTools/Scripts/webkitpy/tool/commands/data/rebaselineserver/index.html:38
> +    <a href="/quitquitquit">Exit</a>

:)

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:50
> +class RebaselineHTTPServer(BaseHTTPServer.HTTPServer):

I wonder if these classes should go in a different package.  We don't really
have anything similar to this now, so we might need to invent something (like
we invented the bot package to hold the bot-related support classes).  Having
this code here seems like it will make this file grow to infinite size.

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:87
> +	   # See if a class method matches.
> +	   func_name = func_or_file_name.replace('.', '_')

Should we prevent calls to function names that start with _ ?  Also, WebKit
doesn't like abbreviations in variable names, so this would be better as
function_name.

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:132
> +    help_text = __doc__

Should we actually add this __doc__ text?

> WebKitTools/Scripts/webkitpy/tool/commands/rebaselineserver.py:143
> +	       print 'Path to results directory is required.'

If it's required, why does its description have [ ] around it?	I think our
argument parser is smart enough to do this checking for you if you used
argument_names = "PATH_TO_RESULTS_DIRECTORY".  Also, we're trying to use "
consistently instead of '.  Yes, we're terrible at that, but I mention it
anyway.