[webkit-reviews] review denied: [Bug 50901] Add remote zip file handling to webkitpy. : [Attachment 77865] Patch

[bugzilla-daemon at webkit.org]

Mihai Parparita <mihaip at chromium.org> has denied James Kozianski
<koz at chromium.org>'s request for review:
Bug 50901: Add remote zip file handling to webkitpy.
https://bugs.webkit.org/show_bug.cgi?id=50901

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

------- Additional Comments from Mihai Parparita <mihaip at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=77865&action=review

> Tools/Scripts/webkitpy/common/directoryfileset.py:30
> +from webkitpy.common.net.zipfileset import FileSetFileHandle

Having DirectoryFileSet depend on zipfileset seems wrong. Can there be a
(Base)FileSet class/file for code that is used in both?

> Tools/Scripts/webkitpy/common/directoryfileset.py:39
> +	   self._filesystem = filesystem if filesystem != None else
FileSystem()

This can just be self._filesystem = filesystem or Filesystem()

> Tools/Scripts/webkitpy/common/directoryfileset.py:72
> +	   # directory heirarchy exists at the output path.

Typo ("heirarchy")

> Tools/Scripts/webkitpy/common/directoryfileset_unittest.py:35
> +	   self._files = {}

Seems like _files doesn't need to be a property of the class (and you don't
need an addFile method), given that you're using a MockFileSystem. Instead you
could just say:
def setUp(self):
  files = {
   '/test/some-file': 'contents',
  ...
  }
  self._fileset = DirectoryFileSet('/test', MockFileSystem(files))

>> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:116

> 
> This is  just [file for file in self.files if file.startswith(path)]

Though this should also handle directory names that are substrings of one
another. I.e. if files is:
/foo/bar
/foo2/bar

Then files_under('/foo') should not include ('foo2/bar). 

(see how listdir is implemented to avoid this)