Bug 130452 – Improving webkitGetUserMedia error handling and error messages

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Thiago de Barros Lacerda]

Checking if first argument of webkitGetUserMedia is a valid Dictionary. If not, throw an exception with a clearer message of the error.

[Thiago de Barros Lacerda]

Created attachment 227182 [details]
Patch

[Thiago de Barros Lacerda]

(In reply to comment #1)
> Created an attachment (id=227182) [details]
> Patch

This patch does not contain the modifications needed in apple build files. Eric, could you provide that?

[Eric Carlson]

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=227182) [details] [details]
> > Patch
> 
> This patch does not contain the modifications needed in apple build files. Eric, could you provide that?

Don't worry about the Xcode modifications, I will fix that later when I get a chance to work on this.

[Eric Carlson]

Comment on attachment 227182 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227182&action=review

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:48
> +    ExceptionCode ec = 0;

Nit: this can be declared just before it is used.

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:59
> +        throwVMTypeError(exec, makeDOMBindingsTypeErrorString("Argument ", "2", " ('", "successCallback", "') to ", "Navigator", ".", "webkitGetUserMedia", " must be a function"));

I think the reason these error messages are split up is to decrease the number of unique strings in the binary, so you can combine:

    ", "Navigator", ".", "webkitGetUserMedia", " must be a function"

to make:

    "') to Navigator.webkitGetUserMedia must be a function"

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:65
> +    JSValue thisValue = exec->hostThisValue();
> +    JSNavigator* castedThis = jsDynamicCast<JSNavigator*>(thisValue);
> +    RefPtr<NavigatorUserMediaSuccessCallback> successCallback = JSNavigatorUserMediaSuccessCallback::create(asObject(exec->uncheckedArgument(1)), castedThis->globalObject());

Nit: there is no reason to do this if the error function is invalid.

[Thiago de Barros Lacerda]

(In reply to comment #4)
> (From update of attachment 227182 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227182&action=review
> 
> > Source/WebCore/bindings/js/JSNavigatorCustom.cpp:48
> > +    ExceptionCode ec = 0;
> 
> Nit: this can be declared just before it is used.
OK

> 
> > Source/WebCore/bindings/js/JSNavigatorCustom.cpp:59
> > +        throwVMTypeError(exec, makeDOMBindingsTypeErrorString("Argument ", "2", " ('", "successCallback", "') to ", "Navigator", ".", "webkitGetUserMedia", " must be a function"));
> 
> I think the reason these error messages are split up is to decrease the number of unique strings in the binary, so you can combine:
> 
>     ", "Navigator", ".", "webkitGetUserMedia", " must be a function"
> 
> to make:
> 
>     "') to Navigator.webkitGetUserMedia must be a function"

OK

> 
> > Source/WebCore/bindings/js/JSNavigatorCustom.cpp:65
> > +    JSValue thisValue = exec->hostThisValue();
> > +    JSNavigator* castedThis = jsDynamicCast<JSNavigator*>(thisValue);
> > +    RefPtr<NavigatorUserMediaSuccessCallback> successCallback = JSNavigatorUserMediaSuccessCallback::create(asObject(exec->uncheckedArgument(1)), castedThis->globalObject());
> 
> Nit: there is no reason to do this if the error function is invalid.

OK

[Thiago de Barros Lacerda]

Created attachment 227206 [details]
Requested changes

[Eric Carlson]

Comment on attachment 227206 [details]
Requested changes

View in context: https://bugs.webkit.org/attachment.cgi?id=227206&action=review

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:58
> +        throwVMError(exec, createTypeError(exec, "Argument 2 ('successCallback') to Navigator.webkitGetUserMedia must be a function"));

"to Navigator.webkitGetUserMedia must be a function" is common to both error messages, you should split this:

"Argument 2 ('successCallback')", "to Navigator.webkitGetUserMedia must be a function"

> Source/WebCore/bindings/js/JSNavigatorCustom.cpp:66
> +            throwVMError(exec, createTypeError(exec, "Argument 3 ('errorCallback') to Navigator.webkitGetUserMedia must be a function"));

And this:

"Argument 3 ('errorCallback')", "to Navigator.webkitGetUserMedia must be a function"

[WebKit Commit Bot]

Comment on attachment 227206 [details]
Requested changes

Clearing flags on attachment: 227206

Committed r165915: <http://trac.webkit.org/changeset/165915>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.