Add promise-based addIceCandidate, setLocalDescription and setRemoteDescription to RTCPeerConnection

third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp

Index: third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
diff --git a/third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp b/third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
index 854a5babc9d35c6b9a4521f59b7bde8c605c4088..ee1cf3a5ee9dd1ba62d73034b2ba321909525976 100644
--- a/third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
+++ b/third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
@@ -35,10 +35,13 @@
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/Nullable.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"

[yhirano, 2016/02/10 18:13:22]

+#include "bindings/core/v8/ScriptState.h"

[Guido Urdaneta, 2016/02/10 20:21:28]

Done.

+#include "bindings/core/v8/V8ThrowException.h"
+#include "bindings/modules/v8/UnionTypesModules.h"
 #include "bindings/modules/v8/V8RTCCertificate.h"
 #include "core/dom/Document.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/dom/ExecutionContext.h"
+#include "core/dom/Microtask.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/UseCounter.h"
 #include "core/html/VoidCallback.h"
@@ -54,10 +57,13 @@
 #include "modules/mediastream/RTCIceCandidateEvent.h"
 #include "modules/mediastream/RTCSessionDescription.h"
 #include "modules/mediastream/RTCSessionDescriptionCallback.h"
+#include "modules/mediastream/RTCSessionDescriptionInit.h"
 #include "modules/mediastream/RTCSessionDescriptionRequestImpl.h"
 #include "modules/mediastream/RTCStatsCallback.h"
 #include "modules/mediastream/RTCStatsRequestImpl.h"
 #include "modules/mediastream/RTCVoidRequestImpl.h"
+#include "modules/mediastream/RTCVoidRequestPromiseImpl.h"
+#include "platform/Timer.h"
 #include "platform/mediastream/RTCConfiguration.h"
 #include "platform/mediastream/RTCOfferOptions.h"
 #include "public/platform/Platform.h"
