Bug 170146 – Fix addIceCandidate after r214441

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

We should throw if no parameter is passed.

[youenn fablet]

Created attachment 305526 [details]
Patch

[Chris Dumez]

Comment on attachment 305526 [details]
Patch

r=me if the bots are happy.

[youenn fablet]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 305526 [details]
> Patch
> 
> r=me if the bots are happy.

Right, I updated the test expectation by hand...

[Chris Dumez]

(In reply to youenn fablet from comment #3)
> (In reply to Chris Dumez from comment #2)
> > Comment on attachment 305526 [details]
> > Patch
> > 
> > r=me if the bots are happy.
> 
> Right, I updated the test expectation by hand...

lol

[Build Bot]

Comment on attachment 305526 [details]
Patch

Attachment 305526 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3420597

New failing tests:
imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl.html

[Build Bot]

Created attachment 305536 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

--- /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/retries/imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl-expected.txt
+++ /Volumes/Data/EWS/WebKit/WebKitBuild/Release/layout-test-results/retries/imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl-actual.txt
@@ -23,7 +23,7 @@
 PASS RTCPeerConnection interface: attribute remoteDescription 
 PASS RTCPeerConnection interface: attribute currentRemoteDescription 
 PASS RTCPeerConnection interface: attribute pendingRemoteDescription 
-PASS RTCPeerConnection interface: operation addIceCandidate([object Object],[object Object])
+PASS RTCPeerConnection interface: operation addIceCandidate([object Object],[object Object]) 
 PASS RTCPeerConnection interface: attribute signalingState 
 PASS RTCPeerConnection interface: attribute iceGatheringState 
 PASS RTCPeerConnection interface: attribute iceConnectionState 
@@ -44,7 +44,9 @@
             fn.apply(obj, args);
         }" did not throw
 FAIL RTCPeerConnection interface: operation setRemoteDescription(RTCSessionDescription,VoidFunction,RTCPeerConnectionErrorCallback) assert_equals: property has wrong .length expected 1 but got 0
-PASS RTCPeerConnection interface: operation addIceCandidate(RTCIceCandidate,VoidFunction,RTCPeerConnectionErrorCallback)
+FAIL RTCPeerConnection interface: operation addIceCandidate(RTCIceCandidate,VoidFunction,RTCPeerConnectionErrorCallback) assert_throws: calling operation with this = null didn't throw TypeError function "function () {
+            fn.apply(obj, args);
+        }" did not throw
 FAIL RTCPeerConnection interface: operation getStats(MediaStreamTrack,RTCStatsCallback,RTCPeerConnectionErrorCallback) assert_throws: calling operation with this = null didn't throw TypeError function "function () {
             fn.apply(obj, args);
         }" did not throw
@@ -79,7 +81,7 @@
 PASS RTCPeerConnection interface: pc must inherit property "currentRemoteDescription" with the proper type (8) 
 PASS RTCPeerConnection interface: pc must inherit property "pendingRemoteDescription" with the proper type (9) 
 PASS RTCPeerConnection interface: pc must inherit property "addIceCandidate" with the proper type (10) 
-PASS RTCPeerConnection interface: calling addIceCandidate([object Object],[object Object]) on pc with too few arguments must throw TypeError
+PASS RTCPeerConnection interface: calling addIceCandidate([object Object],[object Object]) on pc with too few arguments must throw TypeError 
 FAIL RTCPeerConnection interface: pc must inherit property "signalingState" with the proper type (11) Unrecognized type RTCSignalingState
 FAIL RTCPeerConnection interface: pc must inherit property "iceGatheringState" with the proper type (12) Unrecognized type RTCIceGatheringState
 FAIL RTCPeerConnection interface: pc must inherit property "iceConnectionState" with the proper type (13) Unrecognized type RTCIceConnectionState
@@ -106,7 +108,7 @@
             fn.apply(obj, args);
         }" did not throw
 PASS RTCPeerConnection interface: pc must inherit property "addIceCandidate" with the proper type (27) 
