Created PeerConnectionDependencies to avoid creating new CreatePeerConnection overloads.

Currently CreatePeerConnection takes 3 parameters. This is going to expand to four with a new change. Every time a new parameter is added, we need to deprecate the old method, switch clients to the new one, update fake/mock classes and tests, and so on, which is a cumbersome process.

To address this, this CL introduces a PeerConnectionDependencies structure to encapsulate all mandatory and optional dependencies (where a dependency is defined as non trivial
executable code that an API user may want to provide to the native API). This allows adding a new injectable dependency by simply adding a new field to the struct, avoiding the hassle described above.

You may think we could use RTCConfiguration for this. But since RTCConfiguration is both passed in and out of the PeerConnection (using GetConfiguration/SetConfiguration), it can't carry unique_ptrs. And we don't want to inject dependencies via raw pointers, since this could lead to lifetime management bugs, where the dependency is deleted before the PeerConnection is done using it.

Bug: webrtc:7913
Change-Id: I38c3f4ce4f26b37e6fe219d1eeca271294b40db8
Reviewed-on: https://webrtc-review.googlesource.com/73663
Commit-Queue: Benjamin Wright <benwright@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23084}

4 files changed, 71 insertions, 13 deletions

diff --git a/api/peerconnectionfactoryproxy.h b/api/peerconnectionfactoryproxy.h
index 7601ed144e..77778092b7 100644
--- a/api/peerconnectionfactoryproxy.h
+++ b/api/peerconnectionfactoryproxy.h
@@ -44,6 +44,10 @@ BEGIN_SIGNALING_PROXY_MAP(PeerConnectionFactory)
                 std::unique_ptr<cricket::PortAllocator>,
                 std::unique_ptr<rtc::RTCCertificateGeneratorInterface>,
                 PeerConnectionObserver*);
+  PROXY_METHOD2(rtc::scoped_refptr<PeerConnectionInterface>,
+                CreatePeerConnection,
+                const PeerConnectionInterface::RTCConfiguration&,
+                PeerConnectionDependencies);
   PROXY_METHOD1(rtc::scoped_refptr<MediaStreamInterface>,
                 CreateLocalMediaStream, const std::string&)
   PROXY_METHOD1(rtc::scoped_refptr<AudioSourceInterface>,
diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h
index 28129bf28c..6e39b4fb8e 100644
--- a/api/peerconnectioninterface.h
+++ b/api/peerconnectioninterface.h
@@ -1153,6 +1153,30 @@ class PeerConnectionObserver {
       rtc::scoped_refptr<RtpReceiverInterface> receiver) {}
 };
 
+// PeerConnectionDependencies holds all of PeerConnections dependencies.
+// A dependency is distinct from a configuration as it defines significant
+// executable code that can be provided by a user of the API.
+//
+// All new dependencies should be added as a unique_ptr to allow the
+// PeerConnection object to be the definitive owner of the dependencies
+// lifetime making injection safer.
+struct PeerConnectionDependencies final {
+  explicit PeerConnectionDependencies(PeerConnectionObserver* observer_in)
+      : observer(observer_in) {}
+  // This object is not copyable or assignable.
+  PeerConnectionDependencies(const PeerConnectionDependencies&) = delete;
+  PeerConnectionDependencies& operator=(const PeerConnectionDependencies&) =
+      delete;
+  // This object is only moveable.
+  PeerConnectionDependencies(PeerConnectionDependencies&&) = default;
+  PeerConnectionDependencies& operator=(PeerConnectionDependencies&&) = default;
+  // Mandatory dependencies
+  PeerConnectionObserver* observer = nullptr;
+  // Optional dependencies
+  std::unique_ptr<cricket::PortAllocator> allocator;
+  std::unique_ptr<rtc::RTCCertificateGeneratorInterface> cert_generator;
+};
+
 // PeerConnectionFactoryInterface is the factory interface used for creating
 // PeerConnection, MediaStream and MediaStreamTrack objects.
 //
