Bug 130452 - Improving webkitGetUserMedia error handling and error messages
Summary: Improving webkitGetUserMedia error handling and error messages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thiago de Barros Lacerda
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-19 07:34 PDT by Thiago de Barros Lacerda
Modified: 2014-03-19 14:07 PDT (History)
15 users (show)

See Also:


Attachments
Patch (19.93 KB, patch)
2014-03-19 07:44 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff
Requested changes (19.40 KB, patch)
2014-03-19 12:06 PDT, Thiago de Barros Lacerda
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago de Barros Lacerda 2014-03-19 07:34:35 PDT
Checking if first argument of webkitGetUserMedia is a valid Dictionary. If not, throw an exception with a clearer message of the error.
Comment 1 Thiago de Barros Lacerda 2014-03-19 07:44:19 PDT
Created attachment 227182 [details]
Patch
Comment 2 Thiago de Barros Lacerda 2014-03-19 07:45:11 PDT
(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?
Comment 3 Eric Carlson 2014-03-19 09:35:18 PDT
(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.
Comment 4 Eric Carlson 2014-03-19 10:03:44 PDT
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.
Comment 5 Thiago de Barros Lacerda 2014-03-19 11:55:14 PDT
(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
Comment 6 Thiago de Barros Lacerda 2014-03-19 12:06:00 PDT
Created attachment 227206 [details]
Requested changes
Comment 7 Eric Carlson 2014-03-19 13:30:58 PDT
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"
Comment 8 WebKit Commit Bot 2014-03-19 14:07:06 PDT
Comment on attachment 227206 [details]
Requested changes

Clearing flags on attachment: 227206

Committed r165915: <http://trac.webkit.org/changeset/165915>
Comment 9 WebKit Commit Bot 2014-03-19 14:07:12 PDT
All reviewed patches have been landed.  Closing bug.