Revert of Adding error output param to SetConfiguration, using new RTCError type.

webrtc/api/peerconnection.cc

Index: webrtc/api/peerconnection.cc
diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc
index a2b5e3631ef090fc350ad4b4ffd42b8fb7edeb3a..ebce40ca5eb0cf37d03243663826ea911eda06c3 100644
--- a/webrtc/api/peerconnection.cc
+++ b/webrtc/api/peerconnection.cc
@@ -49,8 +49,6 @@
 using webrtc::MediaConstraintsInterface;
 using webrtc::MediaStreamInterface;
 using webrtc::PeerConnectionInterface;
-using webrtc::RTCError;
-using webrtc::RTCErrorType;
 using webrtc::RtpSenderInternal;
 using webrtc::RtpSenderInterface;
 using webrtc::RtpSenderProxy;
@@ -210,11 +208,10 @@
 
 // Adds a STUN or TURN server to the appropriate list,
 // by parsing |url| and using the username/password in |server|.
-RTCErrorType ParseIceServerUrl(
-    const PeerConnectionInterface::IceServer& server,
-    const std::string& url,
-    cricket::ServerAddresses* stun_servers,
-    std::vector<cricket::RelayServerConfig>* turn_servers) {
+bool ParseIceServerUrl(const PeerConnectionInterface::IceServer& server,
+                       const std::string& url,
+                       cricket::ServerAddresses* stun_servers,
+                       std::vector<cricket::RelayServerConfig>* turn_servers) {
   // draft-nandakumar-rtcweb-stun-uri-01
   // stunURI       = scheme ":" stun-host [ ":" stun-port ]
   // scheme        = "stun" / "stuns"
@@ -242,14 +239,14 @@
     rtc::tokenize_with_empty_tokens(uri_transport_param, '=', &tokens);
     if (tokens[0] != kTransport) {
       LOG(LS_WARNING) << "Invalid transport parameter key.";
-      return RTCErrorType::SYNTAX_ERROR;
+      return false;
     }
     if (tokens.size() < 2 ||
         !cricket::StringToProto(tokens[1].c_str(), &turn_transport_type) ||
         (turn_transport_type != cricket::PROTO_UDP &&
          turn_transport_type != cricket::PROTO_TCP)) {
       LOG(LS_WARNING) << "Transport param should always be udp or tcp.";
-      return RTCErrorType::SYNTAX_ERROR;
+      return false;
     }
   }
 
@@ -259,7 +256,7 @@
                                        &service_type,
                                        &hoststring)) {
     LOG(LS_WARNING) << "Invalid transport parameter in ICE URI: " << url;
-    return RTCErrorType::SYNTAX_ERROR;
+    return false;
   }
 
   // GetServiceTypeAndHostnameFromUri should never give an empty hoststring
@@ -272,12 +269,12 @@
   std::string username(server.username);
   if (tokens.size() > kTurnHostTokensNum) {
     LOG(LS_WARNING) << "Invalid user@hostname format: " << hoststring;
-    return RTCErrorType::SYNTAX_ERROR;
+    return false;
   }
   if (tokens.size() == kTurnHostTokensNum) {
     if (tokens[0].empty() || tokens[1].empty()) {
       LOG(LS_WARNING) << "Invalid user@hostname format: " << hoststring;
-      return RTCErrorType::SYNTAX_ERROR;
+      return false;
     }
     username.assign(rtc::s_url_decode(tokens[0]));
     hoststring = tokens[1];
@@ -294,12 +291,12 @@
   std::string address;
   if (!ParseHostnameAndPortFromString(hoststring, &address, &port)) {
     LOG(WARNING) << "Invalid hostname format: " << uri_without_transport;
-    return RTCErrorType::SYNTAX_ERROR;
+    return false;
   }
 
   if (port <= 0 || port > 0xffff) {
     LOG(WARNING) << "Invalid port: " << port;
-    return RTCErrorType::SYNTAX_ERROR;
+    return false;
   }
 
   switch (service_type) {
@@ -309,22 +306,16 @@
       break;
     case TURN:
     case TURNS: {
-      if (username.empty() || server.password.empty()) {
-        // The WebRTC spec requires throwing an InvalidAccessError when username
-        // or credential are ommitted; this is the native equivalent.
-        return RTCErrorType::INVALID_PARAMETER;
-      }
       turn_servers->push_back(cricket::RelayServerConfig(
           address, port, username, server.password, turn_transport_type));
       break;
     }
+    case INVALID:
     default:
-      // We shouldn't get to this point with an invalid service_type, we should
-      // have returned an error already.
-      RTC_DCHECK(false) << "Unexpected service type";
-      return RTCErrorType::INTERNAL_ERROR;
-  }
-  return RTCErrorType::NONE;
+      LOG(WARNING) << "Configuration not supported: " << url;
+      return false;
+  }
+  return true;
 }
 
 // Check if we can send |new_stream| on a PeerConnection.
