[webkit-reviews] review granted: [Bug 47072] Implement http locking in NRWT : [Attachment 70429] proposed_patch_v3

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has granted 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 70429: proposed_patch_v3
https://bugs.webkit.org/attachment.cgi?id=70429&action=review

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

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:48
>  import apache_http_server
>  import test_files
> +import http_lock
>  import http_server
>  import websocket_server

Nit: sort these alphabetically

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:113
> +		   self._process_lock_file_name = (self._lock_file_path_prefix
+
> +					     str(self._next_lock_number()))

Nit: The indention looks weird here.  Either line it up with the ( in the
previous line or just indent 4 spaces.

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:53
> +	   if first != second:
> +	       self.clean_all_lockfile()

This seems ok, although you could also use try/finally in each test.  I don't
really care either way.

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:83
> +	   lock_file_list = [
> +	       self.lock_file_path_prefix + "00",
> +	       self.lock_file_path_prefix + "9",
> +	       self.lock_file_path_prefix + "001",
> +	       self.lock_file_path_prefix + "021",
> +	   ]

Nit: A tuple is a bit faster than using a list.  It also prevents unintended
modification.

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:85
> +	   expected_number_list = ["0", "9", "1", "21"]

Nit: tuple, and why not drop the "" and just make these ints (so you and avoid
the cast below).

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:89
> +	   for i in range(len(lock_file_list)):
> +	      
self.assertEqual(self.http_lock_obj._extract_lock_number(lock_file_list[i]),
> +				int(expected_number_list[i]))

Nit: You could re-write this as:
for lock_file, expected in zip(lock_file_list, expected_number_list):
    self.assertEqual(self.http_lock_obj._extract_lock_number(lock_file),
int(expected_number_list))