Bug 84365 – Media HTTP layout tests for simulating a variety of servers

• Status: NEW
• Component: Media

[Andrew Scherkus]

Experimental!

Sample layout test for generating various responses as well as a perl script for generating WAV data on the fly to avoid checking in huge files.

[Andrew Scherkus]

Created attachment 137940 [details]
sample HTTP server and tests

not for review!

[Andrew Scherkus]

Created attachment 138197 [details]
Patch

[Andrew Scherkus]

Alrighty Eric I wrapped this up and I think it's ready for review!

For now I kept the tests rather simple but I'm not 100% happy with them so feel free to r- and we can think of some better tests.

I'm waiting on my Mac build to finish so I can test it out on Mac DRT. So far trying out the autogenerated WAV URLs on Safari/QuickTime seemed to work.

[Adam Barth]

Dan "the man" Bates, any chance you're interested in a Perl review?

[Adam Barth]

Dan "the man" Bates, any chance you're interested in a Perl review?

[Andrew Scherkus]

note: I am completely new to perl so feel free to drop knowledge on me

[Daniel Bates]

Comment on attachment 138197 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138197&action=review

Looks sane to me. I had some very minor nits.

> LayoutTests/http/tests/media/Wav.pm:10
> +# To keep things simple when calcuating byte offsets, we limit this class to single-channel

Nit: calcuating => calculating.

> LayoutTests/http/tests/media/Wav.pm:18
> +        bits_per_sample => 8,

Nit: We follow the WebKit Code Style guidelines for Perl code. In particular, we use camel case for variables names.

> LayoutTests/http/tests/media/Wav.pm:30
> +                           16,                        # Subchunk1Size

From reading <https://ccrma.stanford.edu/courses/422/projects/WaveFormat/>, this length is specific to PCM. You may want to consider denoting it as such like you did for format on the line below.

> LayoutTests/http/tests/media/Wav.pm:43
> +sub byte_rate {

Nit: We follow the WebKit Code Style guidelines for Perl code. In particular, we use camel case for function/subroutine names.

> LayoutTests/http/tests/media/video-response.cgi:33
> +# Handle HTTP range requests by parsing "Range: bytes=N-M" headers.

Nit: You may want to consider moving this comment inside the if-block. See my remark on line 44 for more details.

> LayoutTests/http/tests/media/video-response.cgi:45
> +# Handle "Range: bytes=N-" requests, but special case "Range: bytes=0-" requests to
> +# return a 200 response if $return200 is true.

Nit: This is OK as-is. This looks stylistically weird since the comment talks about the next conditional block, but is visually positioned in the body of this if-statment block. You may want to consider moving this comment inside the block (starting on line 47) that it describes.

Alternatively, you could write this if-elsif-else block as three if-blocks that call a common function and then return. Then the comment for the block could precede the if-statement. The common function would  write out the Accept-Ranges, Content-Type, and Connection headers, and the WAV data

> LayoutTests/http/tests/media/video-response.cgi:56
> +# Return a 200 response in all other cases.

Nit: Similarly, you may want to consider moving this comment into the else block below.