[Webkit-unassigned] [Bug 64731] Add MediaSource API to HTMLMediaElement
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Aug 17 12:00:04 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=64731
Eric Carlson <eric.carlson at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #103985|review? |review+
Flag| |
--- Comment #30 from Eric Carlson <eric.carlson at apple.com> 2011-08-17 12:00:03 PST ---
(From update of attachment 103985)
View in context: https://bugs.webkit.org/attachment.cgi?id=103985&action=review
Marking r+ although I would prefer to have the stylistic nits addressed before it is landed.
I think that it is important to run test for basic functionality like those in LayoutTests/media/ when data is loaded with this API. It would be great if there was a way to reuse the existing tests so we are sure there aren't any subtle errors caused by the use of this API, but I can't immediately think of a way to make that work.
> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:8
> + <head>
> + <script src="../../../media-resources/video-test.js"></script>
> + <script src="webm-media-source.js"></script>
> + <script>
> + function onError(event) {
> + var video_tag = event.target;
Indentation should be consistent, not a mixture of 2 and 4 tab spaces.
> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:11
> + var error_string = "UNKNOWN";
> + switch(video_tag.error.code) {
Nit: WebKit tests almost always use camelCase for JavaScript variables. I was going to say that *all* WebKit tests do this but then I found a few WebGL tests... None-the-less, I would prefer to see these tests follow the convention used in all other media tests.
> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:29
> + function appendClustersUntilMetadataLoaded(video_tag, done_callback) {
Very minor nit: this function name is confusing, I assumed that it would append clusters until the event, not just append two and wait.
> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:45
> + function didNotSendHeadersFirst(event) {
> + appendCluster(event.target, 0);
> + }
WebKit convention is to put a function's first brace on its own line:
function didNotSendHeadersFirst(event)
{
appendCluster(event.target, 0);
}
> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:54
> + appendClustersUntilMetadataLoaded(video_tag, function(next_cluster_id) {
> + appendCluster(video_tag, next_cluster_id - 1);
> + });
Nit: declaring next_cluster_id as a global would have made this and the other places it is used slightly easier to understand.
> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:130
> + var event_handler_function = function (event) {
> + consoleWrite("running " + function_name);
> + event.target.removeEventListener('webkitsourceopen', event_handler_function);
> + on_open_function(event);
> + };
Minor Nit: Having a blank line between tests results would make them slightly easier to read.
> LayoutTests/http/tests/media/media-source/webm/video-media-source-seek.html:28
> + if (video_tag.currentTime > 2 && do_seek) {
> + consoleWrite("EVENT(timeupdate) : seeking to " + seek_time);
> + video_tag.pause();
> + video_tag.currentTime = seek_time;
> + do_seek = false;
Nit: logging a float with full precision caused platform specific failures in other media tests, so tests were changed to log and compare "value.toFixed(2)".
> LayoutTests/http/tests/media/media-source/webm/video-media-source-seek.html:45
> + if (current_time != seek_time ) {
> + failTest("Seeked to " + current_time + " instead of " + seek_time);
> + return;
> + }
Ditto.
> LayoutTests/http/tests/media/media-source/webm/webm-media-source.js:80
> +function appendCluster(video_tag, clusterIndex) {
Nit: Please use consistent naming style: camelCase or underscores.
> LayoutTests/http/tests/media/media-source/webm/webm-media-source.js:94
> +function appendUntilEndOfStream(video_tag, startIndex) {
Ditto.
> Source/WebKit/chromium/features.gypi:64
> 'ENABLE_METER_TAG=1',
> + 'ENABLE_MEDIA_SOURCE=1',
> 'ENABLE_MEDIA_STATISTICS=1',
Do you mean to turn this feature on for all Chromium builds even though it isn't complete?
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list