Add promise-based addIceCandidate, setLocalDescription and setRemoteDescription to RTCPeerConnection

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

 /*
  * Copyright (C) 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 17 matching lines @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "modules/mediastream/RTCPeerConnection.h"
 
 #include "bindings/core/v8/ArrayValue.h"
 #include "bindings/core/v8/ExceptionMessages.h"
 #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"
 #include "core/dom/ExecutionContext.h"
 #include "core/frame/LocalFrame.h"
 #include "core/frame/UseCounter.h"
 #include "core/html/VoidCallback.h"
 #include "core/loader/FrameLoader.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "modules/crypto/CryptoResultImpl.h"
 #include "modules/mediastream/MediaConstraintsImpl.h"
 #include "modules/mediastream/MediaStreamEvent.h"
 #include "modules/mediastream/RTCDTMFSender.h"
 #include "modules/mediastream/RTCDataChannel.h"
 #include "modules/mediastream/RTCDataChannelEvent.h"
 #include "modules/mediastream/RTCErrorCallback.h"
 #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"
 #include "public/platform/WebCryptoAlgorithmParams.h"
 #include "public/platform/WebCryptoUtil.h"
 #include "public/platform/WebMediaStream.h"
 #include "public/platform/WebRTCCertificate.h"
 #include "public/platform/WebRTCCertificateGenerator.h"
 #include "public/platform/WebRTCConfiguration.h"
 #include "public/platform/WebRTCDataChannelHandler.h"
 #include "public/platform/WebRTCDataChannelInit.h"
 #include "public/platform/WebRTCICECandidate.h"
 #include "public/platform/WebRTCKeyParams.h"
 #include "public/platform/WebRTCOfferOptions.h"
 #include "public/platform/WebRTCSessionDescription.h"
 #include "public/platform/WebRTCSessionDescriptionRequest.h"
 #include "public/platform/WebRTCStatsRequest.h"
 #include "public/platform/WebRTCVoidRequest.h"
 
 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:
     // The created observer is responsible for deleting itself after onSuccess/onError. To avoid memory
     // leak the observer should therefore be used in a WebRTCCertificateGenerator::generateCertificate call
     // which is ensured to invoke one of these. Takes ownership of |resolver|.
     static WebRTCCertificateObserver* create(ScriptPromiseResolver* resolver)
     {
         return new WebRTCCertificateObserver(resolver);
     }
@@ skipping 349 matching lines @@
     WebMediaConstraints constraints = MediaConstraintsImpl::create(context, mediaConstraints, mediaErrorState);
     if (mediaErrorState.hadException()) {
         mediaErrorState.raiseException(exceptionState);
         return;
     }
 
     RTCSessionDescriptionRequest* request = RTCSessionDescriptionRequestImpl::create(executionContext(), this, successCallback, errorCallback);
     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);
     } else {
         if (!successCallback)
             UseCounter::count(context, UseCounter::RTCPeerConnectionSetLocalDescriptionLegacyNoSuccessCallback);
         if (!errorCallback)
             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()
 {
     WebRTCSessionDescription webSessionDescription = m_peerHandler->localDescription();
     if (webSessionDescription.isNull())
         return nullptr;
 
     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);
     } else {
         if (!successCallback)
             UseCounter::count(context, UseCounter::RTCPeerConnectionSetRemoteDescriptionLegacyNoSuccessCallback);
         if (!errorCallback)
             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()
 {
     WebRTCSessionDescription webSessionDescription = m_peerHandler->remoteDescription();
     if (webSessionDescription.isNull())
         return nullptr;
 
     return RTCSessionDescription::create(webSessionDescription);
 }
@@ skipping 81 matching lines @@
     // The observer will manage its own destruction as well as the resolver's destruction.
     certificateGenerator->generateCertificate(
         keyParams.get(),
         toDocument(scriptState->executionContext())->url(),
         toDocument(scriptState->executionContext())->firstPartyForCookies(),
         certificateObserver);
 
     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);
 
+    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)
+        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.

 
-    bool implemented = m_peerHandler->addICECandidate(request, iceCandidate->webCandidate());
-    if (!implemented) {
-        exceptionState.throwDOMException(NotSupportedError, "This method is not yet implemented.");
-    }
+    return ScriptPromise();
 }
 
 String RTCPeerConnection::signalingState() const
 {
     switch (m_signalingState) {
     case SignalingStateStable:
         return "stable";
     case SignalingStateHaveLocalOffer:
         return "have-local-offer";
     case SignalingStateHaveRemoteOffer:
@@ skipping 401 matching lines @@
 {
     visitor->trace(m_localStreams);
     visitor->trace(m_remoteStreams);
     visitor->trace(m_dispatchScheduledEventRunner);
     visitor->trace(m_scheduledEvents);
     RefCountedGarbageCollectedEventTargetWithInlineData<RTCPeerConnection>::trace(visitor);
     ActiveDOMObject::trace(visitor);
 }
 
 } // namespace blink