[webkit-reviews] review granted: [Bug 84365] Media HTTP layout tests for simulating a variety of servers : [Attachment 138197] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Andrew Scherkus
<scherkus at chromium.org>'s request for review:
Bug 84365: Media HTTP layout tests for simulating a variety of servers
https://bugs.webkit.org/show_bug.cgi?id=84365

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
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.