[Webkit-unassigned] [Bug 64731] Add MediaSource API to HTMLMediaElement
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Aug 29 15:51:38 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=64731
--- Comment #31 from Aaron Colwell <acolwell at chromium.org> 2011-08-29 15:51:37 PST ---
(From update of attachment 103985)
View in context: https://bugs.webkit.org/attachment.cgi?id=103985&action=review
>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:8
>> + var video_tag = event.target;
>
> Indentation should be consistent, not a mixture of 2 and 4 tab spaces.
Done.
>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:11
>> + 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.
Done.
>> 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.
Done.
>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:45
>> + }
>
> WebKit convention is to put a function's first brace on its own line:
>
> function didNotSendHeadersFirst(event)
> {
> appendCluster(event.target, 0);
> }
Done.
>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:54
>> + });
>
> Nit: declaring next_cluster_id as a global would have made this and the other places it is used slightly easier to understand.
Done.
>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:130
>> + };
>
> Minor Nit: Having a blank line between tests results would make them slightly easier to read.
Done
>> LayoutTests/http/tests/media/media-source/webm/video-media-source-seek.html:28
>> + 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)".
Done.
>> LayoutTests/http/tests/media/media-source/webm/video-media-source-seek.html:45
>> + }
>
> Ditto.
Done.
>> 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.
Done.
>> LayoutTests/http/tests/media/media-source/webm/webm-media-source.js:94
>> +function appendUntilEndOfStream(video_tag, startIndex) {
>
> Ditto.
Done.
>> Source/WebKit/chromium/features.gypi:64
>> 'ENABLE_MEDIA_STATISTICS=1',
>
> Do you mean to turn this feature on for all Chromium builds even though it isn't complete?
No. I'm going to create a follow-on patch that will make the media source methods enabled at runtime so that Chromium can control this w/ a command-line flag. I'll add this line back in that patch.
--
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