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..7138d963066a334450fbaf40bd19007bb673b5ef 100644
--- a/third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
+++ b/third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
@@ -35,6 +35,8 @@
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/Nullable.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
+#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"
@@ -54,10 +56,12 @@
 #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/mediastream/RTCConfiguration.h"
 #include "platform/mediastream/RTCOfferOptions.h"
 #include "public/platform/Platform.h"
@@ -81,16 +85,54 @@ namespace blink {
 
 namespace {
 
+const char kSignalingStateClosedMsg[] = "The RTCPeerConnection's signalingState is 'closed'.";
+const char kDefaultErrorName[] = "InternalError";

[philipj_slow, 2016/02/09 12:02:03]

I see "Ask the DOM team to extend their list with the following errors" issue in
the spec, can you make sure that an issue is filed on
http://heycam.github.io/webidl/#idl-DOMException-error-names ?

It seems pretty likely to me that you'll be asked to use an existing generic
error name instead of InternalError, at least.

For things that you expect to be added to DOMException, instead add them to
ExceptionCode and then use in the same way as InvalidStateError and friends.

The spec also talks about DOMError, which shouldn't be used for anything new,
but fortunately it looks like it isn't actually used,
MediaErrorState::raiseException instead throws a DOMExceptions.

Are these error conditions new, or why don't they appear anywhere in the
existing code?

[Guido Urdaneta, 2016/02/09 16:51:01]

The spec says to use DOMError with error name TBD. There is a bug filed about
the TBD part. I used InternalError just because it is a value used elsewhere in
the spec. What value would you suggest?

[philipj_slow, 2016/02/09 18:59:17]

Is there a WebIDL issue filed for adding any of the new names? I can't see any
in https://github.com/heycam/webidl/issues, can you do that? Or poke someone?

I think using DOMException with the best matching of the existing types would
make sense. What that is depends on what usually causes InternalError. I could
imagine NotSupportedError, InvalidStateError, SyntaxError, NetworkError,
AbortError, TimeoutError, ConstraintError or perhaps OperationError.

[Guido Urdaneta, 2016/02/10 07:05:13]

Done. Using DOMException("AbortError") until the webrtc spec defines the error
codes to use.

+const char kSessionDescriptionErrorName[] = "InvalidSessionDescription";
+
 static bool throwExceptionIfSignalingStateClosed(RTCPeerConnection::SignalingState state, ExceptionState& exceptionState)
 {
     if (state == RTCPeerConnection::SignalingStateClosed) {
-        exceptionState.throwDOMException(InvalidStateError, "The RTCPeerConnection's signalingState is 'closed'.");
+        exceptionState.throwDOMException(InvalidStateError, kSignalingStateClosedMsg);
         return true;
     }
 
     return false;
 }
 
+static bool callErrorCallbackIfSignalingStateClosed(RTCPeerConnection::SignalingState state, RTCErrorCallback* errorCallback)

[philipj_slow, 2016/02/09 12:02:03]

I think you don't need static inside this anonymous namespace. If not, drop it
above as well.

[Guido Urdaneta, 2016/02/09 16:51:01]

Done.

+{
+    if (state == RTCPeerConnection::SignalingStateClosed) {
+        if (errorCallback)
+            errorCallback->handleEvent(kSignalingStateClosedMsg);
+        return true;
+    }
+
+    return false;
+}
+
+

[philipj_slow, 2016/02/09 12:02:03]

extra newline

[Guido Urdaneta, 2016/02/09 16:51:01]

Done.

+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)

[philipj_slow, 2016/02/09 12:02:03]

Lower-case convertTo

[Guido Urdaneta, 2016/02/09 16:51:01]

Done. 

+{
+    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 +502,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)
 {
     if (successCallback && errorCallback) {
         UseCounter::count(context, UseCounter::RTCPeerConnectionSetLocalDescriptionLegacyCompliant);
@@ -471,13 +525,14 @@ void RTCPeerConnection::setLocalDescription(ExecutionContext* context, RTCSessio
             UseCounter::count(context, UseCounter::RTCPeerConnectionSetLocalDescriptionLegacyNoFailureCallback);
     }
 
-    if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
-        return;
+    if (callErrorCallbackIfSignalingStateClosed(m_signalingState, errorCallback))

[philipj_slow, 2016/02/09 12:02:03]

This can synchronously invoke the error callback inside the
setLocalDescriptionCall(), which I suspect isn't what was intended by the spec.
It defines this in terms of the promise variant, but says "Upon rejection of p
with reason r, invoke failureCallback with r as the argument" which is too
imprecise to say what it means.

Either it means to add steps to the call sites that rejects the promise, or it
means that the callback should be called at the same time as promise rejection
callbacks are invoked. I think only the second interpretation makes sense,
because it is what could be implemented using JavaScript on top of the promise
API, and it avoids invoking the error callback synchronously.

[Guido Urdaneta, 2016/02/09 16:51:01]

Now calling the error callback asynchronously.

[philipj_slow, 2016/02/09 18:59:17]

Thanks, see new comments for more timing fun :)

+        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 +544,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)
 {
     if (successCallback && errorCallback) {
         UseCounter::count(context, UseCounter::RTCPeerConnectionSetRemoteDescriptionLegacyCompliant);
@@ -500,13 +567,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 +676,40 @@ 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);
+    if (!implemented)
+        resolver->reject(DOMException::create(NotSupportedError, "This method is not yet implemented."));

[philipj_slow, 2016/02/09 12:02:03]

Can this actually happen? The whole source code is under your control, no? :)

[Guido Urdaneta, 2016/02/09 16:51:01]

Apparently, it can. 
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

I left NotSupportedError because that is the exception that was being thrown in
the legacy. Looking at the spec, we should return DOMError(TBD), or perhaps
DOMError("InvalidCandidate"), as can be inferred from the spec bug.

[philipj_slow, 2016/02/09 18:59:17]

OK, it's not obvious from RTCPeerConnectionHandler::addICECandidate that
returning false means a missing implementation, there are other error cases in
WebRtcSession::ProcessIceMessage.

But, this isn't new code. Maybe add a TODO to make it more truthful about the
cause?

[Guido Urdaneta, 2016/02/10 07:05:13]

Done.

+
+    return promise;
 }
 
-void RTCPeerConnection::addIceCandidate(RTCIceCandidate* iceCandidate, VoidCallback* successCallback, RTCErrorCallback* errorCallback, ExceptionState& exceptionState)
+ScriptPromise RTCPeerConnection::addIceCandidate(RTCIceCandidate* iceCandidate, VoidCallback* successCallback, RTCErrorCallback* errorCallback)
 {
-    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)
+        errorCallback->handleEvent("This method is not yet implemented.");

[philipj_slow, 2016/02/09 12:02:03]

This is also a synchronous error callback, but in a different shape.

[Guido Urdaneta, 2016/02/09 16:51:01]

Now called asynchronously.

+
+    return ScriptPromise();
 }
 
 String RTCPeerConnection::signalingState() const