[Webkit-unassigned] [Bug 64731] Add MediaSource API to HTMLMediaElement

Mon Aug 15 16:35:11 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=64731

--- Comment #28 from Aaron Colwell <acolwell at chromium.org>  2011-08-15 16:35:10 PST ---
(From update of attachment 102925)
View in context: https://bugs.webkit.org/attachment.cgi?id=102925&action=review

>> LayoutTests/http/tests/media/media-source.js:23
>> +  r.send();
> 
> Nit: We try to use descriptive names for variables in WebKit.

Done

>> LayoutTests/http/tests/media/media-source.js:25
>> +  if (r.status != 200) {
> 
> Nit: Although there isn't a formal standard for tab width in JS files, I believe that all of the media tests use 4-tab spaces.

Done

>> LayoutTests/http/tests/media/media-source.js:39
>> +}
> 
> Nit: We try to use full names instead of abbreviations, eg. getClusterCount, getNumberOfClusters, etc.

Done.

>> LayoutTests/http/tests/media/media-source.js:41
>> +function getCluster(i) {
> 
> Nit: descriptive variable names are preferred. Here and throughout the rest of the file.

Done.

>> LayoutTests/http/tests/media/media-source.js:119
>> +    case 2:
> 
> Is there any reason to not use the named constants, eg. "case HTMLMediaElement.SOURCE_CLOSED:", etc?

Nope. Fixed.

>> LayoutTests/http/tests/media/video-media-source-errors.html:1
>> +<html>
> 
> Nit: Is there any reason to not include an html5 DOCTYPE? Here and elsewhere.

Nope. Done.

>> LayoutTests/http/tests/media/video-media-source-errors.html:35
>> +        window.setTimeout(f, 0, next_cluster_index + 1);
> 
> Is there any way to do this with an event listener instead of setTimeout (here and elsewhere)? Timeouts have been the source of lots of flakiness in media tests.

Yes. Removed.

>> LayoutTests/http/tests/media/video-media-source-seek.html:40
>> +       if (Math.abs(currentTime - seekTime) > 0.001) {
> 
> Why do you need the magic number? At the very least you should add a comment about why this is needed and what this tolerance is OK. How much variation do you actually see?

This was just to compensate for the fact that I only went to 3 decimal places on the timestamps in the cluster_info table. I've replaced the values in the table with full precision values and removed this magic number code.

>> LayoutTests/http/tests/media/video-media-source-state-changes.html:17
>> +      window.setTimeout(appendUntilEnd, 0, v, i + 1);
> 
> More setTimeout.

Removed.

>> LayoutTests/http/tests/media/video-media-source-state-changes.html:23
>> +        return
> 
> Missing a semi-colon here.

Done.

>> LayoutTests/http/tests/media/video-media-source-state-changes.html:62
>> +      window.setTimeout(appendUntilEnd, 0, v, 0);
> 
> More setTimeout.

Removed.

>> LayoutTests/http/tests/media/video-media-source-state-changes.html:88
>> +                        function () { firstSeekClusterAppendComplete(v);} );
> 
> More setTimeout.

Removed.

>> LayoutTests/http/tests/media/video-media-source-state-changes.html:120
>> +    }
> 
> More setTimeout.

Removed.

>> LayoutTests/http/tests/media/video-media-source-state-changes.html:159
>> +      window.setTimeout(appendUntilEnd, 0, v, 0);
> 
> More setTimeout.

Removed.

> Source/WebCore/html/HTMLMediaElement.cpp:571
>          m_readyState = HAVE_NOTHING;

Oops. Good catch. I only need one and I've moved it slightly earlier in the method. I've also moved the location of a similar call in userCancelledLoad() so that the 2 methods fire abortEvent, webkitsourceclose, and emptied in the same order.

>> Source/WebCore/html/HTMLMediaElement.idl:104
>> +    // media source state
> 
> Comment should be a complete sentence.

Done. I was just copying the comment style from the networkState & readyState variables above.

>> Source/WebKit/chromium/ChangeLog:19
>> +
> 
> I am going to assume that you have had someone familiar with Chromium's embedder API review these changes, because I am not qualified.

Ok. I'll have some Chromium WebKit reviewers take a look.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.