@@ -1205,8 +1229,18 @@ class PeerConnectionFactoryInterface : public rtc::RefCountInterface {
   // Set the options to be used for subsequently created PeerConnections.
   virtual void SetOptions(const Options& options) = 0;
 
-  // |allocator| and |cert_generator| may be null, in which case default
-  // implementations will be used.
+  // The preferred way to create a new peer connection. Simply provide the
+  // configuration and a PeerConnectionDependencies structure.
+  // TODO(benwright): Make pure virtual once downstream mock PC factory classes
+  // are updated.
+  virtual rtc::scoped_refptr<PeerConnectionInterface> CreatePeerConnection(
+      const PeerConnectionInterface::RTCConfiguration& configuration,
+      PeerConnectionDependencies dependencies) {
+    return nullptr;
+  }
+
+  // Deprecated; |allocator| and |cert_generator| may be null, in which case
+  // default implementations will be used.
   //
   // |observer| must not be null.
   //
diff --git a/pc/peerconnectionfactory.cc b/pc/peerconnectionfactory.cc
index a7075efdd6..b6865ceff2 100644
--- a/pc/peerconnectionfactory.cc
+++ b/pc/peerconnectionfactory.cc
@@ -264,22 +264,35 @@ PeerConnectionFactory::CreatePeerConnection(
     std::unique_ptr<cricket::PortAllocator> allocator,
     std::unique_ptr<rtc::RTCCertificateGeneratorInterface> cert_generator,
     PeerConnectionObserver* observer) {
+  // Convert the legacy API into the new depnedency structure.
+  PeerConnectionDependencies dependencies(observer);
+  dependencies.allocator = std::move(allocator);
+  dependencies.cert_generator = std::move(cert_generator);
+  // Pass that into the new API.
+  return CreatePeerConnection(configuration, std::move(dependencies));
+}
+
+rtc::scoped_refptr<PeerConnectionInterface>
+PeerConnectionFactory::CreatePeerConnection(
+    const PeerConnectionInterface::RTCConfiguration& configuration,
+    PeerConnectionDependencies dependencies) {
   RTC_DCHECK(signaling_thread_->IsCurrent());
 
-  if (!cert_generator.get()) {
-    // No certificate generator specified, use the default one.
-    cert_generator.reset(
-        new rtc::RTCCertificateGenerator(signaling_thread_, network_thread_));
+  // Set internal defaults if optional dependencies are not set.
+  if (!dependencies.cert_generator) {
+    dependencies.cert_generator = rtc::MakeUnique<rtc::RTCCertificateGenerator>(
+        signaling_thread_, network_thread_);
   }
-
-  if (!allocator) {
-    allocator.reset(new cricket::BasicPortAllocator(
+  if (!dependencies.allocator) {
+    dependencies.allocator.reset(new cricket::BasicPortAllocator(
         default_network_manager_.get(), default_socket_factory_.get(),
         configuration.turn_customizer));
   }
+
   network_thread_->Invoke<void>(
-      RTC_FROM_HERE, rtc::Bind(&cricket::PortAllocator::SetNetworkIgnoreMask,
-                               allocator.get(), options_.network_ignore_mask));
+      RTC_FROM_HERE,
+      rtc::Bind(&cricket::PortAllocator::SetNetworkIgnoreMask,
+                dependencies.allocator.get(), options_.network_ignore_mask));
 
   std::unique_ptr<RtcEventLog> event_log =
       worker_thread_->Invoke<std::unique_ptr<RtcEventLog>>(
@@ -294,8 +307,11 @@ PeerConnectionFactory::CreatePeerConnection(
       new rtc::RefCountedObject<PeerConnection>(this, std::move(event_log),
                                                 std::move(call)));
 
-  if (!pc->Initialize(configuration, std::move(allocator),
-                      std::move(cert_generator), observer)) {
+  // TODO(benwright) Update initialize to take the entire dependencies
+  // structure.
+  if (!pc->Initialize(configuration, std::move(dependencies.allocator),
+                      std::move(dependencies.cert_generator),
+                      dependencies.observer)) {
     return nullptr;
   }
   return PeerConnectionProxy::Create(signaling_thread(), pc);
diff --git a/pc/peerconnectionfactory.h b/pc/peerconnectionfactory.h
index f58542f42e..6fb3c24f84 100644
--- a/pc/peerconnectionfactory.h
+++ b/pc/peerconnectionfactory.h
@@ -56,6 +56,10 @@ class PeerConnectionFactory : public PeerConnectionFactoryInterface {
       std::unique_ptr<rtc::RTCCertificateGeneratorInterface> cert_generator,
       PeerConnectionObserver* observer) override;
 
+  rtc::scoped_refptr<PeerConnectionInterface> CreatePeerConnection(
+      const PeerConnectionInterface::RTCConfiguration& configuration,
+      PeerConnectionDependencies dependencies) override;
+
   bool Initialize();
 
   rtc::scoped_refptr<MediaStreamInterface> CreateLocalMediaStream(