[webkit-reviews] review denied: [Bug 53908] Get rid of code which writes test results from test_type/* classes. : [Attachment 81610] refactor-test-result-writing

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Hayato Ito <hayato at chromium.org>'s
request for review:
Bug 53908: Get rid of code which writes test results from test_type/* classes.
https://bugs.webkit.org/show_bug.cgi?id=53908

Attachment 81610: refactor-test-result-writing
https://bugs.webkit.org/attachment.cgi?id=81610&action=review

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:53
> +	       writer.copy_image_hash(driver_output.image_hash,
> +				      expected_driver_output.image_hash)

webkit doesn't have an 80char rule. I think this line and the ones below would
be easier to read on one line.

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:57
> +	       writer.copy_image_hash(driver_output.image_hash,
> +				      expected_image_hash=None)

ditto

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:61
> +	       writer.copy_image_hash(driver_output.image_hash,
> +				      expected_driver_output.image_hash)

ditto

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:75
> +    # whether iamges are same or not. So need this hack until we have a
better

typo: iamges

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:92
> +    FILENAME_SUFFIX_COMPARE = "-diff.png"

FILENAME_SUFFIX_IMAGE_DIFF ?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:95
> +	   """Initialize a TestTypeBase object.

This comment is wrong and not necessary anyways (i.e. clearly __init__
initializes). :)

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:109
> +	   """Creates the output directory (if needed) for a given test
> +	   filename."""

no need to wrap here

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:125
> +	   For example, if filename is c:/.../fast/dom/foo.html and modifier is

> +	   "-expected.txt", the return value is
> +	   c:/cygwin/tmp/layout-test-results/fast/dom/foo-expected.txt
> +
> +	   Args:
> +	     modifier: a string to replace the extension of filename with
> +
> +	   Return:
> +	     The absolute windows path to the output filename

I think these comments are outdated. I don't see anything windows specific
about these paths.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:131

>> +	def _write_into_file_at_path(self, file_path, contents, encoding):
> 
> You can get rid of the encoding parameter now, right? At which point, this
whole function might be kind of unnecessary.

Looks like it's still used to me. There one case where 'ascii' is passed in.
Although, I'm not convinced it's needed for the image hashes.

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:158
> +	   actual_filename = self.output_filename(self.FILENAME_SUFFIX_ACTUAL +

> +						  file_type)
> +	   expected_filename =
self.output_filename(self.FILENAME_SUFFIX_EXPECTED +
> +						    file_type)

ditto: re 80c wrapping. this would be more readable without wrapping.

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:160
> +	   # FIXME: This function is poorly designed.  We should be passing in
some
> +	   # sort of encoding information from the callers.

I'm confused by this FIXME. Aren't we already already passing the encoding from
the callers?

> Tools/Scripts/webkitpy/layout_tests/test_types/text_diff.py:69
>	       # Text doesn't match, write output files.

This comment no longer matches. It's a pretty useless comment anyways. Please
just delete it. :)