Bug 158832 – WebRTC: Replace RTCPeerConnection custom constructor with a JS built-in constructor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Adam Bergkvist]

This will make it easier to initialize JS fields and to verify the type of an RTCPeerConnection instance.

[Adam Bergkvist]

Created attachment 281643 [details]
Proposed patch

[Eric Carlson]

Comment on attachment 281643 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281643&action=review

> LayoutTests/fast/mediastream/RTCPeerConnection-expected.txt:7
> +PASS new webkitRTCPeerConnection(null); threw exception TypeError: Type error.
> +PASS new webkitRTCPeerConnection(undefined); threw exception TypeError: Type error.

I think it may be confusing to a developer that passing null or undefined results in an exception with the generic message 'Type error', but passing an empty string yields the much more informative message 'RTCPeerConnection argument must be a valid Dictionary'. Why the difference?

> LayoutTests/fast/mediastream/RTCPeerConnection-expected.txt:10
> +PASS new webkitRTCPeerConnection({}); threw exception TypeError: Type error.

Ditto.

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:84
> +

Nit: I am not sure this blank line aids readability.

[Adam Bergkvist]

Created attachment 281665 [details]
Updated patch

[Adam Bergkvist]

(In reply to comment #2)
> Comment on attachment 281643 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281643&action=review
> 
> > LayoutTests/fast/mediastream/RTCPeerConnection-expected.txt:7
> > +PASS new webkitRTCPeerConnection(null); threw exception TypeError: Type error.
> > +PASS new webkitRTCPeerConnection(undefined); threw exception TypeError: Type error.
> 
> I think it may be confusing to a developer that passing null or undefined
> results in an exception with the generic message 'Type error', but passing
> an empty string yields the much more informative message 'RTCPeerConnection
> argument must be a valid Dictionary'. Why the difference?

Let's not introduce such a behavior. I modified the JS constructor slightly to behave exactly like the old custom constructor; hence no changes to the test any more. The current behavior is not really spec compliant, but there's a bug to address that [1] (also noted in change log).

[1] http://webkit.org/b/158936

> > LayoutTests/fast/mediastream/RTCPeerConnection-expected.txt:10
> > +PASS new webkitRTCPeerConnection({}); threw exception TypeError: Type error.
> 
> Ditto.
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:84
> > +
> 
> Nit: I am not sure this blank line aids readability.

Removed.

[Adam Bergkvist]

This change depends on [1] which the build bot don't seem to have picked up yet.

https://bugs.webkit.org/show_bug.cgi?id=158834

[youenn fablet]

Comment on attachment 281665 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281665&action=review

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:83
> +    Document& document = downcast<Document>(*scriptExecutionContext());

It might be better to use CallWith=Document in the IDL to pass Document as parameter to the IDL

> Source/WebCore/Modules/mediastream/RTCPeerConnection.h:66
> +    void initializeWith(const Dictionary& rtcConfiguration, ExceptionCode&);

Is "rtcConfiguration" needed?

> Source/WebCore/Modules/mediastream/RTCPeerConnection.js:44
> +        this.@initializeWith(configuration);

I am not quite sure but can we find a better handling of errors than catching-and-rethrowing?
Would it be more readable to make initializeWith return a status code instead?

[Adam Bergkvist]

(In reply to comment #6)
> Comment on attachment 281665 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281665&action=review
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:83
> > +    Document& document = downcast<Document>(*scriptExecutionContext());
> 
> It might be better to use CallWith=Document in the IDL to pass Document as
> parameter to the IDL

Fixed.

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.h:66
> > +    void initializeWith(const Dictionary& rtcConfiguration, ExceptionCode&);
> 
> Is "rtcConfiguration" needed?

It contains all the settings passed to the constructor.

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:44
> > +        this.@initializeWith(configuration);
> 
> I am not quite sure but can we find a better handling of errors than
> catching-and-rethrowing?
> Would it be more readable to make initializeWith return a status code
> instead?

I agree that catching and re-throwing is not ideal and that we should aim for something nicer. My plan is to revamp the RTCPeerConnection constructor, and probably the related setConfiguration() method, entirely in [1] to make them spec compliant. We probably want to define the RTCConfiguration dictionary (argument to constructor and setConfiguration()) properly in IDL or do something in the JS built-ins to get nice error messages. So further tweaks to this code will most likely be throw away soon. Is it OK going forward with that plan and commit this to unlock the blocked bugs?

[1] http://webkit.org/b/158936

[Adam Bergkvist]

Created attachment 281714 [details]
Updated patch (for landing)

Waiting for OK from Youenn

[WebKit Commit Bot]

Attachment 281714 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/mediastream/RTCPeerConnection.h:66:  The parameter name "document" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Bergkvist]

Created attachment 281716 [details]
Updated patch (for landing)

Fixed style. Waiting for OK from Youenn

[Adam Bergkvist]

(In reply to comment #6)
> Comment on attachment 281665 [details]
> Updated patch
> 
> > Source/WebCore/Modules/mediastream/RTCPeerConnection.h:66
> > +    void initializeWith(const Dictionary& rtcConfiguration, ExceptionCode&);
> 
> Is "rtcConfiguration" needed?

Looking at this again I think I get what you mean. :) Fixed in last patch.

[youenn fablet]

Sounds ok to me.
I would maybe add a FIXME for the rethrowing just in case, but this is not mandatory.

[Adam Bergkvist]

(In reply to comment #12)
> Sounds ok to me.
> I would maybe add a FIXME for the rethrowing just in case, but this is not
> mandatory.

Thanks. I can add it since I need to update the patch anyhow.

[Adam Bergkvist]

Created attachment 281819 [details]
Updated patch (for landing)

[WebKit Commit Bot]

Comment on attachment 281819 [details]
Updated patch (for landing)

Clearing flags on attachment: 281819

Committed r202337: <http://trac.webkit.org/changeset/202337>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.