WebRTC: RTCPeerConnection constructor argument should be optional

[Nael Ouedraogo]

RTCPeerConnection constructor argument should be optional.

[Nael Ouedraogo]

Created attachment 293010 [details]
Patch

[youenn fablet]

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.

[youenn fablet]

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

[youenn fablet]

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

[Adam Bergkvist]

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

[youenn fablet]

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

[youenn fablet]

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

[Nael Ouedraogo]

Created attachment 297458 [details]
Patch

[youenn fablet]

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?

[Nael Ouedraogo]

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.

[Nael Ouedraogo]

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

[youenn fablet]

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

[Nael Ouedraogo]

Created attachment 297463 [details]
Patch

[Nael Ouedraogo]

Thanks for the review!

[WebKit Commit Bot]

Comment on attachment 297463 [details]
Patch

Clearing flags on attachment: 297463

Committed r210017: <http://trac.webkit.org/changeset/210017>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.