[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 #31 from Steve Block <steveblock at google.com>  2011-04-04 06:37:22 PST ---
(From update of attachment 87905)
View in context: https://bugs.webkit.org/attachment.cgi?id=87905&action=review

Need to update the Android makefiles and fix the EFL build.

I don't see a test for that fact that NavigatorUserMediaError is marked NoInterfaceObject? Also, can you add a test that navigator.webkitGetUserMedia() is present and enumerable, like fast/dom/Geolocation/enabled.html.

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

Do you use this functionality?

> LayoutTests/fast/dom/MediaStream/script-tests/argument-types.js:76
> +test('navigator.webkitGetUserMedia("video", emptyFunction, -Infinity)', true);

I don't think that any of these should throw, other than the one using objectThrowingException. I think the argument should just get converted to a string using the normal JS conversions.

> 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.

> 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

> 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.

> 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?

> Source/WebCore/bindings/v8/custom/V8NavigatorCustom.cpp:97
> +    if (!succeeded || !successCallback)

Is the check for successCallback required? Can V8NavigatorUserMediaSuccessCallback::create return 0?

> 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.

> Source/WebCore/page/NavigatorUserMediaError.h:49
> +    unsigned short code() const { return m_code; }

The constructor param and this getter should use the enum type.

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