[webkit-reviews] review denied: [Bug 52604] new-run-webkit-tests: use filesystem wrapper in more places : [Attachment 79231] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 52604: new-run-webkit-tests: use filesystem wrapper in more places
https://bugs.webkit.org/show_bug.cgi?id=52604

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

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

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:51
> +	   if idx == -1:
> +	       idx = 0
> +	   return path[idx:]

If we find '/', won't this include '/' in the returned value?  E.g., basename
of '/foo/bar' would be '/bar' but it should be 'bar'.

I think you can just remove the -1 check and use path[idx + 1:].

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:78
> +    def glob(self, path):
> +	   if path[-1] == '*':
> +	       return [f for f in self.files if f.startswith(path[:-1])]
> +	   else:
> +	       return [f for f in self.files if f == path]

This doesn't handle cases where * is in the middle of the string, but it's
probably good enough for now.  Maybe mention that in a comment?

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:89
> +	   files = self.files.keys()[:]
> +	   return any(f.startswith(path) for f in files)

Why is this copy needed?

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:144
> +    def splitext(self, path):
> +	   idx = path.rfind('.')
> +	   if idx == -1:
> +	       idx = 0
> +	   return (path[0:idx], path[idx:])

This looks like it produces the wrong output if idx == -1.  E.g.,
splitext("/foo/bar") should return ("/foo/bar", ""), but this returns ("",
"/foo/bar").

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:148
> +	   # Ripped from Python 2.6's os.walk.
> +	   # equivalent to walk(top, topdown=True, followlinks=False).

If we're going to create a derivative work (i.e., copy/paste + modification),
we need to include the python license and copyright.  You could do it inline in
the file or add the license in a separate file-- I'm not sure how webkit
normally does this.