If invalid parameters are passed on RTCPeerConnection creation we must throw a TypeError exception. According to the spec it requires a Dictionary argument, the RTCConfiguration, which is mandatory.
Created attachment 216406 [details] Patch
(In reply to comment #1) > Created an attachment (id=216406) [details] > Patch Eric, could you please help me to add it to xcode's build files?
Comment on attachment 216406 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216406&action=review I will email you a patch to add the new file to the Xcode project. > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:36 > + CustomConstructor, > + CustomConstructor(Dictionary rtcIceServers, optional Dictionary mediaConstraints), I don't think you need both of these. > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:-36 > - ConstructorCallWith=ScriptExecutionContext, Why remove this when we still require the ScriptExecutionContext? > LayoutTests/fast/mediastream/RTCPeerConnection-expected.txt:19 > PASS new webkitRTCPeerConnection({iceServers:[{url:'turn:foo.com', credential:'x'}]}, null); did not throw exception. > PASS new webkitRTCPeerConnection({iceServers:[{url:'turn:foo.com', credential:'x'},{url:'stun:bar.com'}]}, null); did not throw exception. We should also check that "username" is allowed. > LayoutTests/fast/mediastream/RTCPeerConnection.html:13 > <script> > description("Tests the RTCPeerConnection constructor."); > > -shouldNotThrow("new webkitRTCPeerConnection(null);"); > -shouldNotThrow("new webkitRTCPeerConnection(null, null);"); > -shouldNotThrow("new webkitRTCPeerConnection(undefined);"); > -shouldNotThrow("new webkitRTCPeerConnection(undefined, undefined);"); > +shouldThrow("new webkitRTCPeerConnection(null);"); > +shouldThrow("new webkitRTCPeerConnection(null, null);"); > +shouldThrow("new webkitRTCPeerConnection(undefined);"); > +shouldThrow("new webkitRTCPeerConnection(undefined, undefined);"); Bonus points if you add some indentation to make this more readable.
(In reply to comment #3) > (From update of attachment 216406 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=216406&action=review > > I will email you a patch to add the new file to the Xcode project. Thanks > > > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:36 > > + CustomConstructor, > > + CustomConstructor(Dictionary rtcIceServers, optional Dictionary mediaConstraints), > > I don't think you need both of these. OK, removing the first one. > > > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:-36 > > - ConstructorCallWith=ScriptExecutionContext, > > Why remove this when we still require the ScriptExecutionContext? I was following the how other CustomConstructors do. And all CustomConstructors, do not have it. But I can put it back if there is no harm. > > > LayoutTests/fast/mediastream/RTCPeerConnection-expected.txt:19 > > PASS new webkitRTCPeerConnection({iceServers:[{url:'turn:foo.com', credential:'x'}]}, null); did not throw exception. > > PASS new webkitRTCPeerConnection({iceServers:[{url:'turn:foo.com', credential:'x'},{url:'stun:bar.com'}]}, null); did not throw exception. > > We should also check that "username" is allowed. True story. > > > LayoutTests/fast/mediastream/RTCPeerConnection.html:13 > > <script> > > description("Tests the RTCPeerConnection constructor."); > > > > -shouldNotThrow("new webkitRTCPeerConnection(null);"); > > -shouldNotThrow("new webkitRTCPeerConnection(null, null);"); > > -shouldNotThrow("new webkitRTCPeerConnection(undefined);"); > > -shouldNotThrow("new webkitRTCPeerConnection(undefined, undefined);"); > > +shouldThrow("new webkitRTCPeerConnection(null);"); > > +shouldThrow("new webkitRTCPeerConnection(null, null);"); > > +shouldThrow("new webkitRTCPeerConnection(undefined);"); > > +shouldThrow("new webkitRTCPeerConnection(undefined, undefined);"); > > Bonus points if you add some indentation to make this more readable. I think all indentation could be in a separated commit, so we won't get confused by some git blame, because there will be an explanation on the commit that it was just indenting.
Created attachment 216420 [details] Patch
Comment on attachment 216420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216420&action=review > Source/WebCore/ChangeLog:22 > + * GNUmakefile.list.am: > + * Modules/mediastream/RTCPeerConnection.idl: > + * UseJSC.cmake: > + * WebCore.vcxproj/WebCore.vcxproj: > + * WebCore.vcxproj/WebCore.vcxproj.filters: > + * bindings/js/JSRTCPeerConnectionCustom.cpp: Added. > + (WebCore::JSRTCPeerConnectionConstructor::constructJSRTCPeerConnection): You forgot to include the Xcode changes here. > LayoutTests/fast/mediastream/RTCPeerConnection-expected.txt:15 > +PASS new webkitRTCPeerConnection({}); threw exception TypeError: Error creating RTCPeerConnection. > +PASS new webkitRTCPeerConnection({}, ''); threw exception TypeError: Error creating RTCPeerConnection. > +PASS new webkitRTCPeerConnection({}, null); threw exception TypeError: Error creating RTCPeerConnection. It seems unfortunate to not have the same helpful exception error message here that we use when a non-object is passed for the Dictionary. "First argument of RTCPeerConnection must be a valid Dictionary containing a list of ice servers" is just as valid in this case. Could we use the same error message when "Dictionary mediaConstraints(exec, exec->argument(1))" returns a TypeError?
Comment on attachment 216420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216420&action=review > Source/WebCore/bindings/js/JSRTCPeerConnectionCustom.cpp:49 > + Dictionary mediaConstraints(exec, exec->argument(1)); > + if (exec->hadException()) > + return JSValue::encode(jsUndefined()); Oops, this needs "if (exec->argumentCount() > 1)"
Comment on attachment 216420 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=216420&action=review >> LayoutTests/fast/mediastream/RTCPeerConnection-expected.txt:15 >> +PASS new webkitRTCPeerConnection({}, null); threw exception TypeError: Error creating RTCPeerConnection. > > It seems unfortunate to not have the same helpful exception error message here that we use when a non-object is passed for the Dictionary. "First argument of RTCPeerConnection must be a valid Dictionary containing a list of ice servers" is just as valid in this case. > > Could we use the same error message when "Dictionary mediaConstraints(exec, exec->argument(1))" returns a TypeError? Or rather, when "RTCPeerConnection::create()" returns a TYPE_MISMATCH_ERR
Created attachment 216446 [details] Patch
Comment on attachment 216446 [details] Patch Clearing flags on attachment: 216446 Committed r158964: <http://trac.webkit.org/changeset/158964>
All reviewed patches have been landed. Closing bug.