@@ -81,16 +87,60 @@ namespace blink {
 
 namespace {
 
-static bool throwExceptionIfSignalingStateClosed(RTCPeerConnection::SignalingState state, ExceptionState& exceptionState)
+const char kSignalingStateClosedMsg[] = "The RTCPeerConnection's signalingState is 'closed'.";
+const char kDefaultErrorName[] = "AbortError";
+const char kSessionDescriptionErrorName[] = "InvalidSessionDescription";
+
+bool throwExceptionIfSignalingStateClosed(RTCPeerConnection::SignalingState state, ExceptionState& exceptionState)
+{
+    if (state == RTCPeerConnection::SignalingStateClosed) {
+        exceptionState.throwDOMException(InvalidStateError, kSignalingStateClosedMsg);
+        return true;
+    }
+
+    return false;
+}
+
+
+void asyncCallErrorCallback(RTCErrorCallback* errorCallback, const String& errorMsg)
+{
+    Microtask::enqueueMicrotask(WTF::bind(&RTCErrorCallback::handleEvent, errorCallback, errorMsg));
+}
+
+bool callErrorCallbackIfSignalingStateClosed(RTCPeerConnection::SignalingState state, RTCErrorCallback* errorCallback)
 {
     if (state == RTCPeerConnection::SignalingStateClosed) {
-        exceptionState.throwDOMException(InvalidStateError, "The RTCPeerConnection's signalingState is 'closed'.");
+        if (errorCallback)
+            asyncCallErrorCallback(errorCallback, kSignalingStateClosedMsg);
+
         return true;
     }
 
     return false;
 }
 
+bool isIceCandidateMissingSdp(const RTCIceCandidateInitOrRTCIceCandidate& candidate)
+{
+    if (candidate.isRTCIceCandidateInit()) {
+        const RTCIceCandidateInit& iceCandidateInit = candidate.getAsRTCIceCandidateInit();
+        return !iceCandidateInit.hasSdpMid() && !iceCandidateInit.hasSdpMLineIndex();
+    }
+
+    ASSERT(candidate.isRTCIceCandidate());
+    return false;
+}
+
+WebRTCICECandidate convertToWebRTCIceCandidate(const RTCIceCandidateInitOrRTCIceCandidate& candidate)
+{
+    if (candidate.isRTCIceCandidateInit()) {
+        const RTCIceCandidateInit& iceCandidateInit = candidate.getAsRTCIceCandidateInit();
+        return WebRTCICECandidate(iceCandidateInit.candidate(), iceCandidateInit.sdpMid(), iceCandidateInit.sdpMLineIndex());
+    }
+
+    ASSERT(candidate.isRTCIceCandidate());
+    return candidate.getAsRTCIceCandidate()->webCandidate();
+}
+
 // Helper class for RTCPeerConnection::generateCertificate.
 class WebRTCCertificateObserver : public WebCallbacks<WebRTCCertificate*, void> {
 public:
@@ -460,7 +510,19 @@ void RTCPeerConnection::createAnswer(ExecutionContext* context, RTCSessionDescri
     m_peerHandler->createAnswer(request, constraints);
 }
 
-void RTCPeerConnection::setLocalDescription(ExecutionContext* context, RTCSessionDescription* sessionDescription, VoidCallback* successCallback, RTCErrorCallback* errorCallback, ExceptionState& exceptionState)
+ScriptPromise RTCPeerConnection::setLocalDescription(ScriptState* scriptState, const RTCSessionDescriptionInit& sessionDescriptionInit)
+{
+    if (m_signalingState == SignalingStateClosed)
+        return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, kSignalingStateClosedMsg));
+
+    ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
+    ScriptPromise promise = resolver->promise();
+    RTCVoidRequest* request = RTCVoidRequestPromiseImpl::create(this, resolver, kSessionDescriptionErrorName);
+    m_peerHandler->setLocalDescription(request, WebRTCSessionDescription(sessionDescriptionInit.type(), sessionDescriptionInit.sdp()));
+    return promise;
+}
+
+ScriptPromise RTCPeerConnection::setLocalDescription(ExecutionContext* context, RTCSessionDescription* sessionDescription, VoidCallback* successCallback, RTCErrorCallback* errorCallback)

[yhirano, 2016/02/10 18:13:22]

How about implement this function using the above one?

The implementation would be essentially:

return setLocalDescription(scriptState, init).then(successCallback,
errorCallback);

[Guido Urdaneta, 2016/02/10 20:21:28]

Actually, that's what the spec says. However, our legacy functions are from an
older version of the spec and have some slight differences.
For example, the new versions return DOMExceptions in case of error, but legacy
error callbacks expect a string. Input can also be slightly different
(RTCSessionDescription vs RTCSessionDescriptionInit). In addition, for legacy
setLocalDescription() and setRemoteDescription() we have a bunch of counters we
want to keep.
Once the legacy versions become less used, our plan is to implement them in
terms of the new versions.

[yhirano, 2016/02/10 22:29:19]

I see.
But I don't like to return an empty ScriptPromise - Can you return a valid
promise? I think returning ScriptPromise::cast(undefined) is OK here because the
reason you returns a promise is that all or none of overloaded functions must
return a promise in IDL. Adding a comment in the IDL for the intention would be
good, too.

Regarding the legacy interface, there are three functions,

1. Promise-based interface implemented by this CL
2. legacy new interface taking RTCSessionDescriptionInit
3. legacy old interface taking RTCSessionDescription

right?

I'm not sure what "implement them in terms of the new versions" means. Does that
mean you will remove 3 and add 2?

[Guido Urdaneta, 2016/02/10 23:53:14]

Done.
Done
I meant that according to the spec there should be only 1 and 2, and 2 should be
implemented as you proposed in your previous comment (return
setLocalDescription(scriptState, init).then(successCallback,
errorCallback);), and our goal is to eventually match the spec.

For now, however, we plan to have 1 and 3. The strategy to remove 3 and add 2
has not been fully decided yet beyond the TODOs in the IDL file. However, but we
have to keep supporting 3 until 1 is widely adopted. Note that there are 5
methods and each legacy version departs from the latest spec in different ways.

 {
     if (successCallback && errorCallback) {
         UseCounter::count(context, UseCounter::RTCPeerConnectionSetLocalDescriptionLegacyCompliant);
@@ -471,13 +533,14 @@ void RTCPeerConnection::setLocalDescription(ExecutionContext* context, RTCSessio
             UseCounter::count(context, UseCounter::RTCPeerConnectionSetLocalDescriptionLegacyNoFailureCallback);
     }
 
-    if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
-        return;
+    if (callErrorCallbackIfSignalingStateClosed(m_signalingState, errorCallback))
+        return ScriptPromise();
 
     ASSERT(sessionDescription);
 
     RTCVoidRequest* request = RTCVoidRequestImpl::create(executionContext(), this, successCallback, errorCallback);
     m_peerHandler->setLocalDescription(request, sessionDescription->webSessionDescription());
+    return ScriptPromise();
 }
 
 RTCSessionDescription* RTCPeerConnection::localDescription()
@@ -489,7 +552,19 @@ RTCSessionDescription* RTCPeerConnection::localDescription()
     return RTCSessionDescription::create(webSessionDescription);
 }
 
-void RTCPeerConnection::setRemoteDescription(ExecutionContext* context, RTCSessionDescription* sessionDescription, VoidCallback* successCallback, RTCErrorCallback* errorCallback, ExceptionState& exceptionState)
+ScriptPromise RTCPeerConnection::setRemoteDescription(ScriptState* scriptState, const RTCSessionDescriptionInit& sessionDescriptionInit)
+{
+    if (m_signalingState == SignalingStateClosed)
+        return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, kSignalingStateClosedMsg));
+
+    ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
+    ScriptPromise promise = resolver->promise();
+    RTCVoidRequest* request = RTCVoidRequestPromiseImpl::create(this, resolver, kSessionDescriptionErrorName);
+    m_peerHandler->setRemoteDescription(request, WebRTCSessionDescription(sessionDescriptionInit.type(), sessionDescriptionInit.sdp()));
+    return promise;
+}
+
+ScriptPromise RTCPeerConnection::setRemoteDescription(ExecutionContext* context, RTCSessionDescription* sessionDescription, VoidCallback* successCallback, RTCErrorCallback* errorCallback)

