[webkit-reviews] review denied: [Bug 47072] Implement http locking in NRWT : [Attachment 69607] proposed patch

[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 69607: proposed patch
https://bugs.webkit.org/attachment.cgi?id=69607&action=review

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

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:308
> +	   return max(self._stop_time - self._start_time - \
> +		      self._http_lock_wait_time(), 0.0)

Nit: I don't think you need the \ here.  It should be implicit since you're in
().

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:350
> +	   self._canceled = True

Instead of setting _canceled directly, can you just call
WatchableThread.cancel(self)?

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:362
> +	   if self._http_lock_wait_end == 0:
> +	       return time.time() - self._http_lock_wait_begin
> +	   return self._http_lock_wait_end - self._http_lock_wait_begin

If wait_for_httpd has not been set, won't we return time.time()?  That is, the
default value for _http_lock_wait_end and _http_lock_wait_begin are 0.	This
means that get_total_time() will return self._stop_time - self._start_time -
time.time(), which seems incorrect.

>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:402
> +		   if self._options.wait_for_httpd and \
> +		     self._current_dir == "tests_to_http_lock" and \
> +		     not self._http_server_running:

Nit: () around conditions is preferred to using \.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:34
> +import os
> +import sys
> +import time
> +import fcntl
> +import signal
> +import glob

Nit: please sort these alphabetically

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:42
> +	   self._lock_path = "/tmp"
> +	   self._lock_file_prefix = "WebKitHttpd.lock."
> +	   self._lock_file_path_prefix = os.path.join(self._lock_path,

Would it be better to use tempfile.gettempdir() instead of /tmp?  This would
allows people to override the location by setting TEMPDIR.  On the other hand,
apache uses /tmp/WebKit for some fimes.  If we don't use tempfile, we should at
put the files in /tmp/WebKit/.

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:45
> +	   self._my_lock_file_name = ""

Nit: Drop the "my" in the variable name.  Maybe process_lock_file_name would be
better?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:53
> +	   if not os.path.exists(lock_file_name):
> +	       return -1

Do we need this check?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/http_lock.py:86
> +	       if not current_pid or \
> +		 not (sys.platform in ('darwin', 'linux2') and \
> +		 self._check_pid(int(current_pid))):

Nit: use () for line continuation instead of \.  Also, you can do "sys.platform
not in ('darwin', 'linux2')", which is a little easier to read.