[webkit-reviews] review denied: [Bug 49573] new-run-webkit-tests: rename TestInfo to TestInput, move image hash to work thread : [Attachment 73947] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 49573: new-run-webkit-tests: rename TestInfo to TestInput, move image hash
to work thread
https://bugs.webkit.org/show_bug.cgi?id=49573

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

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

>>
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_threa
d.py:193
>> +	    if _needs_image_checksum(self._options):
> 
> I am not sure what is a correct behaviour, but it seems your patch will
change the behaviour. I mean:
> 
> Before your patch:
>   If test_args.new_baseline is true, self._driver.run_test() always receives
'None' as an image_hash.
> 
> After your patch:
>   If test_args.new_baseline is true, self._driver.run_test() may receive
non-None value as a image_hash if a test has a '.checksum' file.
> 
> I am wondering that is your intentional or not.

Can we make _needs_image_checksum a method of TestInput and give TestInput a
populate_checksum method that takes port and options as arguments? Maybe it
should be populate_checksum_and_uri. Then the filename_to_uri logic could go
back into TestInput as well.