Report errors in RTCPeerConnection legacy functions via the the failure callback instead of excepti…

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 e9d8a83a8216914370e554a463933fb421ee3a09..a2280e5f52a70fcf121116cc441767f3cdbc4e65 100644
--- a/third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
+++ b/third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
@@ -359,8 +359,9 @@ RTCConfiguration* RTCPeerConnection::parseConfiguration(const Dictionary& config
     return rtcConfiguration;
 }
 
-RTCOfferOptions* RTCPeerConnection::parseOfferOptions(const Dictionary& options, ExceptionState& exceptionState)
+RTCOfferOptions* RTCPeerConnection::parseOfferOptions(const Dictionary& options, RTCErrorCallback* errorCallback, bool& hadError)
 {
+    hadError = false;
     if (options.isUndefinedOrNull())
         return 0;
 
@@ -378,12 +379,14 @@ RTCOfferOptions* RTCPeerConnection::parseOfferOptions(const Dictionary& options,
     bool iceRestart = false;
 
     if (DictionaryHelper::get(options, "offerToReceiveVideo", offerToReceiveVideo) && offerToReceiveVideo < 0) {
-        exceptionState.throwTypeError("Invalid offerToReceiveVideo");
+        asyncCallErrorCallback(errorCallback, "Invalid offerToReceiveVideo");

[philipj_slow, 2016/02/22 10:03:41]

Before this would throw a TypeError, and if I'm reading ErrorCallbackTask it
will now call the error callback with a string argument? I failed to pay
attention to this in the previous review, but that doesn't match what the spec
says, and I think the other calls to asyncCallErrorCallback have the same
problem.

Because the handling of RTCOfferOptions should eventually move into generated
bindings, this will presumably end up as a promise rejected with a TypeError
when everything has settled down. Because that should be handled by bindings, it
wouldn't be passed to the error callback, which should only ever take a
DOMException per spec.

[Guido Urdaneta, 2016/02/22 11:37:50]

Yes. The spec uses RTCPeerConnectionErrorCallback, which takes a DOMException.
Chromium is using the older RTCErrorCallback, which takes a String. We cannot
migrate to RTCPeerConnectionErrorCallback right away, as it will break all
existing applications, which expect a string.

So, the plan is:
1. Make the promise-based versions fully spec compliant.
2. Continue supporting the existing legacy version with as few changes as
possible.
3. Make the legacy version more and more compliant with the spec as uncompliant
use cases become less popular. If we are lucky, at some point we will make it
fully compliant.
The TypeErrors that are currently being thrown (and which I am proposing to
report via the callback) correspond to use cases that are not compliant with the
spec but which are commonplace today. In a fully compliant implementation those
TypeErrors would not exist.
The only TypeErrors that would exist in a 100% spec-compliant implementation
will be the cases were it is impossible to know which of the different versions
of the methods is being requested, so in those cases there would be no need to
invoke the error callback.
However, we are not there yet. There are a couple of errors that our legacy
implementation is reporting via exceptions and those will not be able to be
handled once we introduce the promise-based versions since we will have to
change the legacy method to return Promise<void>.

I think the best  way to avoid surprises once we do that is to report those
errors using the callback. The fact that we are still on the old
RTCErrorCallback that gets strings instead of DOMExceptions is actually an
advantage during the transition.

I guess the plan for the legacy methods would be:
1. Report constraint and old-style RTCOfferOptions parsing errors via the error
callback as strings. (This CL)
2. Remove support for constraints arguments.
3. Remove support for old-style RTCOfferOptions.
At this point, asyncCallErrorCallback will be called only when the connection
state is closed.
4. Replace RTCErrorCallback with RTCPeerConnectionErrorCallback and report
errors are DOMExceptions instead of strings.

Steps 2, 3 and 4 can proceed only when the respective use cases become rare,
which is not the case at the moment.

+        hadError = true;
         return 0;
     }
 
     if (DictionaryHelper::get(options, "offerToReceiveAudio", offerToReceiveAudio) && offerToReceiveAudio < 0) {
-        exceptionState.throwTypeError("Invalid offerToReceiveAudio");
+        asyncCallErrorCallback(errorCallback, "Invalid offerToReceiveAudio");
+        hadError = true;
         return 0;
     }
 
@@ -465,20 +468,17 @@ RTCPeerConnection::~RTCPeerConnection()
     ASSERT(m_closed || m_stopped);
 }
 
-void RTCPeerConnection::createOffer(ExecutionContext* context, RTCSessionDescriptionCallback* successCallback, RTCErrorCallback* errorCallback, const Dictionary& rtcOfferOptions, ExceptionState& exceptionState)
+void RTCPeerConnection::createOffer(ExecutionContext* context, RTCSessionDescriptionCallback* successCallback, RTCErrorCallback* errorCallback, const Dictionary& rtcOfferOptions)
 {
-    if (errorCallback)
-        UseCounter::count(context, UseCounter::RTCPeerConnectionCreateOfferLegacyFailureCallback);
-    else
-        Deprecation::countDeprecation(context, UseCounter::RTCPeerConnectionCreateOfferLegacyNoFailureCallback);
-
-    if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
-        return;
-
     ASSERT(successCallback);
+    ASSERT(errorCallback);

[philipj_slow, 2016/02/22 10:03:41]

The nullability of errorCallback didn't change with this CL, I guess this was
changed earlier without changing the implementation?

[Guido Urdaneta, 2016/02/22 11:37:50]

Correct. There was a previous CL making the error callback mandatory which only
changed the IDL, but not the implementation.
If we decide not to land this CL, we can prepare a separate CL that only
assumes/asserts that the error callbacks are not null.

[philipj_slow, 2016/02/23 09:13:54]

Sounds fine to do here, just checking.

+    UseCounter::count(context, UseCounter::RTCPeerConnectionCreateOfferLegacyFailureCallback);
+    if (callErrorCallbackIfSignalingStateClosed(m_signalingState, errorCallback))
+        return;
 
-    RTCOfferOptions* offerOptions = parseOfferOptions(rtcOfferOptions, exceptionState);
-    if (exceptionState.hadException())
+    bool hadOfferOptionsError = false;
+    RTCOfferOptions* offerOptions = parseOfferOptions(rtcOfferOptions, errorCallback, hadOfferOptionsError);
+    if (hadOfferOptionsError)
         return;
 
     RTCSessionDescriptionRequest* request = RTCSessionDescriptionRequestImpl::create(executionContext(), this, successCallback, errorCallback);
@@ -486,7 +486,7 @@ void RTCPeerConnection::createOffer(ExecutionContext* context, RTCSessionDescrip
     if (offerOptions) {
         if (offerOptions->offerToReceiveAudio() != -1 || offerOptions->offerToReceiveVideo() != -1)
             UseCounter::count(context, UseCounter::RTCPeerConnectionCreateOfferLegacyOfferOptions);
-        else if (errorCallback)
+        else
             UseCounter::count(context, UseCounter::RTCPeerConnectionCreateOfferLegacyCompliant);
 
         m_peerHandler->createOffer(request, offerOptions);
@@ -494,40 +494,38 @@ void RTCPeerConnection::createOffer(ExecutionContext* context, RTCSessionDescrip
         MediaErrorState mediaErrorState;
         WebMediaConstraints constraints = MediaConstraintsImpl::create(context, rtcOfferOptions, mediaErrorState);
         if (mediaErrorState.hadException()) {
-            mediaErrorState.raiseException(exceptionState);
+            String errorMsg = mediaErrorState.getErrorMessage();
+            asyncCallErrorCallback(errorCallback, errorMsg);
             return;
         }
 
         if (!constraints.isEmpty())
             UseCounter::count(context, UseCounter::RTCPeerConnectionCreateOfferLegacyConstraints);
-        else if (errorCallback)
+        else
             UseCounter::count(context, UseCounter::RTCPeerConnectionCreateOfferLegacyCompliant);
 
         m_peerHandler->createOffer(request, constraints);
     }
 }
 
-void RTCPeerConnection::createAnswer(ExecutionContext* context, RTCSessionDescriptionCallback* successCallback, RTCErrorCallback* errorCallback, const Dictionary& mediaConstraints, ExceptionState& exceptionState)
+void RTCPeerConnection::createAnswer(ExecutionContext* context, RTCSessionDescriptionCallback* successCallback, RTCErrorCallback* errorCallback, const Dictionary& mediaConstraints)
 {
-    if (errorCallback)
-        UseCounter::count(context, UseCounter::RTCPeerConnectionCreateAnswerLegacyFailureCallback);
-    else
-        Deprecation::countDeprecation(context, UseCounter::RTCPeerConnectionCreateAnswerLegacyNoFailureCallback);
-
+    ASSERT(successCallback);
+    ASSERT(errorCallback);
+    UseCounter::count(context, UseCounter::RTCPeerConnectionCreateAnswerLegacyFailureCallback);
     if (mediaConstraints.isObject())
         UseCounter::count(context, UseCounter::RTCPeerConnectionCreateAnswerLegacyConstraints);
-    else if (errorCallback)
+    else
         UseCounter::count(context, UseCounter::RTCPeerConnectionCreateAnswerLegacyCompliant);
 
-    if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
+    if (callErrorCallbackIfSignalingStateClosed(m_signalingState, errorCallback))
         return;
 
-    ASSERT(successCallback);
-
     MediaErrorState mediaErrorState;
     WebMediaConstraints constraints = MediaConstraintsImpl::create(context, mediaConstraints, mediaErrorState);
     if (mediaErrorState.hadException()) {
-        mediaErrorState.raiseException(exceptionState);
+        String errorMsg = mediaErrorState.getErrorMessage();
+        asyncCallErrorCallback(errorCallback, errorMsg);
         return;
     }