Bug 151303

Created attachment 281386 [details] Proposed patch

Comment on attachment 281386 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=281386&action=review > LayoutTests/fast/mediastream/RTCPeerConnection-js-built-ins-check-this.html:9 > + description("Verify that the RTCPeerConnection JS built-in methods checks calling 'this'"); Nit: "checks" -> "check"

Would you be able to add 2 tests: - Changing a RTCPeerConnection object prototype, then calling one of RTCPeerConnection function on the object. - Setting an object to the RTCPeerConnection prototype and calling one of the methods. Ideally we should not rely on the object prototype itself.

(In reply to comment #3) > Would you be able to add 2 tests: > - Changing a RTCPeerConnection object prototype, then calling one of > RTCPeerConnection function on the object. > - Setting an object to the RTCPeerConnection prototype and calling one of > the methods. > > Ideally we should not rely on the object prototype itself. The current @isRTCPeerConnection() cannot deal with the cases you describe Youenn. My initial thinking is to add a JSBuiltinConstructor where I can initialize the @operations field and use that one as a probe. Right now the existence for @operations is checked before every use, so a JS built-in constructor would be useful anyhow.

(In reply to comment #4) > (In reply to comment #3) > > Would you be able to add 2 tests: > > - Changing a RTCPeerConnection object prototype, then calling one of > > RTCPeerConnection function on the object. > > - Setting an object to the RTCPeerConnection prototype and calling one of > > the methods. > > > > Ideally we should not rely on the object prototype itself. > > The current @isRTCPeerConnection() cannot deal with the cases you describe > Youenn. My initial thinking is to add a JSBuiltinConstructor where I can > initialize the @operations field and use that one as a probe. Right now the > existence for @operations is checked before every use, so a JS built-in > constructor would be useful anyhow. That would probably cover the cases. In the future we might want to make checks more uniform, for instance something like: if (!(object instanceof @RTCPeerConnection)) throw new @TypeError(...)

(In reply to comment #5) > In the future we might want to make checks more uniform, for instance > something like: > if (!(object instanceof @RTCPeerConnection)) > throw new @TypeError(...) That would be nice.

Created attachment 281448 [details] Updated patch

(In reply to comment #2) > Comment on attachment 281386 [details] > Proposed patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=281386&action=review > > > LayoutTests/fast/mediastream/RTCPeerConnection-js-built-ins-check-this.html:9 > > + description("Verify that the RTCPeerConnection JS built-in methods checks calling 'this'"); > > Nit: "checks" -> "check" Fixed

I would prefer to not add a JS built-in constructor in this change so I've added expected failures that can be converted to passing tests when [1] is resolved. Which in turns depend on [2]. [1] https://bugs.webkit.org/show_bug.cgi?id=158831 [2] https://bugs.webkit.org/show_bug.cgi?id=158832

Comment on attachment 281448 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=281448&action=review > LayoutTests/fast/mediastream/RTCPeerConnection-js-built-ins-check-this.html:52 > + window.successfullyParsed = true; With testharness.js, these 10 lines would no longer be needed. Promise-based test handling by testharness.js is really nice. I would tend to use those here. You can have a look in fetch or streams tests. > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:38 > + return @Promise.@reject(new @TypeError("Function should be called on an RTCPeerConnection")); This message is consistent with other built-ins which is good. But it is not with binding generated messages (something like "Can only call RTCPeerConnection.createOffer on instances of RTCPeerConnection"). Maybe we should add something like @createBadThisFunctionError taking interface and function name as input to have the same messages in both places. Or maybe making consistent is not all that important?

(In reply to comment #10) > Comment on attachment 281448 [details] > Updated patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=281448&action=review > > > LayoutTests/fast/mediastream/RTCPeerConnection-js-built-ins-check-this.html:52 > > + window.successfullyParsed = true; > > With testharness.js, these 10 lines would no longer be needed. > > Promise-based test handling by testharness.js is really nice. > I would tend to use those here. You can have a look in fetch or streams > tests. I might do that and see if we can contribute tests to the W3C test suite. > > Source/WebCore/Modules/mediastream/RTCPeerConnection.js:38 > > + return @Promise.@reject(new @TypeError("Function should be called on an RTCPeerConnection")); > > This message is consistent with other built-ins which is good. > But it is not with binding generated messages (something like "Can only call > RTCPeerConnection.createOffer on instances of RTCPeerConnection"). > > Maybe we should add something like @createBadThisFunctionError taking > interface and function name as input to have the same messages in both > places. > > Or maybe making consistent is not all that important? I think that's a good idea.

Comment on attachment 281448 [details] Updated patch Thanks for reviewing Youenn and Eric!

Comment on attachment 281448 [details] Updated patch Clearing flags on attachment: 281448 Committed r202130: <http://trac.webkit.org/changeset/202130>

All reviewed patches have been landed. Closing bug.

Adam, I am one of the maintainer of the w3c webrtc tests, so you can send the test my way for commit, I have access.

(In reply to comment #15) > Adam, I am one of the maintainer of the w3c webrtc tests, so you can send > the test my way for commit, I have access. Got it!