| Summary: | Media HTTP layout tests for simulating a variety of servers | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Andrew Scherkus <scherkus> | ||||||
| Component: | Media | Assignee: | Andrew Scherkus <scherkus> | ||||||
| Status: | NEW --- | ||||||||
| Severity: | Normal | CC: | abarth, ap, dbates, eric.carlson, eric, feature-media-reviews, pnormand, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Andrew Scherkus
2012-04-19 11:53:28 PDT
Created attachment 137940 [details]
sample HTTP server and tests
not for review!
Created attachment 138197 [details]
Patch
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. Dan "the man" Bates, any chance you're interested in a Perl review? Dan "the man" Bates, any chance you're interested in a Perl review? note: I am completely new to perl so feel free to drop knowledge on me 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. |