[webkit-reviews] review denied: [Bug 57987] new-run-webkit-tests: implement support for audio files : [Attachment 88524] 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 57987: new-run-webkit-tests: implement support for audio files
https://bugs.webkit.org/show_bug.cgi?id=57987

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

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

r- because no tests and until it's clear whether we have .txt and .wav or just
.wav.

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:85
> +	       for suffix in ['.txt', '.checksum', '.png', '.wav']:

Nit: [] to ()

> Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:241
> +	       if expected_audio == '':

'' or None?  It looks like this will be None if the file is not found.	Maybe
just change this to 'if not expected_audio:'

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_expectations.py:44
> +(PASS, FAIL, TEXT, IMAGE, IMAGE_PLUS_TEXT, AUDIO, AUDIO_PLUS_TEXT, TIMEOUT,

I thought these tests only generated .wav files (i.e., no text file)?

> Tools/Scripts/webkitpy/layout_tests/port/webkit.py:447
>	       elif line.startswith('Content-Type:'):
> -		   pass
> +		   content_type = ' '.split(line)[1]
> +	       elif line.startswith('Content-Transfer-Encoding:'):

Should Content-Type: and Content-Transfer-Encoding: be consts like
Content-Length:?