Bug 46241

<rdar://problem/8463568>

Created attachment 130426 [details] Proposed patch

Comment on attachment 130426 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=130426&action=review This patch looks straightforward to me. Notice that the code in LayoutTests/http/tests/media/resources/serve-video.php doesn't adhere to the WebKit style guide. Although we don't seem to strictly enforce these rules for test files and test support files, you may want to consider fixing the style so as to be consistent with the style guide either in this patch or in a follow up patch. > Source/WebCore/ChangeLog:3 > + Html 5 video element Useragent string is Quicktime Nit: "Html 5" => "HTML5" > LayoutTests/http/tests/media/resources/serve-video.php:12 > + $range = explode("-", substr($contentRange, 6)); I know that you extracted this code from file LayoutTests/http/tests/media/resources/video-referer-check-referer.php. The number 6 for the length parameter of substr() seems mysterious, especially for people who may not be familiar with the syntax for Byte Ranges (maybe we don't have to worry about this audience?) . For your consideration, I suggest either substituting the expression strlen("bytes=") or adding an inline comment that explains that 6 is the length of the bytes-unit string "bytes=".

(In reply to comment #3) > >[...] length of the bytes-unit string "bytes=". This should read: [...] length of the bytes-unit string plus "=".

(In reply to comment #3) > (From update of attachment 130426 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=130426&action=review > > Notice that the code in LayoutTests/http/tests/media/resources/serve-video.php doesn't adhere to the WebKit style guide. Although we don't seem to strictly enforce these rules for test files and test support files, you may want to consider fixing the style so as to be consistent with the style guide either in this patch or in a follow up patch. > Done. > > Source/WebCore/ChangeLog:3 > > + Html 5 video element Useragent string is Quicktime > > Nit: "Html 5" => "HTML5" > > > LayoutTests/http/tests/media/resources/serve-video.php:12 > > + $range = explode("-", substr($contentRange, 6)); > > I know that you extracted this code from file LayoutTests/http/tests/media/resources/video-referer-check-referer.php. > Well, I "extracted" code that I wrote in the first place so the fault is mine ;-) > The number 6 for the length parameter of substr() seems mysterious, especially for people who may not be familiar with the syntax for Byte Ranges (maybe we don't have to worry about this audience?) . For your consideration, I suggest either substituting the expression strlen("bytes=") or adding an inline comment that explains that 6 is the length of the bytes-unit string "bytes=". Done. Thanks for the review!