[webkit-reviews] review granted: [Bug 98071] webkitpy should accept a different httpd.conf specified by the user : [Attachment 166545] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has granted Raphael Kubo da Costa (rakuco)
<rakuco at webkit.org>'s request for review:
Bug 98071: webkitpy should accept a different httpd.conf specified by the user
https://bugs.webkit.org/show_bug.cgi?id=98071

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

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=166545&action=review


> Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:603
> +	   os.environ = saved_environ.copy()

You should wrap these in a try-finally block to ensure that os.environ gets
restored no matter what happens.

Longer term we might want to consider enhancing the environment wrapper in
webkitpy.common.system to make this more mockable.

> Tools/Scripts/webkitpy/layout_tests/servers/http_server.py:103
> +	   base_conf_file = self._path_to_config_file()

I'm not a fan of the duplication between this and the code in base.py. I'm also
not sure that this is even worth doing since AFAIK only chromium uses this
routine and we're not likely to set this. I appreciate your diligence, but how
about we just add a FIXME to consider supporting the variable and otherwise
leave the files alone?