Bug 130452

Created attachment 227182 [details] Patch

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

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

(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

Created attachment 227206 [details] Requested changes

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 on attachment 227206 [details] Requested changes Clearing flags on attachment: 227206 Committed r165915: <http://trac.webkit.org/changeset/165915>

All reviewed patches have been landed. Closing bug.