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

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 341 matching lines @@
                 return 0;
             }
 
             rtcConfiguration->appendCertificate(certificate->certificateShallowCopy());
         }
     }
 
     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;
 
     Vector<String> propertyNames;
     options.getPropertyNames(propertyNames);
 
     // Treat |options| as MediaConstraints if it is empty or has "optional" or "mandatory" properties for compatibility.
     // TODO(jiayl): remove constraints when RTCOfferOptions reaches Stable and client code is ready.
     if (propertyNames.isEmpty() || propertyNames.contains("optional") || propertyNames.contains("mandatory"))
         return 0;
 
     int32_t offerToReceiveVideo = -1;
     int32_t offerToReceiveAudio = -1;
     bool voiceActivityDetection = true;
     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;
     }
 
     DictionaryHelper::get(options, "voiceActivityDetection", voiceActivityDetection);
     DictionaryHelper::get(options, "iceRestart", iceRestart);
 
     RTCOfferOptions* rtcOfferOptions = RTCOfferOptions::create(offerToReceiveVideo, offerToReceiveAudio, voiceActivityDetection, iceRestart);
     return rtcOfferOptions;
 }
 
@@ skipping 61 matching lines @@
     }
 }
 
 RTCPeerConnection::~RTCPeerConnection()
 {
     // This checks that close() or stop() is called before the destructor.
     // We are assuming that a wrapper is always created when RTCPeerConnection is created.
     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))
+    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;
 
-    ASSERT(successCallback);
-
-    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);
 
     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);
     } else {
         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;
     }
 
     RTCSessionDescriptionRequest* request = RTCSessionDescriptionRequestImpl::create(executionContext(), this, successCallback, errorCallback);
     m_peerHandler->createAnswer(request, constraints);
 }
 
 ScriptPromise RTCPeerConnection::setLocalDescription(ScriptState* scriptState, const RTCSessionDescriptionInit& sessionDescriptionInit)
 {
     if (m_signalingState == SignalingStateClosed)
@@ skipping 619 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