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

Tue Aug 30 13:28:52 PDT 2011

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

Aaron Colwell <acolwell at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #105535|0                           |1
        is obsolete|                            |
 Attachment #105535|review+                     |
               Flag|                            |

--- Comment #34 from Aaron Colwell <acolwell at chromium.org>  2011-08-30 13:28:49 PST ---
(From update of attachment 105535)
View in context: https://bugs.webkit.org/attachment.cgi?id=105535&action=review

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:11
>> +    var errorString = "UNKNOWN";
> 
> Now your indentation is using a mixture of *one* and four space tabs:
> 
> <html>
> +<head>
> ++<script src="webm-media-source.js"></script>
> ++<script>
> function onError(event)
> {
> ++++var videoTag = event.target;
> 
> Please be consistent - use the same size tabs throughout for JS and markup. Indenting the markup and leaving the JS flush left is unusual.
> 
> Please pick a tab size and stick with it. As I noted before, the rest of the media tests use four space tabs so I would prefer that:
> 
> <html>
> ++++<head>
> ++++++++<script src="webm-media-source.js"></script>
> ++++++++<script>
> ++++++++++++function onError(event)
> ++++++++++++{
> ++++++++++++++++var videoTag = event.target;
> 
> In any case, be consistent. This may seem a niggling complaint, but indentation is meant to make code easier to read and comprehend and to make structure obvious. This is especially important in a big project like WebKit, and I don't think your current style succeeds on either account.

Done. Sorry about this. I was only focusing on the JS last time and my editor wasn't helping me with the HTML/JS mix. :(

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:60
>> +        });
> 
> The closing brace should line up with the beginning of the line with the opening brace, as it does in the "eventHandler" function in appendEnoughClustersToTriggerMetadataLoaded above.

Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:75
>> +        });
> 
> The closing brace should line up with the beginning of the line with the opening brace

Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:101
>> +        });
> 
> Ditto.

Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:123
>> +                 ];
> 
> Ditto, here and throughout the rest of the patch.

Done.

>> Source/WebCore/ChangeLog:20
>> +        (WebCore::HTMLMediaElement::loadResource):
> 
> It is helpful to have function by function explanations of what changed in your changelog.

Ok. Will when I upload the new patch.

>> Source/WebCore/html/HTMLMediaElement.cpp:1297
>> +    bool noSeekRequired = (!seekableRanges->length() || (time == now && displayMode() != Poster));
> 
> Is this change necessary?

Nope. Removed.

>> Source/WebCore/html/HTMLMediaElement.h:149
>> +    void webkitSourceEndOfStream(unsigned short status, ExceptionCode&);
> 
> Is there any reason to not have "status" be an EndOfStreamStatus instead of an unsigned short?

It doesn't compile if I use EndOfStreamStatus. Clang complains w/ this error.

In file included from gen/webkit/bindings/V8DerivedSources06.cpp:58:
gen/webcore/bindings/V8HTMLMediaElement.cpp:449:34: error: cannot initialize a parameter of type 'WebCore::HTMLMediaElement::EndOfStreamStatus' with an lvalue of type 'int'
    imp->webkitSourceEndOfStream(status, ec);

I think this has to do with how I have the method declared in the IDL. I was modelling my code after the readyState & networkState attributes. I haven't seen a example of an enum in IDL that doesn't use unsigned short. If you can point me to an example, I'd be happy to change this code.

>> Source/WebCore/html/HTMLMediaElement.idl:110
>> +#endif
> 
> Can these use "[Conditional= ENABLE_MEDIA_SOURCE]" instead of "#if definedENABLE_MEDIA_SOURCE"? See https://bugs.webkit.org/show_bug.cgi?id=64231 and https://bugs.webkit.org/show_bug.cgi?id=64961.

So it appears that the short answer to this is no. It looks like the V8 code generator doesn't fully support Conditional on constants and functions. I'm getting all sorts of compile errors in the generated code that indicate some of the code isn't aware that the method/constant should be disabled. 

I can track down what is going on with the code generator, but for now I'd just like to leave this as is. I'll circle back and update this once the code generator is fixed.

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