Convert Dictionary handling to RTCConfiguration IDL dictionary

third_party/WebKit/Source/modules/peerconnection/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 12 matching lines @@
  * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
  * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
  * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
  * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "modules/peerconnection/RTCPeerConnection.h"
 
-#include "bindings/core/v8/ArrayValue.h"
 #include "bindings/core/v8/ExceptionMessages.h"
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/Microtask.h"
 #include "bindings/core/v8/Nullable.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "bindings/core/v8/ScriptState.h"
 #include "bindings/core/v8/ScriptValue.h"
 #include "bindings/core/v8/V8ThrowException.h"
 #include "bindings/modules/v8/RTCIceCandidateInitOrRTCIceCandidate.h"
 #include "bindings/modules/v8/V8MediaStreamTrack.h"
 #include "bindings/modules/v8/V8RTCCertificate.h"
 #include "core/dom/DOMException.h"
 #include "core/dom/DOMTimeStamp.h"
 #include "core/dom/Document.h"
 #include "core/dom/ExceptionCode.h"
 #include "core/dom/ExecutionContext.h"
 #include "core/frame/HostsUsingFeatures.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/peerconnection/RTCAnswerOptions.h"
+#include "modules/peerconnection/RTCConfiguration.h"
 #include "modules/peerconnection/RTCDTMFSender.h"
 #include "modules/peerconnection/RTCDataChannel.h"
 #include "modules/peerconnection/RTCDataChannelEvent.h"
 #include "modules/peerconnection/RTCIceCandidateEvent.h"
+#include "modules/peerconnection/RTCIceServer.h"
 #include "modules/peerconnection/RTCOfferOptions.h"
 #include "modules/peerconnection/RTCPeerConnectionErrorCallback.h"
 #include "modules/peerconnection/RTCSessionDescription.h"
 #include "modules/peerconnection/RTCSessionDescriptionCallback.h"
 #include "modules/peerconnection/RTCSessionDescriptionInit.h"
 #include "modules/peerconnection/RTCSessionDescriptionRequestImpl.h"
 #include "modules/peerconnection/RTCSessionDescriptionRequestPromiseImpl.h"
 #include "modules/peerconnection/RTCStatsCallback.h"
 #include "modules/peerconnection/RTCStatsReport.h"
 #include "modules/peerconnection/RTCStatsRequestImpl.h"
@@ skipping 136 matching lines @@
 
   void onSuccess(std::unique_ptr<WebRTCCertificate> certificate) override {
     m_resolver->resolve(new RTCCertificate(std::move(certificate)));
   }
 
   void onError() override { m_resolver->reject(); }
 
   Persistent<ScriptPromiseResolver> m_resolver;
 };
 
