[webkit-reviews] review denied: [Bug 47220] new-run-webkit-tests - enable cygwin support for chromium win : [Attachment 70702] 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 70702: Patch
https://bugs.webkit.org/attachment.cgi?id=70702&action=review

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

I'm generally a fan of this change.  I feel like there is a bit much going on
for my little brain to follow it all, but that may still be OK.

Paths forward:
1. Use the magic of git (commit -i perhaps?) to split out the simple changes
here and just get them rubberstamped and landed.
2. Land pretty-much as is and iterate from there.

If 1. sounds easy to you, lets do that.  Otherwise I could be convinced on #2.

If you want to go down #2, post a patch for review with the nits fixed and I'll
r+ it.	Patches tend to be easier to understand on a second reading anyway.

Thanks!

> WebKitTools/Scripts/webkitpy/common/system/path.py:78
> +_cygpath_global_lock = None
> +_cygpath_global_object = None

Can we make these class members and have them still be global?	Then you can
use self. or cls. to access them instead of global.

> WebKitTools/Scripts/webkitpy/common/system/path.py:81
> +def cygpath(path):

This method is a bit difficult to process at this size.  I wonder if there is
something which coudl be broken out of this?

> WebKitTools/Scripts/webkitpy/layout_tests/deduplicate_tests_unittest.py:210
> +if __name__ == '__main__':
> +    unittest.main()

We've slowly been removing these and just running individual modules using
"test-webkitpy webkitpy.layout_tests.deduplicate_tests_unittest" for example.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:354
> +    def _convert_path(self, path):

Why is _convert_path a port thing?  Seems it's a FileSystem thing.  Maybe just
a path() method in a common.filesystem.py?  Doesn't have to be in this patch,
but just thinking about the proper long term place for this.  Especially with
your previous tool.filesystem() proposal for handling mocking open, exists,
etc. (Which Adam and I both are *huge* fans of.)

Oh, I guess we have a path module already.  Maybe the free-function goes there?
 (I'm failing to come up with a good (ideally short) name...

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:381
> +	       # See note above in diff_image() for why we need
> +	       # _convert_path().

Did we just miss the 80c boundary? ;)

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py:382
> +	       driver_args.append("--pixel-tests=" +

I wonder if we should have a fancy Args class which knows how to handle things
like this.  A special append_path, method for example.	Donno.	Just a thought
for the future.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:182
> +		   else:
> +		       return ''

WebKit has a policy of avoiding else after return (at least for our C++ code,
and I think it should apply to python as well).

> WebKitTools/Scripts/webkitpy/layout_tests/port/http_lock_unittest.py:46
> +	       os.unlink(self.guard_lock_file)

Seems this should have been _guard_lock_file (but obviously that's a separate
change.