[Webkit-unassigned] [Bug 56586] Media Stream API: add the getUserMedia method and the Javascript bindings

[bugzilla-daemon at webkit.org]

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





--- Comment #35 from Leandro Graciá Gil <leandrogracia at chromium.org>  2011-04-07 07:51:20 PST ---
(From update of attachment 87905)
View in context: https://bugs.webkit.org/attachment.cgi?id=87905&action=review

>> LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:16
>> +            shouldThrow(expression, '(function() { return "' + expectedException + '"; })();');
> 
> Do you use this functionality?

We use it now. Fixed.

>> LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:35
>> +// 1 Argument
> 
> Maybe add a comment that the reason all these throw is not due to the type conversion, but simply because the API requires 2 arguments. Test with emptyFunction too for completeness.

Fixed.

>> LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:40
>> +test('navigator.webkitGetUserMedia("video")', true);
> 
> Could use "video" for the other arguments below, too.

Fixed.

>> LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:52
>> +test('navigator.webkitGetUserMedia(objectThrowingException, emptyFunction)', false);
> 
> Shouldn't this one throw 'valueOf threw exception'? Or does toString() not use valueOf()? Maybe try an object with a custom toString() that throws?

Fixed.

>> Source/WebCore/DerivedSources.make:780
>> +endif
> 
> Usually we add new files to the makefiles for all ports, but leave it up to that port to add the machinery for flags etc.

Fixed.

>> Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:4
>> + * Redistribution and use in source and binary forms, with or without
> 
> Wrong license - http://trac.webkit.org/browser/trunk/Source/WebKit/LICENSE - throughout

Fixed.

>> Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:48
>> +static void throwTypeMismatchException()
> 
> This function and constant are copied from Geolocation, right? Is it used elsewhere too? It would be good to move it to a common place and share it. Maybe do this as a separate patch to keep this one manageable.

Fixed. Integrating into V8Utilities in https://bugs.webkit.org/show_bug.cgi?id=57760.

>> Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:53
>> +static PassRefPtr<NavigatorUserMediaSuccessCallback> createNavigatorUserMediaSuccessCallback(v8::Local<v8::Value> value, bool& succeeded)
> 
> This method and the one below are very similar to each other and to corresponding methods in Geolocation. How about we factor them out by adding allowNull and allowUndefined arguments? Are there other places in existing code that could use such helpers?

Fixed. Integrating into V8Utilities in https://bugs.webkit.org/show_bug.cgi?id=57760.

>> Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:97
>> +    if (!succeeded || !successCallback)
> 
> Is the check for successCallback required? Can V8NavigatorUserMediaSuccessCallback::create return 0?

Fixed.

>> Source/WebCore/page/CallbackTask.h:84
>> +class CallbackTask2 : public ScriptExecutionContext::Task {
> 
> It looks like CallbackTask2 isn't used in this patch? Also, it's not clear from this patch why the error callback needs to implement CallbackTask1::Scheduler, as the functionality isn't used yet.
> 
> I think it's better to take CallbackTask out of this patch. It can then be added separately before we add the UserMedia functionality that actually requires it.

Fixed.

>> Source/WebCore/page/NavigatorUserMediaError.h:49
>> +    unsigned short code() const { return m_code; }
> 
> The constructor param and this getter should use the enum type.

Fixed.

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