[Webkit-unassigned] [Bug 47072] Implement http locking in NRWT

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=47072


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #69607|review?                     |review-
               Flag|                            |




--- Comment #2 from Tony Chang <tony at chromium.org>  2010-10-04 10:09:00 PST ---
(From update of attachment 69607)
View in context: https://bugs.webkit.org/attachment.cgi?id=69607&action=review

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.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_thread.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_thread.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_thread.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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.