Bug 84365 - Media HTTP layout tests for simulating a variety of servers
Summary: Media HTTP layout tests for simulating a variety of servers
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andrew Scherkus
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-19 11:53 PDT by Andrew Scherkus
Modified: 2012-05-21 16:05 PDT (History)
8 users (show)

See Also:


Attachments
sample HTTP server and tests (11.03 KB, patch)
2012-04-19 11:58 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff
Patch (15.48 KB, patch)
2012-04-20 16:44 PDT, Andrew Scherkus
dbates: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Scherkus 2012-04-19 11:53:28 PDT
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.
Comment 1 Andrew Scherkus 2012-04-19 11:58:03 PDT
Created attachment 137940 [details]
sample HTTP server and tests

not for review!
Comment 2 Andrew Scherkus 2012-04-20 16:44:02 PDT
Created attachment 138197 [details]
Patch
Comment 3 Andrew Scherkus 2012-04-20 16:47:16 PDT
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.
Comment 4 Adam Barth 2012-05-21 14:37:10 PDT
Dan "the man" Bates, any chance you're interested in a Perl review?
Comment 5 Adam Barth 2012-05-21 14:37:11 PDT
Dan "the man" Bates, any chance you're interested in a Perl review?
Comment 6 Andrew Scherkus 2012-05-21 15:23:45 PDT
note: I am completely new to perl so feel free to drop knowledge on me
Comment 7 Daniel Bates 2012-05-21 16:05:18 PDT
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.