[webkit-reviews] review denied: [Bug 52136] Add TestOutput classes to webkitpy. : [Attachment 79891] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied James Kozianski <koz at chromium.org>'s
request for review:
Bug 52136: Add TestOutput classes to webkitpy.
https://bugs.webkit.org/show_bug.cgi?id=52136

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79891&action=review

This is close. Just a few more things...

> Tools/Scripts/webkitpy/common/net/testoutput.py:45
> +	   self._main_file = files[0]

This _main_file stuff is just confusing. It seems like you only need
"contents()" for checking same_contents(), right? so how about getting rid of
contents() and instead making same_contents abstract and have the subclasses
implement it since it seems subclass specific.

> Tools/Scripts/webkitpy/common/net/testoutput.py:60
> +	   test_name = filename

this line doesn't seem to add much.

> Tools/Scripts/webkitpy/common/net/testoutput.py:105
> +    def is_newer_than(self, other):

?

> Tools/Scripts/webkitpy/common/net/testoutput.py:128
> +    def _path_to_platform(self, platform):

Why pass in platform instead of just using self._platform?

> Tools/Scripts/webkitpy/common/net/testoutput.py:135
> +    def _install_file(self, file, path):

s/_install_file/_save_expected_result/ ? (see below)

> Tools/Scripts/webkitpy/common/net/testoutput.py:141
> +    def install(self, path_to_layout_tests):

install is a weird word for this. how about..."save_expected_results"?

> Tools/Scripts/webkitpy/common/net/testoutput.py:149
> +    def delete(self):

why do we need this?

> Tools/Scripts/webkitpy/common/net/testoutput.py:181
> +	   if self.has_checksum() and other.has_checksum():
> +	       return self._checksum_file.contents() ==
other._checksum_file.contents()

We shouldn't assume the checksums actually match the image files. It has def
happened that people commit the new checksum but forget to commit the new
image. In either case, a FIXME is sufficient for now.

> Tools/Scripts/webkitpy/common/net/testoutputset.py:33
> +    """A set of test outputs"""

This comment doesn't add anything over the class name. Just delete it. It's
pretty clear what this class does.

> Tools/Scripts/webkitpy/common/net/testoutputset.py:129
> +    def sub_builders(self):

what is "sub" about these builders?