[yhirano, 2016/02/10 18:13:22]

ditto

 {
     if (successCallback && errorCallback) {
         UseCounter::count(context, UseCounter::RTCPeerConnectionSetRemoteDescriptionLegacyCompliant);
@@ -500,13 +575,14 @@ void RTCPeerConnection::setRemoteDescription(ExecutionContext* context, RTCSessi
             UseCounter::count(context, UseCounter::RTCPeerConnectionSetRemoteDescriptionLegacyNoFailureCallback);
     }
 
-    if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
-        return;
+    if (callErrorCallbackIfSignalingStateClosed(m_signalingState, errorCallback))
+        return ScriptPromise();
 
     ASSERT(sessionDescription);
 
     RTCVoidRequest* request = RTCVoidRequestImpl::create(executionContext(), this, successCallback, errorCallback);
     m_peerHandler->setRemoteDescription(request, sessionDescription->webSessionDescription());
+    return ScriptPromise();
 }
 
 RTCSessionDescription* RTCPeerConnection::remoteDescription()
@@ -608,33 +684,41 @@ ScriptPromise RTCPeerConnection::generateCertificate(ScriptState* scriptState, c
     return promise;
 }
 
-void RTCPeerConnection::addIceCandidate(RTCIceCandidate* iceCandidate, ExceptionState& exceptionState)
+ScriptPromise RTCPeerConnection::addIceCandidate(ScriptState* scriptState, const RTCIceCandidateInitOrRTCIceCandidate& candidate)
 {
-    if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
-        return;
+    if (m_signalingState == SignalingStateClosed)
+        return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, kSignalingStateClosedMsg));
 
-    ASSERT(iceCandidate);
+    if (isIceCandidateMissingSdp(candidate))
+        return ScriptPromise::reject(scriptState, V8ThrowException::createTypeError(scriptState->isolate(), "Candidate missing values for both sdpMid and sdpMLineIndex"));
 
-    bool valid = m_peerHandler->addICECandidate(iceCandidate->webCandidate());
-    if (!valid)
-        exceptionState.throwDOMException(SyntaxError, "The ICE candidate could not be added.");
+    ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
+    ScriptPromise promise = resolver->promise();
+    RTCVoidRequest* request = RTCVoidRequestPromiseImpl::create(this, resolver, kDefaultErrorName);
+    WebRTCICECandidate webCandidate = convertToWebRTCIceCandidate(candidate);
+    bool implemented = m_peerHandler->addICECandidate(request, webCandidate);
+    // TODO(guidou): replace NotSupportedError when error handling in the spec is finalized. crbug.com/585621
+    if (!implemented)
+        resolver->reject(DOMException::create(NotSupportedError, "This method is not yet implemented."));
+
+    return promise;
 }
 
-void RTCPeerConnection::addIceCandidate(RTCIceCandidate* iceCandidate, VoidCallback* successCallback, RTCErrorCallback* errorCallback, ExceptionState& exceptionState)
+ScriptPromise RTCPeerConnection::addIceCandidate(RTCIceCandidate* iceCandidate, VoidCallback* successCallback, RTCErrorCallback* errorCallback)

[yhirano, 2016/02/10 18:13:22]

ditto

 {
-    if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
-        return;
-
     ASSERT(iceCandidate);
     ASSERT(successCallback);
     ASSERT(errorCallback);
 
-    RTCVoidRequest* request = RTCVoidRequestImpl::create(executionContext(), this, successCallback, errorCallback);
+    if (callErrorCallbackIfSignalingStateClosed(m_signalingState, errorCallback))
+        return ScriptPromise();
 
+    RTCVoidRequest* request = RTCVoidRequestImpl::create(executionContext(), this, successCallback, errorCallback);
     bool implemented = m_peerHandler->addICECandidate(request, iceCandidate->webCandidate());
-    if (!implemented) {
-        exceptionState.throwDOMException(NotSupportedError, "This method is not yet implemented.");
-    }
+    if (!implemented)
+        asyncCallErrorCallback(errorCallback, "This method is not yet implemented.");
+
+    return ScriptPromise();
 }
 
 String RTCPeerConnection::signalingState() const