[webkit-reviews] review denied: [Bug 47220] new-run-webkit-tests - enable cygwin support for chromium win : [Attachment 71094] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 47220: new-run-webkit-tests - enable cygwin support for chromium win
https://bugs.webkit.org/show_bug.cgi?id=47220

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71094&action=review

I'd like to see a new version using with_statement.  Otherwise I think this is
fine.

The _Cygpath class is a bit big (does management of a singleton, as well as the
nuts and bolts of talking to the process), but it's about to get a lot cleaner
with "with", so it may not matter.

> WebKitTools/Scripts/webkitpy/common/system/path.py:53
> +    lock = None

Shouldn't this be _lock?  I guess the class is already _, but hte lock seems
internal.

> WebKitTools/Scripts/webkitpy/common/system/path.py:58
> +	   if _CygPath.lock:

I would have early returned isntead.

> WebKitTools/Scripts/webkitpy/common/system/path.py:59
> +	       _CygPath.lock.acquire()

using "with" here would make the locking cleaner.  No need for the try/finally
then.

> WebKitTools/Scripts/webkitpy/common/system/path.py:72
> +	   _CygPath.lock.acquire()
> +	   if not _CygPath.singleton:

"with" again would make this easier.

> WebKitTools/Scripts/webkitpy/common/system/path.py:81
> +	   result = None
> +	   try:
> +	       result = _CygPath.singleton.convert(path)
> +	   finally:
> +	       _CygPath.lock.release()

with allows you to just return directly too.

> WebKitTools/Scripts/webkitpy/common/system/path.py:86
> +	   self._child_process = None

Naming this _child might be sufficiently clear, and would save some typing. 
Your call.

> WebKitTools/Scripts/webkitpy/common/system/path.py:94
> +						  close_fds=False)

close_fds defaults to false.

> WebKitTools/Scripts/webkitpy/common/system/path_unittest.py:38
> +	   if platform == 'cygwin' and sys.platform != 'cygwin':
> +	       return

Sad we can't require python 2.7:
http://docs.python.org/library/unittest.html#skipping-tests-and-expected-failur
es

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:44
>      def clean_all_lockfile(self):

sigh.  This is not english, obviously not your fault.