Bug 158832 - WebRTC: Replace RTCPeerConnection custom constructor with a JS built-in constructor
Summary: WebRTC: Replace RTCPeerConnection custom constructor with a JS built-in const...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 158834 159145
Blocks: 143211 158936 158831 158940
  Show dependency treegraph
 
Reported: 2016-06-16 01:38 PDT by Adam Bergkvist
Modified: 2016-06-27 07:27 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch (22.90 KB, patch)
2016-06-20 04:54 PDT, Adam Bergkvist
no flags Details | Formatted Diff | Diff
Updated patch (15.98 KB, patch)
2016-06-20 12:38 PDT, Adam Bergkvist
eric.carlson: review+
Details | Formatted Diff | Diff
Updated patch (for landing) (15.98 KB, patch)
2016-06-20 22:38 PDT, Adam Bergkvist
no flags Details | Formatted Diff | Diff
Updated patch (for landing) (15.96 KB, patch)
2016-06-20 22:50 PDT, Adam Bergkvist
no flags Details | Formatted Diff | Diff
Updated patch (for landing) (16.07 KB, patch)
2016-06-21 22:57 PDT, Adam Bergkvist
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Bergkvist 2016-06-16 01:38:25 PDT
This will make it easier to initialize JS fields and to verify the type of an RTCPeerConnection instance.
Comment 1 Adam Bergkvist 2016-06-20 04:54:30 PDT
Created attachment 281643 [details]
Proposed patch
Comment 2 Eric Carlson 2016-06-20 07:33:09 PDT
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.
Comment 3 Adam Bergkvist 2016-06-20 12:38:50 PDT
Created attachment 281665 [details]
Updated patch
Comment 4 Adam Bergkvist 2016-06-20 12:44:36 PDT
(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.
Comment 5 Adam Bergkvist 2016-06-20 12:49:50 PDT
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
Comment 6 youenn fablet 2016-06-20 14:47:32 PDT
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?
Comment 7 Adam Bergkvist 2016-06-20 22:29:16 PDT
(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
Comment 8 Adam Bergkvist 2016-06-20 22:38:29 PDT
Created attachment 281714 [details]
Updated patch (for landing)

Waiting for OK from Youenn
Comment 9 WebKit Commit Bot 2016-06-20 22:40:31 PDT
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.
Comment 10 Adam Bergkvist 2016-06-20 22:50:06 PDT
Created attachment 281716 [details]
Updated patch (for landing)

Fixed style. Waiting for OK from Youenn
Comment 11 Adam Bergkvist 2016-06-20 22:56:54 PDT
(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.
Comment 12 youenn fablet 2016-06-21 14:41:01 PDT
Sounds ok to me.
I would maybe add a FIXME for the rethrowing just in case, but this is not mandatory.
Comment 13 Adam Bergkvist 2016-06-21 22:40:44 PDT
(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.
Comment 14 Adam Bergkvist 2016-06-21 22:57:47 PDT
Created attachment 281819 [details]
Updated patch (for landing)
Comment 15 WebKit Commit Bot 2016-06-22 11:12:57 PDT
Comment on attachment 281819 [details]
Updated patch (for landing)

Clearing flags on attachment: 281819

Committed r202337: <http://trac.webkit.org/changeset/202337>
Comment 16 WebKit Commit Bot 2016-06-22 11:13:01 PDT
All reviewed patches have been landed.  Closing bug.