Bug 164057

Created attachment 293010 [details] Patch

Comment on attachment 293010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293010&action=review > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:43 > + this.@initializeWith(configuration); I think we may need to call @initializeWith even if configuration is undefined. In that case, we might need to pass an empty dictionary. Or we could pass a dictionary with the default values if we want to handle defaults here. Probably simpler to just pass an empty object.

Also, some of the newly imported tests in LayoutTests/imported/w3c/web-platform-tests/webrtc might cover these changes. Might want to activate some of them

(In reply to comment #2) > Comment on attachment 293010 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=293010&action=review > > > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:43 > > + this.@initializeWith(configuration); > > I think we may need to call @initializeWith even if configuration is > undefined. > In that case, we might need to pass an empty dictionary. > Or we could pass a dictionary with the default values if we want to handle > defaults here. > Probably simpler to just pass an empty object. If we do not need to call @initializeWith, it might be best to rename this function to something like @setConfiguration.

(A side note.) A lot of tests have a dummy configuration used to construct RTCPeerConnection since the argument wasn't optional. We might want to clean that up.

(In reply to comment #5) > (A side note.) A lot of tests have a dummy configuration used to construct > RTCPeerConnection since the argument wasn't optional. We might want to clean > that up. OK, it might be safer then to make the call to @initializeWith mandatory for now and pass it a { } if parameter is undefined. Refactoring to make it optional sounds good as a follow-up.

Related test LayoutTests/imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html seems to expect that a null parameter should not throw.

Created attachment 297458 [details] Patch

Comment on attachment 297458 [details] Patch LGTM. View in context: https://bugs.webkit.org/attachment.cgi?id=297458&action=review > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:40 > if (!@isObject(configuration)) You could add "else if" here > LayoutTests/fast/mediastream/RTCPeerConnection.html:12 > + shouldNotThrow("new RTCPeerConnection();"); Maybe we can remove those two tests since they are not providing more than what the wpt tests provide already?

Thanks for the review and comments. > > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:40 > > if (!@isObject(configuration)) > > You could add "else if" here Ok I will upload a new patch. > > LayoutTests/fast/mediastream/RTCPeerConnection.html:12 > > + shouldNotThrow("new RTCPeerConnection();"); > > Maybe we can remove those two tests since they are not providing more than > what the wpt tests provide already? Ok.

(In reply to comment #7) > Related test > LayoutTests/imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/ > rtcpeerconnection-constructor.html seems to expect that a null parameter > should not throw. I think it should throw as per specification (https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection).

(In reply to comment #11) > (In reply to comment #7) > > Related test > > LayoutTests/imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/ > > rtcpeerconnection-constructor.html seems to expect that a null parameter > > should not throw. > I think it should throw as per specification > (https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection). Right, I think chromium and firefox accept null parameters currently, while webkit does not. We should try to converge on that issue.

Created attachment 297463 [details] Patch

Thanks for the review!

Comment on attachment 297463 [details] Patch Clearing flags on attachment: 297463 Committed r210017: <http://trac.webkit.org/changeset/210017>

All reviewed patches have been landed. Closing bug.