Removing webrtc::PortAllocatorFactoryInterface.

talk/app/webrtc/peerconnectionfactory.cc

Index: talk/app/webrtc/peerconnectionfactory.cc
diff --git a/talk/app/webrtc/peerconnectionfactory.cc b/talk/app/webrtc/peerconnectionfactory.cc
index 672355085a6b775e57264dd92a241f64295260e3..c66bffa873b923b554e698d3ae405d4bab2a9a42 100644
--- a/talk/app/webrtc/peerconnectionfactory.cc
+++ b/talk/app/webrtc/peerconnectionfactory.cc
@@ -35,7 +35,6 @@
 #include "talk/app/webrtc/peerconnection.h"
 #include "talk/app/webrtc/peerconnectionfactoryproxy.h"
 #include "talk/app/webrtc/peerconnectionproxy.h"
-#include "talk/app/webrtc/portallocatorfactory.h"
 #include "talk/app/webrtc/videosource.h"
 #include "talk/app/webrtc/videosourceproxy.h"
 #include "talk/app/webrtc/videotrack.h"
@@ -155,7 +154,6 @@ PeerConnectionFactory::PeerConnectionFactory(
 PeerConnectionFactory::~PeerConnectionFactory() {
   RTC_DCHECK(signaling_thread_->IsCurrent());
   channel_manager_.reset(nullptr);
-  default_allocator_factory_ = nullptr;
 
   // Make sure |worker_thread_| and |signaling_thread_| outlive
   // |dtls_identity_store_|, |default_socket_factory_| and
@@ -175,11 +173,6 @@ bool PeerConnectionFactory::Initialize() {
   RTC_DCHECK(signaling_thread_->IsCurrent());
   rtc::InitRandom(rtc::Time());
 
-  default_allocator_factory_ = PortAllocatorFactory::Create(worker_thread_);
-  if (!default_allocator_factory_) {
-    return false;
-  }
-
   default_network_manager_.reset(new rtc::BasicNetworkManager());
   if (!default_network_manager_) {
     return false;
@@ -254,41 +247,6 @@ rtc::scoped_refptr<PeerConnectionInterface>
 PeerConnectionFactory::CreatePeerConnection(
     const PeerConnectionInterface::RTCConfiguration& configuration,
     const MediaConstraintsInterface* constraints,
-    PortAllocatorFactoryInterface* allocator_factory,
-    rtc::scoped_ptr<DtlsIdentityStoreInterface> dtls_identity_store,
-    PeerConnectionObserver* observer) {
-  RTC_DCHECK(signaling_thread_->IsCurrent());
-  RTC_DCHECK(allocator_factory || default_allocator_factory_);
-
-  if (!dtls_identity_store.get()) {
-    // Because |pc|->Initialize takes ownership of the store we need a new
-    // wrapper object that can be deleted without deleting the underlying
-    // |dtls_identity_store_|, protecting it from being deleted multiple times.
-    dtls_identity_store.reset(
-        new DtlsIdentityStoreWrapper(dtls_identity_store_));
-  }
-
-  PortAllocatorFactoryInterface* chosen_allocator_factory =
-      allocator_factory ? allocator_factory : default_allocator_factory_.get();
-  chosen_allocator_factory->SetNetworkIgnoreMask(options_.network_ignore_mask);
-
-  rtc::scoped_refptr<PeerConnection> pc(
-      new rtc::RefCountedObject<PeerConnection>(this));
-  if (!pc->Initialize(
-      configuration,
-      constraints,
-      chosen_allocator_factory,
-      dtls_identity_store.Pass(),
-      observer)) {
-    return NULL;
-  }
-  return PeerConnectionProxy::Create(signaling_thread(), pc);
-}
-
-rtc::scoped_refptr<PeerConnectionInterface>
-PeerConnectionFactory::CreatePeerConnection(
-    const PeerConnectionInterface::RTCConfiguration& configuration,
-    const MediaConstraintsInterface* constraints,
     rtc::scoped_ptr<cricket::PortAllocator> allocator,
     rtc::scoped_ptr<DtlsIdentityStoreInterface> dtls_identity_store,
     PeerConnectionObserver* observer) {
@@ -306,8 +264,7 @@ PeerConnectionFactory::CreatePeerConnection(
     allocator.reset(new cricket::BasicPortAllocator(
         default_network_manager_.get(), default_socket_factory_.get()));
   }
-  default_network_manager_->set_network_ignore_mask(
-      options_.network_ignore_mask);
+  allocator->SetNetworkIgnoreMask(options_.network_ignore_mask);

[pthatcher1, 2015/12/15 07:58:44]

So is this is really just a way to set the network ignore mask of the underlying
network manager, which will then be used to filter out networks when used by the
allocator in BasicPortAllocatorSession::DoAllocate amd
BasicPortAllocatorSession::OnNetworksChanged()?

If so, I have a few suggestions:

1.  Would it make sense to just skip setting the network mask in the
NetworkManager and instead apply filtering in the PortAllocator?  We could do
inside of DoAllocate and OnNetworksChanged quite easily.

2.  If that's infeasible, would it make sense to add a network_manager() getter
on the allocator, so the above line of code would be
allocator->newtork_manager()->SetNetworkIgnoreMask(...).  That would perhaps be
cleaner and make it more clear what the scope of the change is.

3.  If neither of those are feasible, then could we at least document what is
really going on here at this point (that the NetworkManager of the PortAllocator
is being modified, not really the PortAllocator)?

[phoglund, 2015/12/15 12:41:31]

Yeah. When I made the network mask change, I had to go through the port
allocator to get the information in options_ to the network manager. I don't
fully understand the role of the port allocator: does it make sense to tell it
to apply a ignore mask? If it does, 1) sounds great. The network manager already
classifies networks as loopback or not, so one could move all whole ignore logic
to the port allocator and make it do the filtering. Just watch out if there are
other users of the network manager, that doesn't go through the port allocator.

Also, please be careful so that the mask is set before the networks are
enumerated. Otherwise the list of networks will be incorrect.

 
   rtc::scoped_refptr<PeerConnection> pc(
       new rtc::RefCountedObject<PeerConnection>(this));