-WebRTCConfiguration parseConfiguration(const Dictionary& configuration,
+WebRTCConfiguration parseConfiguration(ExecutionContext* context,
+                                       const RTCConfiguration& configuration,
                                        ExceptionState& exceptionState,
                                        RtcpMuxPolicy* selectedRtcpMuxPolicy) {

[hbos_chromium, 2016/10/24 11:35:24]

nit:
DCHECK(context) or make it ExecutionContext&
DCHECK(selectedRtcpMuxPolicy) or make it RtcpMuxPolicy&

[foolip, 2016/10/24 12:05:26]

Done with DCHECKs.

-  WebRTCConfiguration rtcConfiguration;
-  if (configuration.isUndefinedOrNull())
-    return WebRTCConfiguration();
-
   WebRTCIceTransports iceTransports = WebRTCIceTransports::kAll;
-  String iceTransportsString;
-  if (DictionaryHelper::get(configuration, "iceTransports",
-                            iceTransportsString)) {
-    if (iceTransportsString == "none") {
-      iceTransports = WebRTCIceTransports::kNone;
-    } else if (iceTransportsString == "relay") {
-      iceTransports = WebRTCIceTransports::kRelay;
-    } else if (iceTransportsString != "all") {
-      exceptionState.throwTypeError("Malformed RTCIceTransports");
-      return WebRTCConfiguration();
-    }
-  }
-
-  ArrayValue iceServers;
-  bool ok = DictionaryHelper::get(configuration, "iceServers", iceServers);
-  if (!ok || iceServers.isUndefinedOrNull()) {
-    exceptionState.throwTypeError("Malformed RTCConfiguration");
-    return WebRTCConfiguration();
-  }
-
-  size_t numberOfServers;
-  ok = iceServers.length(numberOfServers);
-  if (!ok) {
-    exceptionState.throwTypeError("Malformed RTCConfiguration");
-    return WebRTCConfiguration();
+  String iceTransportsString = configuration.iceTransports();
+  if (iceTransportsString == "none") {
+    UseCounter::count(context, UseCounter::RTCConfigurationIceTransportsNone);
+    iceTransports = WebRTCIceTransports::kNone;
+  } else if (iceTransportsString == "relay") {
+    iceTransports = WebRTCIceTransports::kRelay;
+  } else {
+    DCHECK_EQ(iceTransportsString, "all");
   }
 
   WebRTCBundlePolicy bundlePolicy = WebRTCBundlePolicy::kBalanced;
-  String bundlePolicyString;
-  if (DictionaryHelper::get(configuration, "bundlePolicy",
-                            bundlePolicyString)) {
-    if (bundlePolicyString == "max-compat") {
-      bundlePolicy = WebRTCBundlePolicy::kMaxCompat;
-    } else if (bundlePolicyString == "max-bundle") {
-      bundlePolicy = WebRTCBundlePolicy::kMaxBundle;
-    } else if (bundlePolicyString != "balanced") {
-      exceptionState.throwTypeError("Malformed RTCBundlePolicy");
-      return WebRTCConfiguration();
-    }
+  String bundlePolicyString = configuration.bundlePolicy();
+  if (bundlePolicyString == "max-compat") {
+    bundlePolicy = WebRTCBundlePolicy::kMaxCompat;
+  } else if (bundlePolicyString == "max-bundle") {
+    bundlePolicy = WebRTCBundlePolicy::kMaxBundle;
+  } else {
+    DCHECK_EQ(bundlePolicyString, "balanced");
   }
 
   // For the histogram value of "WebRTC.PeerConnection.SelectedRtcpMuxPolicy".
   *selectedRtcpMuxPolicy = RtcpMuxPolicyDefault;
   WebRTCRtcpMuxPolicy rtcpMuxPolicy = WebRTCRtcpMuxPolicy::kNegotiate;
-  String rtcpMuxPolicyString;
-  if (DictionaryHelper::get(configuration, "rtcpMuxPolicy",
-                            rtcpMuxPolicyString)) {
+  if (configuration.hasRtcpMuxPolicy()) {
+    String rtcpMuxPolicyString = configuration.rtcpMuxPolicy();
     if (rtcpMuxPolicyString == "require") {
       *selectedRtcpMuxPolicy = RtcpMuxPolicyRequire;
       rtcpMuxPolicy = WebRTCRtcpMuxPolicy::kRequire;
-    } else if (rtcpMuxPolicyString == "negotiate") {
+    } else {
+      DCHECK_EQ(rtcpMuxPolicyString, "negotiate");
       *selectedRtcpMuxPolicy = RtcpMuxPolicyNegotiate;
-      rtcpMuxPolicy = WebRTCRtcpMuxPolicy::kNegotiate;
-    } else {
-      exceptionState.throwTypeError("Malformed RTCRtcpMuxPolicy");
-      return WebRTCConfiguration();
     }
   }
 
-  rtcConfiguration.iceTransports = iceTransports;
-  rtcConfiguration.bundlePolicy = bundlePolicy;
-  rtcConfiguration.rtcpMuxPolicy = rtcpMuxPolicy;
+  WebRTCConfiguration webConfiguration;
+  webConfiguration.iceTransports = iceTransports;
+  webConfiguration.bundlePolicy = bundlePolicy;
+  webConfiguration.rtcpMuxPolicy = rtcpMuxPolicy;
 
-  for (size_t i = 0; i < numberOfServers; ++i) {
-    Dictionary iceServer;
-    ok = iceServers.get(i, iceServer);
-    if (!ok) {
-      exceptionState.throwTypeError("Malformed RTCIceServer");
-      return WebRTCConfiguration();
-    }
-
-    Vector<String> names;
-    iceServer.getPropertyNames(names);
-
-    Vector<String> urlStrings;
-    if (names.contains("urls")) {
-      if (!DictionaryHelper::get(iceServer, "urls", urlStrings) ||
-          !urlStrings.size()) {
-        String urlString;
-        if (DictionaryHelper::get(iceServer, "urls", urlString)) {
-          urlStrings.append(urlString);
+  if (configuration.hasIceServers()) {
+    Vector<WebRTCIceServer> iceServers;
+    for (const RTCIceServer& iceServer : configuration.iceServers()) {
+      Vector<String> urlStrings;
+      if (iceServer.hasURLs()) {
+        UseCounter::count(context, UseCounter::RTCIceServerURLs);
+        const StringOrStringSequence& urls = iceServer.urls();
+        if (urls.isString()) {
+          urlStrings.append(urls.getAsString());
         } else {
-          exceptionState.throwTypeError("Malformed RTCIceServer");
-          return WebRTCConfiguration();
+          DCHECK(urls.isStringSequence());
+          urlStrings = urls.getAsStringSequence();
         }
-      }
-    } else if (names.contains("url")) {
-      String urlString;
-      if (DictionaryHelper::get(iceServer, "url", urlString)) {
-        urlStrings.append(urlString);
+      } else if (iceServer.hasURL()) {
+        UseCounter::count(context, UseCounter::RTCIceServerURL);
+        urlStrings.append(iceServer.url());
       } else {
         exceptionState.throwTypeError("Malformed RTCIceServer");
         return WebRTCConfiguration();
       }
-    } else {
-      exceptionState.throwTypeError("Malformed RTCIceServer");
-      return WebRTCConfiguration();
+
+      String username = iceServer.username();
+      String credential = iceServer.credential();
+
+      for (const String& urlString : urlStrings) {
+        KURL url(KURL(), urlString);
+        if (!url.isValid() ||
+            !(url.protocolIs("turn") || url.protocolIs("turns") ||
+              url.protocolIs("stun"))) {
+          exceptionState.throwTypeError("Malformed URL");
+          return WebRTCConfiguration();
+        }
+        iceServers.append(WebRTCIceServer{url, username, credential});

[hbos_chromium, 2016/10/24 11:35:24]

Add TODO that there should be one WebRTCIceServer with multiple "urls" instead
of multiple WebRTCIceServer with single "url"s.

[foolip, 2016/10/24 12:05:26]

The spec has a kind of flattening into a validatedServers list in
https://w3c.github.io/webrtc-pc/#set-pc-configuration. Assuming no differences
in semantics between one RTCIceServer with multiple URLs and multiple
RTCIceServer entries, this flattening probably makes implementation easier, so
will stay around I think.

+      }
     }
-
-    String username, credential;
-    DictionaryHelper::get(iceServer, "username", username);
-    DictionaryHelper::get(iceServer, "credential", credential);
-
-    Vector<WebRTCIceServer> iceServers;
-    for (Vector<String>::iterator iter = urlStrings.begin();
-         iter != urlStrings.end(); ++iter) {
-      KURL url(KURL(), *iter);
-      if (!url.isValid() ||
-          !(url.protocolIs("turn") || url.protocolIs("turns") ||
-            url.protocolIs("stun"))) {
-        exceptionState.throwTypeError("Malformed URL");
-        return WebRTCConfiguration();
-      }
-      iceServers.append(WebRTCIceServer{url, username, credential});
-    }
-    rtcConfiguration.iceServers = iceServers;
+    webConfiguration.iceServers = iceServers;
   }
 
-  ArrayValue certificates;
-  if (DictionaryHelper::get(configuration, "certificates", certificates) &&
-      !certificates.isUndefinedOrNull()) {
-    size_t numberOfCertificates;
-    certificates.length(numberOfCertificates);
+  if (configuration.hasCertificates()) {
+    const HeapVector<Member<RTCCertificate>>& certificates =
+        configuration.certificates();
+    size_t numberOfCertificates = certificates.size();

[hbos_chromium, 2016/10/24 11:35:24]

nit: remove numberOfCertificates in favor of certificates.size()

[foolip, 2016/10/24 12:05:26]

Done.

     WebVector<std::unique_ptr<WebRTCCertificate>> certificatesCopy(
         numberOfCertificates);
     for (size_t i = 0; i < numberOfCertificates; ++i) {
-      RTCCertificate* certificate = nullptr;
+      certificatesCopy[i] = certificates[i]->certificateShallowCopy();
+    }
+    webConfiguration.certificates = std::move(certificatesCopy);
+  }
 
-      Dictionary dictCert;
-      certificates.get(i, dictCert);
-      v8::Local<v8::Value> valCert = dictCert.v8Value();
-      if (!valCert.IsEmpty()) {
-        certificate = V8RTCCertificate::toImplWithTypeCheck(
-            configuration.isolate(), valCert);
-      }
-      if (!certificate) {
-        exceptionState.throwTypeError("Malformed sequence<RTCCertificate>");
-        return WebRTCConfiguration();
-      }
-
-      certificatesCopy[i] = certificate->certificateShallowCopy();
-    }
-    rtcConfiguration.certificates = std::move(certificatesCopy);
-  }
-  return rtcConfiguration;
+  return webConfiguration;
 }
 
 RTCOfferOptionsPlatform* parseOfferOptions(const Dictionary& options) {
   if (options.isUndefinedOrNull())
     return 0;
 
   Vector<String> propertyNames;
   options.getPropertyNames(propertyNames);
 
   // Treat |options| as MediaConstraints if it is empty or has "optional" or
@@ skipping 62 matching lines @@
   if (m_setupFunction) {
     return (*m_setupFunction)();
   }
   return true;
 }
 
 DEFINE_TRACE(RTCPeerConnection::EventWrapper) {
   visitor->trace(m_event);
 }
 
-RTCPeerConnection* RTCPeerConnection::create(ExecutionContext* context,
-                                             const Dictionary& rtcConfiguration,
-                                             const Dictionary& mediaConstraints,
-                                             ExceptionState& exceptionState) {
+RTCPeerConnection* RTCPeerConnection::create(
+    ExecutionContext* context,
+    const RTCConfiguration& rtcConfiguration,
+    const Dictionary& mediaConstraints,
+    ExceptionState& exceptionState) {
   if (mediaConstraints.isObject())
     UseCounter::count(context,
                       UseCounter::RTCPeerConnectionConstructorConstraints);
   else
     UseCounter::count(context,
                       UseCounter::RTCPeerConnectionConstructorCompliant);
 
   // Record the RtcpMuxPolicy for histogram
   // "WebRTC.PeerConnection.SelectedRtcpMuxPolicy".
   RtcpMuxPolicy selectedRtcpMuxPolicy = RtcpMuxPolicyDefault;
   WebRTCConfiguration configuration = parseConfiguration(
-      rtcConfiguration, exceptionState, &selectedRtcpMuxPolicy);
+      context, rtcConfiguration, exceptionState, &selectedRtcpMuxPolicy);
   if (exceptionState.hadException())
     return 0;
 
   // Make sure no certificates have expired.
+  // TODO(hbos): Should this be part of parseConfiguration and thus apply to
+  // updateIce as well?

[hbos_chromium, 2016/10/24 11:35:24]

Remove TODO comment.

Because I'm lazy.
Kidding. Because it is still a valid RTCConfiguration even if the certificates
are expired so I don't think parseConfiguration should fail, but it is invalid
to try to use an expired certificate in the construction of a peer connection.
It is not invalid to use the same certificates in updateIce even if they expired
in-between construction and updateIce.

[foolip, 2016/10/24 12:05:25]

You're right, https://w3c.github.io/webrtc-pc/#dom-peerconnection has this in
the spec but
https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-setconfiguration does
not.

   if (configuration.certificates.size() > 0) {
     DOMTimeStamp now = convertSecondsToDOMTimeStamp(currentTime());
     for (const std::unique_ptr<WebRTCCertificate>& certificate :
          configuration.certificates) {
       DOMTimeStamp expires = certificate->expires();
       if (expires <= now) {
         // TODO(hbos): Per https://w3c.github.io/webrtc-pc/#operation this
         // should throw InvalidAccessError, not InvalidStateError.
         exceptionState.throwDOMException(InvalidStateError,
                                          "Expired certificate(s).");
@@ skipping 333 matching lines @@
 
 RTCSessionDescription* RTCPeerConnection::remoteDescription() {
   WebRTCSessionDescription webSessionDescription =
       m_peerHandler->remoteDescription();
   if (webSessionDescription.isNull())
     return nullptr;
 
   return RTCSessionDescription::create(webSessionDescription);
 }
 
-void RTCPeerConnection::updateIce(const Dictionary& rtcConfiguration,
+void RTCPeerConnection::updateIce(ExecutionContext* context,

[hbos_chromium, 2016/10/24 11:35:24]

Note that not only is the name different from the spec (should be called
setConfiguration), but the implementation is different. Both here and in lower
layers (m_peerHandler->updateICE). File a bug?

Can you add a UseCounter for updateIce?

[foolip, 2016/10/24 12:05:26]

I added [Measure] to updateIce in https://codereview.chromium.org/2413703002

There's https://bugs.chromium.org/p/chromium/issues/detail?id=581348 and
https://bugs.chromium.org/p/chromium/issues/detail?id=587453, can you dupe one
into the other and comment on the real one that it replaces updateIce?

+                                  const RTCConfiguration& rtcConfiguration,
                                   const Dictionary& mediaConstraints,
                                   ExceptionState& exceptionState) {
   if (throwExceptionIfSignalingStateClosed(m_signalingState, exceptionState))
     return;
 
   RtcpMuxPolicy selectedRtcpMuxPolicy = RtcpMuxPolicyDefault;
   WebRTCConfiguration configuration = parseConfiguration(
-      rtcConfiguration, exceptionState, &selectedRtcpMuxPolicy);
+      context, rtcConfiguration, exceptionState, &selectedRtcpMuxPolicy);
 
   if (exceptionState.hadException())
     return;
 
   MediaErrorState mediaErrorState;
   if (mediaErrorState.hadException()) {
     mediaErrorState.raiseException(exceptionState);
     return;
   }
 
@@ skipping 658 matching lines @@
 DEFINE_TRACE(RTCPeerConnection) {
   visitor->trace(m_localStreams);
   visitor->trace(m_remoteStreams);
   visitor->trace(m_dispatchScheduledEventRunner);
   visitor->trace(m_scheduledEvents);
   EventTargetWithInlineData::trace(visitor);
   ActiveDOMObject::trace(visitor);
 }
 
 }  // namespace blink