[webkit-reviews] review denied: [Bug 47072] Implement http locking in NRWT : [Attachment 69679] proposed_patch_v2

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Gabor Rapcsanyi
<rgabor at inf.u-szeged.hu>'s request for review:
Bug 47072: Implement http locking in NRWT
https://bugs.webkit.org/show_bug.cgi?id=47072

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

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

r- mainly because there are no tests

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:1609
> +	   optparse.make_option("--no-wait-for-httpd", action="store_false",
> +	       dest="wait_for_httpd", help="Do not use http locks."),

I realize you're just trying to match ORWT, but do you think we could get by
without --no-wait-for-httpd ?

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:413
> +		       elif self._http_server_running:
> +			   self._clean_up_http_lock()

Why was this added?  Doesn't this say other threads should stop the http
server?

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:560
> +    def _clean_up_http_lock(self):

Do we need to call _clean_up_http_lock when the "test_to_http_lock" are done
running?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:38
> +class HttpLock(object):
> +

I agree with Dirk that this file needs unittests and should include a class
docstring.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:98
> +		   self._process_lock_file_name = self._lock_file_path_prefix +
\
> +					     str(self._next_lock_number())

Nit: () instead of \.  Also the indention looks a bit off.