@@ -436,19 +427,11 @@
   }
 }
 
-// Helper to set an error and return from a method.
-bool SafeSetError(webrtc::RTCErrorType type, webrtc::RTCError* error) {
-  if (error) {
-    error->set_type(type);
-  }
-  return type == webrtc::RTCErrorType::NONE;
-}
-
 }  // namespace
 
 namespace webrtc {
 
-static const char* const kRTCErrorTypeNames[] = {
+static const char* const kRtcErrorNames[] = {
     "NONE",
     "UNSUPPORTED_PARAMETER",
     "INVALID_PARAMETER",
@@ -459,82 +442,12 @@
     "NETWORK_ERROR",
     "INTERNAL_ERROR",
 };
-static_assert(static_cast<int>(RTCErrorType::INTERNAL_ERROR) ==
-                  (arraysize(kRTCErrorTypeNames) - 1),
-              "kRTCErrorTypeNames must have as many strings as RTCErrorType "
-              "has values.");
-
-std::ostream& operator<<(std::ostream& stream, RTCErrorType error) {
+
+std::ostream& operator<<(std::ostream& stream, RtcError error) {
   int index = static_cast<int>(error);
-  return stream << kRTCErrorTypeNames[index];
-}
-
-bool PeerConnectionInterface::RTCConfiguration::operator==(
-    const PeerConnectionInterface::RTCConfiguration& o) const {
-  // This static_assert prevents us from accidentally breaking operator==.
-  struct stuff_being_tested_for_equality {
-    IceTransportsType type;
-    IceServers servers;
-    BundlePolicy bundle_policy;
-    RtcpMuxPolicy rtcp_mux_policy;
-    TcpCandidatePolicy tcp_candidate_policy;
-    CandidateNetworkPolicy candidate_network_policy;
-    int audio_jitter_buffer_max_packets;
-    bool audio_jitter_buffer_fast_accelerate;
-    int ice_connection_receiving_timeout;
-    int ice_backup_candidate_pair_ping_interval;
-    ContinualGatheringPolicy continual_gathering_policy;
-    std::vector<rtc::scoped_refptr<rtc::RTCCertificate>> certificates;
-    bool prioritize_most_likely_ice_candidate_pairs;
-    struct cricket::MediaConfig media_config;
-    bool disable_ipv6;
-    bool enable_rtp_data_channel;
-    bool enable_quic;
-    rtc::Optional<int> screencast_min_bitrate;
-    rtc::Optional<bool> combined_audio_video_bwe;
-    rtc::Optional<bool> enable_dtls_srtp;
-    int ice_candidate_pool_size;
-    bool prune_turn_ports;
-    bool presume_writable_when_fully_relayed;
-    bool enable_ice_renomination;
-    bool redetermine_role_on_ice_restart;
-  };
-  static_assert(sizeof(stuff_being_tested_for_equality) == sizeof(*this),
-                "Did you add something to RTCConfiguration and forget to "
-                "update operator==?");
-  return type == o.type && servers == o.servers &&
-         bundle_policy == o.bundle_policy &&
-         rtcp_mux_policy == o.rtcp_mux_policy &&
-         tcp_candidate_policy == o.tcp_candidate_policy &&
-         candidate_network_policy == o.candidate_network_policy &&
-         audio_jitter_buffer_max_packets == o.audio_jitter_buffer_max_packets &&
-         audio_jitter_buffer_fast_accelerate ==
-             o.audio_jitter_buffer_fast_accelerate &&
-         ice_connection_receiving_timeout ==
-             o.ice_connection_receiving_timeout &&
-         ice_backup_candidate_pair_ping_interval ==
-             o.ice_backup_candidate_pair_ping_interval &&
-         continual_gathering_policy == o.continual_gathering_policy &&
-         certificates == o.certificates &&
-         prioritize_most_likely_ice_candidate_pairs ==
-             o.prioritize_most_likely_ice_candidate_pairs &&
-         media_config == o.media_config && disable_ipv6 == o.disable_ipv6 &&
-         enable_rtp_data_channel == o.enable_rtp_data_channel &&
-         enable_quic == o.enable_quic &&
-         screencast_min_bitrate == o.screencast_min_bitrate &&
-         combined_audio_video_bwe == o.combined_audio_video_bwe &&
-         enable_dtls_srtp == o.enable_dtls_srtp &&
-         ice_candidate_pool_size == o.ice_candidate_pool_size &&
-         prune_turn_ports == o.prune_turn_ports &&
-         presume_writable_when_fully_relayed ==
-             o.presume_writable_when_fully_relayed &&
-         enable_ice_renomination == o.enable_ice_renomination &&
-         redetermine_role_on_ice_restart == o.redetermine_role_on_ice_restart;
-}
-
-bool PeerConnectionInterface::RTCConfiguration::operator!=(
-    const PeerConnectionInterface::RTCConfiguration& o) const {
-  return !(*this == o);
+  RTC_CHECK(index < static_cast<int>(sizeof(kRtcErrorNames) /
+                                     sizeof(kRtcErrorNames[0])));
+  return stream << kRtcErrorNames[index];
 }
 
 // Generate a RTCP CNAME when a PeerConnection is created.
@@ -636,33 +549,28 @@
   return mandatory_constraints_satisfied == constraints->GetMandatory().size();
 }
 
-RTCErrorType ParseIceServers(
-    const PeerConnectionInterface::IceServers& servers,
-    cricket::ServerAddresses* stun_servers,
-    std::vector<cricket::RelayServerConfig>* turn_servers) {
+bool ParseIceServers(const PeerConnectionInterface::IceServers& servers,
+                     cricket::ServerAddresses* stun_servers,
+                     std::vector<cricket::RelayServerConfig>* turn_servers) {
   for (const webrtc::PeerConnectionInterface::IceServer& server : servers) {
     if (!server.urls.empty()) {
       for (const std::string& url : server.urls) {
         if (url.empty()) {
           LOG(LS_ERROR) << "Empty uri.";
-          return RTCErrorType::SYNTAX_ERROR;
+          return false;
         }
-        RTCErrorType err =
-            ParseIceServerUrl(server, url, stun_servers, turn_servers);
-        if (err != RTCErrorType::NONE) {
-          return err;
+        if (!ParseIceServerUrl(server, url, stun_servers, turn_servers)) {
+          return false;
         }
       }
     } else if (!server.uri.empty()) {
       // Fallback to old .uri if new .urls isn't present.
-      RTCErrorType err =
-          ParseIceServerUrl(server, server.uri, stun_servers, turn_servers);
-      if (err != RTCErrorType::NONE) {
-        return err;
+      if (!ParseIceServerUrl(server, server.uri, stun_servers, turn_servers)) {
+        return false;
       }
     } else {
       LOG(LS_ERROR) << "Empty uri.";
-      return RTCErrorType::SYNTAX_ERROR;
+      return false;
     }
   }
   // Candidates must have unique priorities, so that connectivity checks
@@ -672,7 +580,7 @@
     // First in the list gets highest priority.
     turn_server.priority = priority--;
   }
-  return RTCErrorType::NONE;
+  return true;
 }
 
 PeerConnection::PeerConnection(PeerConnectionFactory* factory)
@@ -715,18 +623,12 @@
     std::unique_ptr<rtc::RTCCertificateGeneratorInterface> cert_generator,
     PeerConnectionObserver* observer) {
   TRACE_EVENT0("webrtc", "PeerConnection::Initialize");
-  if (!allocator) {
-    LOG(LS_ERROR) << "PeerConnection initialized without a PortAllocator? "
-                  << "This shouldn't happen if using PeerConnectionFactory.";
-    return false;
-  }
+  RTC_DCHECK(observer != nullptr);
   if (!observer) {
-    // TODO(deadbeef): Why do we do this?
-    LOG(LS_ERROR) << "PeerConnection initialized without a "
-                  << "PeerConnectionObserver";
     return false;
   }
   observer_ = observer;
+
   port_allocator_ = std::move(allocator);
 
   // The port allocator lives on the network thread and should be initialized
@@ -1382,8 +1284,7 @@
   return configuration_;
 }
 
-bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration,
-                                      RTCError* error) {
+bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration) {
   TRACE_EVENT0("webrtc", "PeerConnection::SetConfiguration");
 
   if (session_->local_description() &&
@@ -1391,61 +1292,32 @@
           configuration_.ice_candidate_pool_size) {
     LOG(LS_ERROR) << "Can't change candidate pool size after calling "
                      "SetLocalDescription.";
-    return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error);
-  }
-
-  // The simplest (and most future-compatible) way to tell if the config was
-  // modified in an invalid way is to copy each property we do support
-  // modifying, then use operator==. There are far more properties we don't
-  // support modifying than those we do, and more could be added.
-  RTCConfiguration modified_config = configuration_;
-  modified_config.servers = configuration.servers;
-  modified_config.type = configuration.type;
-  modified_config.ice_candidate_pool_size =
-      configuration.ice_candidate_pool_size;
-  modified_config.prune_turn_ports = configuration.prune_turn_ports;
-  if (configuration != modified_config) {
-    LOG(LS_ERROR) << "Modifying the configuration in an unsupported way.";
-    return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error);
-  }
-
-  // Note that this isn't possible through chromium, since it's an unsigned
-  // short in WebIDL.
-  if (configuration.ice_candidate_pool_size < 0 ||
-      configuration.ice_candidate_pool_size > UINT16_MAX) {
-    return SafeSetError(RTCErrorType::INVALID_RANGE, error);
-  }
-
-  // Parse ICE servers before hopping to network thread.
-  cricket::ServerAddresses stun_servers;
-  std::vector<cricket::RelayServerConfig> turn_servers;
-  RTCErrorType parse_error =
-      ParseIceServers(configuration.servers, &stun_servers, &turn_servers);
-  if (parse_error != RTCErrorType::NONE) {
-    return SafeSetError(parse_error, error);
-  }
-
-  // In theory this shouldn't fail.
-  if (!network_thread()->Invoke<bool>(
-          RTC_FROM_HERE,
-          rtc::Bind(&PeerConnection::ReconfigurePortAllocator_n, this,
-                    stun_servers, turn_servers, modified_config.type,
-                    modified_config.ice_candidate_pool_size,
-                    modified_config.prune_turn_ports))) {
-    LOG(LS_ERROR) << "Failed to apply configuration to PortAllocator.";
-    return SafeSetError(RTCErrorType::INTERNAL_ERROR, error);
-  }
+    return false;
+  }
+  // TODO(deadbeef): Return false and log an error if there are any unsupported
+  // modifications.
+  if (port_allocator_) {
+    if (!network_thread()->Invoke<bool>(
+            RTC_FROM_HERE,
+            rtc::Bind(&PeerConnection::ReconfigurePortAllocator_n, this,
+                      configuration))) {
+      LOG(LS_ERROR) << "Failed to apply configuration to PortAllocator.";
+      return false;
+    }
+  }
+
+  // TODO(deadbeef): Shouldn't have to hop to the network thread twice...
+  session_->SetIceConfig(session_->ParseIceConfig(configuration));
 
   // As described in JSEP, calling setConfiguration with new ICE servers or
   // candidate policy must set a "needs-ice-restart" bit so that the next offer
   // triggers an ICE restart which will pick up the changes.