-PASS RTCPeerConnection interface: calling addIceCandidate(RTCIceCandidate,VoidFunction,RTCPeerConnectionErrorCallback) on pc with too few arguments must throw TypeError
+FAIL RTCPeerConnection interface: calling addIceCandidate(RTCIceCandidate,VoidFunction,RTCPeerConnectionErrorCallback) on pc with too few arguments must throw TypeError assert_throws: Called with 0 arguments function "function () {
             fn.apply(obj, args);
         }" did not throw
 PASS RTCPeerConnection interface: pc must inherit property "getStats" with the proper type (28)

[youenn fablet]

Created attachment 305538 [details]
Patch

[youenn fablet]

(In reply to youenn fablet from comment #8)
> Created attachment 305538 [details]
> Patch

This one should have a better expectation...

[youenn fablet]

Created attachment 305626 [details]
Patch for landing

[youenn fablet]

(In reply to youenn fablet from comment #10)
> Created attachment 305626 [details]
> Patch for landing

Retrying EWS since it was submitted at a bad time apparently.

[Carlos Alberto Lopez Perez]

r214441 <https://trac.webkit.org/r214441> Also caused this failure on the WebKitGTK port (OpenWebRTC) on the test fast/mediastream/RTCPeerConnection-overloaded-operations-params.html


--- /home/clopez/webkit/webkit/layout-test-results/fast/mediastream/RTCPeerConnection-overloaded-operations-params-expected.txt
+++ /home/clopez/webkit/webkit/layout-test-results/fast/mediastream/RTCPeerConnection-overloaded-operations-params-actual.txt
@@ -80,10 +80,10 @@
 PASS promise pc.addIceCandidate(candidate) did not reject with TypeError.
 PASS promise pc.addIceCandidate(candidate, emptyFunc, emptyFunc) did not reject with TypeError.
 *** candidate is not optional
-PASS promise pc.addIceCandidate() rejected with TypeError: Not enough arguments
+FAIL promise pc.addIceCandidate() fulfilled unexpectedly.
 *** candidate is not nullable
-PASS promise pc.addIceCandidate(null) rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate
-PASS promise pc.addIceCandidate(null, emptyFunc, emptyFunc) rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate
+FAIL promise pc.addIceCandidate(null) fulfilled unexpectedly.
+FAIL promise pc.addIceCandidate(null, emptyFunc, emptyFunc) fulfilled unexpectedly.
 PASS promise pc.addIceCandidate(1) rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate
 *** Bad input as candidate
 PASS promise pc.addIceCandidate('foo') rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate



Note that the test fast/mediastream/RTCPeerConnection-overloaded-operations-params.html is Skipped on all ports but the WebKitGTK+ one.

[Eric Carlson]

Comment on attachment 305626 [details]
Patch for landing

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

> LayoutTests/imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl-expected.txt:49
> +FAIL RTCPeerConnection interface: operation addIceCandidate(RTCIceCandidate,VoidFunction,RTCPeerConnectionErrorCallback) assert_throws: calling operation with this = null didn't throw TypeError function "function () {
> +            fn.apply(obj, args);
> +        }" did not throw

Does the test need to be updated or is this correct?

[Carlos Alberto Lopez Perez]

The patch that is about to land reduces the test diff to this:


$ cat layout-test-results/fast/mediastream/RTCPeerConnection-overloaded-operations-params-diff.txt 
--- /home/clopez/webkit/webkit/layout-test-results/fast/mediastream/RTCPeerConnection-overloaded-operations-params-expected.txt
+++ /home/clopez/webkit/webkit/layout-test-results/fast/mediastream/RTCPeerConnection-overloaded-operations-params-actual.txt
@@ -82,8 +82,8 @@
 *** candidate is not optional
 PASS promise pc.addIceCandidate() rejected with TypeError: Not enough arguments
 *** candidate is not nullable
-PASS promise pc.addIceCandidate(null) rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate
-PASS promise pc.addIceCandidate(null, emptyFunc, emptyFunc) rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate
+FAIL promise pc.addIceCandidate(null) fulfilled unexpectedly.
+FAIL promise pc.addIceCandidate(null, emptyFunc, emptyFunc) fulfilled unexpectedly.
 PASS promise pc.addIceCandidate(1) rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate
 *** Bad input as candidate
 PASS promise pc.addIceCandidate('foo') rejected with TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of RTCIceCandidate

[Chris Dumez]

Comment on attachment 305626 [details]
Patch for landing

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

>> LayoutTests/imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl-expected.txt:49
>> +        }" did not throw
> 
> Does the test need to be updated or is this correct?

