[webkit-reviews] review granted: [Bug 124262] Add support for HTMLMediaElement.fastSeek() : [Attachment 216808] Patch

Wed Nov 13 09:46:41 PST 2013

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 124262: Add support for HTMLMediaElement.fastSeek()
https://bugs.webkit.org/show_bug.cgi?id=124262

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

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=216808&action=review

> Source/WebCore/Modules/mediacontrols/mediaControlsApple.js:691
> +	   // Do a precise seek when we lift the mouse:
> +	   this.video.currentTime = this.controls.timeline.value;

Nice!

> Source/WebCore/html/HTMLMediaElement.idl:78
> +[RaisesException] void fastSeek(double time);

It doesn't look like this (or currentTime) is supposed to throw.

> LayoutTests/media/video-fast-seek.html:4
> +<p>Test that fastSeek() commands work correctly
> + </p>
> +<script src=media-file.js></script>

Odd indentation here.

It would be nice to stick the <script> in a <head>, include a <!DOCTYPE html>,
etc.

> LayoutTests/media/video-fast-seek.html:9
> +    // The test.mp4 file has sync samples at the following presentation time
stamps:
> +    // 0.0000, 0.7968, 1.5936, 2.3904, 3.1872, 3.9840, 4.7808, 5.5776

This mentions test.mp4, but uses findMediaFile() so other ports could end up
with a different file. It may be worth renaming this test and hard coding the
test.mp4.