-  if (modified_config.servers != configuration_.servers ||
-      modified_config.type != configuration_.type ||
-      modified_config.prune_turn_ports != configuration_.prune_turn_ports) {
+  if (configuration.servers != configuration_.servers ||
+      configuration.type != configuration_.type) {
     session_->SetNeedsIceRestartFlag();
   }
-  configuration_ = modified_config;
-  return SafeSetError(RTCErrorType::NONE, error);
+  configuration_ = configuration;
+  return true;
 }
 
 bool PeerConnection::AddIceCandidate(
@@ -1472,7 +1344,7 @@
   }
 
   // Send information about IPv4/IPv6 status.
-  if (uma_observer_) {
+  if (uma_observer_ && port_allocator_) {
     port_allocator_->SetMetricsObserver(uma_observer_);
     if (port_allocator_->flags() & cricket::PORTALLOCATOR_ENABLE_IPV6) {
       uma_observer_->IncrementEnumCounter(
@@ -2465,8 +2337,7 @@
     const RTCConfiguration& configuration) {
   cricket::ServerAddresses stun_servers;
   std::vector<cricket::RelayServerConfig> turn_servers;
-  if (ParseIceServers(configuration.servers, &stun_servers, &turn_servers) !=
-      RTCErrorType::NONE) {
+  if (!ParseIceServers(configuration.servers, &stun_servers, &turn_servers)) {
     return false;
   }
 
@@ -2512,17 +2383,19 @@
 }
 
 bool PeerConnection::ReconfigurePortAllocator_n(
-    const cricket::ServerAddresses& stun_servers,
-    const std::vector<cricket::RelayServerConfig>& turn_servers,
-    IceTransportsType type,
-    int candidate_pool_size,
-    bool prune_turn_ports) {
+    const RTCConfiguration& configuration) {
+  cricket::ServerAddresses stun_servers;
+  std::vector<cricket::RelayServerConfig> turn_servers;
+  if (!ParseIceServers(configuration.servers, &stun_servers, &turn_servers)) {
+    return false;
+  }
   port_allocator_->set_candidate_filter(
-      ConvertIceTransportTypeToCandidateFilter(type));
+      ConvertIceTransportTypeToCandidateFilter(configuration.type));
   // Call this last since it may create pooled allocator sessions using the
   // candidate filter set above.
   return port_allocator_->SetConfiguration(
-      stun_servers, turn_servers, candidate_pool_size, prune_turn_ports);
+      stun_servers, turn_servers, configuration.ice_candidate_pool_size,
+      configuration.prune_turn_ports);
 }
 
 bool PeerConnection::StartRtcEventLog_w(rtc::PlatformFile file,