I think the test is wrong here. The spec says:
 Promise<void> addIceCandidate((RTCIceCandidateInit or RTCIceCandidate) candidate,
                                  VoidFunction successCallback,
                                  RTCPeerConnectionErrorCallback failureCallback);

But the test's IDL returns a void, not a Promise<void>.

[youenn fablet]

(In reply to Eric Carlson from comment #13)
> Comment on attachment 305626 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305626&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-idl-expected.txt:49
> > +FAIL RTCPeerConnection interface: operation addIceCandidate(RTCIceCandidate,VoidFunction,RTCPeerConnectionErrorCallback) assert_throws: calling operation with this = null didn't throw TypeError function "function () {
> > +            fn.apply(obj, args);
> > +        }" did not throw
> 
> Does the test need to be updated or is this correct?

As discussed with Chris, it needs to be updated since it is returning a Promise.
In the end, I hope we will be able to remove support for these legacies

[youenn fablet]

> -PASS promise pc.addIceCandidate(null) rejected with TypeError: Argument 1
> ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of
> RTCIceCandidate
> -PASS promise pc.addIceCandidate(null, emptyFunc, emptyFunc) rejected with
> TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate
> must be an instance of RTCIceCandidate
> +FAIL promise pc.addIceCandidate(null) fulfilled unexpectedly.
> +FAIL promise pc.addIceCandidate(null, emptyFunc, emptyFunc) fulfilled
> unexpectedly.

We might want to check with other browsers, but I think it is ok to do addIceCandidate(null). It will mean that there is no more ice candidate, just as would be done with addIceCandidate({ })

[Carlos Alberto Lopez Perez]

(In reply to youenn fablet from comment #17)
> > -PASS promise pc.addIceCandidate(null) rejected with TypeError: Argument 1
> > ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of
> > RTCIceCandidate
> > -PASS promise pc.addIceCandidate(null, emptyFunc, emptyFunc) rejected with
> > TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate
> > must be an instance of RTCIceCandidate
> > +FAIL promise pc.addIceCandidate(null) fulfilled unexpectedly.
> > +FAIL promise pc.addIceCandidate(null, emptyFunc, emptyFunc) fulfilled
> > unexpectedly.
> 
> We might want to check with other browsers, but I think it is ok to do
> addIceCandidate(null). It will mean that there is no more ice candidate,
> just as would be done with addIceCandidate({ })

That seems right: https://github.com/w3c/webrtc-pc/issues/569

So I guess this test just needs to be rebaselined. I will do that.

[WebKit Commit Bot]

Comment on attachment 305626 [details]
Patch for landing

Clearing flags on attachment: 305626

Committed r214490: <http://trac.webkit.org/changeset/214490>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #18)
> (In reply to youenn fablet from comment #17)
> > > -PASS promise pc.addIceCandidate(null) rejected with TypeError: Argument 1
> > > ('candidate') to RTCPeerConnection.addIceCandidate must be an instance of
> > > RTCIceCandidate
> > > -PASS promise pc.addIceCandidate(null, emptyFunc, emptyFunc) rejected with
> > > TypeError: Argument 1 ('candidate') to RTCPeerConnection.addIceCandidate
> > > must be an instance of RTCIceCandidate
> > > +FAIL promise pc.addIceCandidate(null) fulfilled unexpectedly.
> > > +FAIL promise pc.addIceCandidate(null, emptyFunc, emptyFunc) fulfilled
> > > unexpectedly.
> > 
> > We might want to check with other browsers, but I think it is ok to do
> > addIceCandidate(null). It will mean that there is no more ice candidate,
> > just as would be done with addIceCandidate({ })
> 
> That seems right: https://github.com/w3c/webrtc-pc/issues/569
> 
> So I guess this test just needs to be rebaselined. I will do that.

Updated on bug 170223