[webkit-reviews] review denied: [Bug 48515] [NRWT] Clear invalid http locks on Windows platform as well : [Attachment 72823] 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 48515: [NRWT] Clear invalid http locks on Windows platform as well
https://bugs.webkit.org/show_bug.cgi?id=48515

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

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

In general, I think this approach is fine.

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:102
> +	       CreateToolhelp32Snapshot =
ctypes.windll.kernel32.CreateToolhelp32Snapshot
> +	       Process32First = ctypes.windll.kernel32.Process32First
> +	       Process32Next = ctypes.windll.kernel32.Process32Next

Can we move this win32 code into a separate method?  Maybe _check_pid_win32? 
Also, I would declare the PROESSENTRY32 class inside this method.  It seems
like you could then unittest it (makes sure it kills a pid and make sure it
doesn't infinite loop if the pid is not found).

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock.py:103
> +	       CloseHandle = ctypes.windll.kernel32.CloseHandle

Were you planning on using this?  Do we need to close something to avoid a
leak?