Bug 158832

Created attachment 281643 [details] Proposed patch

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.

Created attachment 281665 [details] Updated patch

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

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 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?

(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

Created attachment 281714 [details] Updated patch (for landing) Waiting for OK from Youenn

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.

Created attachment 281716 [details] Updated patch (for landing) Fixed style. Waiting for OK from Youenn

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

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

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

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

Comment on attachment 281819 [details] Updated patch (for landing) Clearing flags on attachment: 281819 Committed r202337: <http://trac.webkit.org/changeset/202337>

All reviewed patches have been landed. Closing bug.