From 3756e29b150a13c89c4028cb7f4d4e82d70e971f Mon Sep 17 00:00:00 2001 From: Tommi Date: Fri, 1 Sep 2023 12:45:06 +0200 Subject: [PATCH] Remove another ctor from BasicPortAllocator This constructor isn't used in production. Removing it further made the construction state of the class simpler, allowed for removal of the separate Init() method and making more members const. Bug: none Change-Id: Ibc8516a01ce7e385207251d841d21bb7b72c9d9a Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/318281 Reviewed-by: Per Kjellander Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#40678} --- p2p/base/p2p_transport_channel_unittest.cc | 24 ++++++----- p2p/client/basic_port_allocator.cc | 47 +++++++-------------- p2p/client/basic_port_allocator.h | 20 +++------ p2p/client/basic_port_allocator_unittest.cc | 28 +++++------- pc/peer_connection_bundle_unittest.cc | 5 ++- pc/peer_connection_histogram_unittest.cc | 18 ++++---- pc/peer_connection_ice_unittest.cc | 5 ++- pc/peer_connection_rampup_tests.cc | 11 +++-- pc/test/integration_test_helpers.h | 8 ++-- 9 files changed, 70 insertions(+), 96 deletions(-) diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index e0b837ca5c..e414e3f558 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -170,7 +170,7 @@ cricket::Candidate CreateUdpCandidate(absl::string_view type, cricket::BasicPortAllocator* CreateBasicPortAllocator( rtc::NetworkManager* network_manager, - rtc::SocketServer* socket_server, + rtc::PacketSocketFactory* socket_factory, const cricket::ServerAddresses& stun_servers, const rtc::SocketAddress& turn_server_udp, const rtc::SocketAddress& turn_server_tcp) { @@ -187,9 +187,8 @@ cricket::BasicPortAllocator* CreateBasicPortAllocator( std::vector turn_servers(1, turn_server); std::unique_ptr allocator = - std::make_unique( - network_manager, - std::make_unique(socket_server)); + std::make_unique(network_manager, + socket_factory); allocator->Initialize(); allocator->SetConfiguration(stun_servers, turn_servers, 0, webrtc::NO_PRUNE); return allocator.release(); @@ -282,6 +281,7 @@ class P2PTransportChannelTestBase : public ::testing::Test, vss_(new rtc::VirtualSocketServer()), nss_(new rtc::NATSocketServer(vss_.get())), ss_(new rtc::FirewallSocketServer(nss_.get())), + socket_factory_(new rtc::BasicPacketSocketFactory(ss_.get())), main_(ss_.get()), stun_server_(TestStunServer::Create(ss_.get(), kStunAddr)), turn_server_(&main_, ss_.get(), kTurnUdpIntAddr, kTurnUdpExtAddr), @@ -300,11 +300,11 @@ class P2PTransportChannelTestBase : public ::testing::Test, ServerAddresses stun_servers; stun_servers.insert(kStunAddr); ep1_.allocator_.reset(CreateBasicPortAllocator( - &ep1_.network_manager_, ss_.get(), stun_servers, kTurnUdpIntAddr, - rtc::SocketAddress())); + &ep1_.network_manager_, socket_factory_.get(), stun_servers, + kTurnUdpIntAddr, rtc::SocketAddress())); ep2_.allocator_.reset(CreateBasicPortAllocator( - &ep2_.network_manager_, ss_.get(), stun_servers, kTurnUdpIntAddr, - rtc::SocketAddress())); + &ep2_.network_manager_, socket_factory_.get(), stun_servers, + kTurnUdpIntAddr, rtc::SocketAddress())); ep1_.SetIceTiebreaker(kTiebreakerDefault); ep1_.allocator_->SetIceTiebreaker(kTiebreakerDefault); @@ -1018,6 +1018,8 @@ class P2PTransportChannelTestBase : public ::testing::Test, std::unique_ptr vss_; std::unique_ptr nss_; std::unique_ptr ss_; + std::unique_ptr socket_factory_; + rtc::AutoSocketServerThread main_; rtc::scoped_refptr safety_ = PendingTaskSafetyFlag::Create(); @@ -5039,9 +5041,9 @@ class P2PTransportChannelMostLikelyToWorkFirstTest kTurnUdpIntAddr, kTurnUdpExtAddr) { network_manager_.AddInterface(kPublicAddrs[0]); - allocator_.reset( - CreateBasicPortAllocator(&network_manager_, ss(), ServerAddresses(), - kTurnUdpIntAddr, rtc::SocketAddress())); + allocator_.reset(CreateBasicPortAllocator( + &network_manager_, packet_socket_factory(), ServerAddresses(), + kTurnUdpIntAddr, rtc::SocketAddress())); allocator_->set_flags(allocator_->flags() | PORTALLOCATOR_DISABLE_STUN | PORTALLOCATOR_DISABLE_TCP); allocator_->set_step_delay(kMinimumStepDelay); diff --git a/p2p/client/basic_port_allocator.cc b/p2p/client/basic_port_allocator.cc index ccdd37dc41..b6cbf1fff9 100644 --- a/p2p/client/basic_port_allocator.cc +++ b/p2p/client/basic_port_allocator.cc @@ -169,28 +169,19 @@ BasicPortAllocator::BasicPortAllocator( const webrtc::FieldTrialsView* field_trials) : field_trials_(field_trials), network_manager_(network_manager), - socket_factory_(socket_factory) { - Init(relay_port_factory); - RTC_DCHECK(relay_port_factory_ != nullptr); - RTC_DCHECK(network_manager_ != nullptr); - RTC_CHECK(socket_factory_ != nullptr); + socket_factory_(socket_factory), + default_relay_port_factory_(relay_port_factory ? nullptr + : new TurnPortFactory()), + relay_port_factory_(relay_port_factory + ? relay_port_factory + : default_relay_port_factory_.get()) { + RTC_CHECK(socket_factory_); + RTC_DCHECK(relay_port_factory_); + RTC_DCHECK(network_manager_); SetConfiguration(ServerAddresses(), std::vector(), 0, webrtc::NO_PRUNE, customizer); } -BasicPortAllocator::BasicPortAllocator( - rtc::NetworkManager* network_manager, - std::unique_ptr owned_socket_factory, - const webrtc::FieldTrialsView* field_trials) - : field_trials_(field_trials), - network_manager_(network_manager), - socket_factory_(std::move(owned_socket_factory)) { - Init(nullptr); - RTC_DCHECK(relay_port_factory_ != nullptr); - RTC_DCHECK(network_manager_ != nullptr); - RTC_CHECK(socket_factory_ != nullptr); -} - BasicPortAllocator::BasicPortAllocator( rtc::NetworkManager* network_manager, rtc::PacketSocketFactory* socket_factory, @@ -198,11 +189,12 @@ BasicPortAllocator::BasicPortAllocator( const webrtc::FieldTrialsView* field_trials) : field_trials_(field_trials), network_manager_(network_manager), - socket_factory_(socket_factory) { - Init(nullptr); - RTC_DCHECK(relay_port_factory_ != nullptr); - RTC_DCHECK(network_manager_ != nullptr); - RTC_CHECK(socket_factory_ != nullptr); + socket_factory_(socket_factory), + default_relay_port_factory_(new TurnPortFactory()), + relay_port_factory_(default_relay_port_factory_.get()) { + RTC_CHECK(socket_factory_); + RTC_DCHECK(relay_port_factory_); + RTC_DCHECK(network_manager_); SetConfiguration(stun_servers, std::vector(), 0, webrtc::NO_PRUNE, nullptr); } @@ -276,15 +268,6 @@ void BasicPortAllocator::AddTurnServerForTesting( turn_port_prune_policy(), turn_customizer()); } -void BasicPortAllocator::Init(RelayPortFactoryInterface* relay_port_factory) { - if (relay_port_factory != nullptr) { - relay_port_factory_ = relay_port_factory; - } else { - default_relay_port_factory_.reset(new TurnPortFactory()); - relay_port_factory_ = default_relay_port_factory_.get(); - } -} - // BasicPortAllocatorSession BasicPortAllocatorSession::BasicPortAllocatorSession( BasicPortAllocator* allocator, diff --git a/p2p/client/basic_port_allocator.h b/p2p/client/basic_port_allocator.h index 7e30012e47..95bbdb183e 100644 --- a/p2p/client/basic_port_allocator.h +++ b/p2p/client/basic_port_allocator.h @@ -41,10 +41,6 @@ class RTC_EXPORT BasicPortAllocator : public PortAllocator { webrtc::TurnCustomizer* customizer = nullptr, RelayPortFactoryInterface* relay_port_factory = nullptr, const webrtc::FieldTrialsView* field_trials = nullptr); - BasicPortAllocator( - rtc::NetworkManager* network_manager, - std::unique_ptr owned_socket_factory, - const webrtc::FieldTrialsView* field_trials = nullptr); BasicPortAllocator(rtc::NetworkManager* network_manager, rtc::PacketSocketFactory* socket_factory, const ServerAddresses& stun_servers, @@ -64,7 +60,7 @@ class RTC_EXPORT BasicPortAllocator : public PortAllocator { // creates its own socket factory. rtc::PacketSocketFactory* socket_factory() { CheckRunOnValidThreadIfInitialized(); - return socket_factory_.get(); + return socket_factory_; } PortAllocatorSession* CreateSessionInternal( @@ -91,24 +87,20 @@ class RTC_EXPORT BasicPortAllocator : public PortAllocator { void OnIceRegathering(PortAllocatorSession* session, IceRegatheringReason reason); - // This function makes sure that relay_port_factory_ is set properly. - void Init(RelayPortFactoryInterface* relay_port_factory); - bool MdnsObfuscationEnabled() const override; webrtc::AlwaysValidPointer field_trials_; rtc::NetworkManager* network_manager_; - const webrtc::AlwaysValidPointerNoDefault - socket_factory_; + // Always externally-owned pointer to a socket factory. + rtc::PacketSocketFactory* const socket_factory_; int network_ignore_mask_ = rtc::kDefaultNetworkIgnoreMask; - // This is the factory being used. - RelayPortFactoryInterface* relay_port_factory_; - // This instance is created if caller does pass a factory. - std::unique_ptr default_relay_port_factory_; + const std::unique_ptr default_relay_port_factory_; + // This is the factory being used. + RelayPortFactoryInterface* const relay_port_factory_; }; struct PortConfiguration; diff --git a/p2p/client/basic_port_allocator_unittest.cc b/p2p/client/basic_port_allocator_unittest.cc index 7b2bba4293..55222a1be2 100644 --- a/p2p/client/basic_port_allocator_unittest.cc +++ b/p2p/client/basic_port_allocator_unittest.cc @@ -211,9 +211,8 @@ class BasicPortAllocatorTestBase : public ::testing::Test, } // Endpoint is on the public network. No STUN or TURN. void ResetWithNoServersOrNat() { - allocator_.reset(new BasicPortAllocator( - &network_manager_, - std::make_unique(fss_.get()))); + allocator_.reset( + new BasicPortAllocator(&network_manager_, &socket_factory_)); allocator_->Initialize(); allocator_->SetIceTiebreaker(kTiebreakerDefault); allocator_->set_step_delay(kMinimumStepDelay); @@ -608,9 +607,8 @@ class BasicPortAllocatorTest : public FakeClockBase, // Add two IP addresses on the same interface. AddInterface(kClientAddr, "net1"); AddInterface(kClientIPv6Addr, "net1"); - allocator_.reset(new BasicPortAllocator( - &network_manager_, - std::make_unique(fss_.get()))); + allocator_.reset( + new BasicPortAllocator(&network_manager_, &socket_factory_)); allocator_->Initialize(); allocator_->SetConfiguration(allocator_->stun_servers(), allocator_->turn_servers(), 0, @@ -651,9 +649,8 @@ class BasicPortAllocatorTest : public FakeClockBase, bool tcp_pruned) { turn_server_.AddInternalSocket(kTurnTcpIntAddr, PROTO_TCP); AddInterface(kClientAddr); - allocator_.reset(new BasicPortAllocator( - &network_manager_, - std::make_unique(fss_.get()))); + allocator_.reset( + new BasicPortAllocator(&network_manager_, &socket_factory_)); allocator_->Initialize(); allocator_->SetConfiguration(allocator_->stun_servers(), allocator_->turn_servers(), 0, prune_policy); @@ -705,9 +702,8 @@ class BasicPortAllocatorTest : public FakeClockBase, AddInterface(kClientIPv6Addr, "net1", rtc::ADAPTER_TYPE_WIFI); AddInterface(kClientAddr2, "net2", rtc::ADAPTER_TYPE_CELLULAR); AddInterface(kClientIPv6Addr2, "net2", rtc::ADAPTER_TYPE_CELLULAR); - allocator_.reset(new BasicPortAllocator( - &network_manager_, - std::make_unique(fss_.get()))); + allocator_.reset( + new BasicPortAllocator(&network_manager_, &socket_factory_)); allocator_->Initialize(); allocator_->SetConfiguration(allocator_->stun_servers(), allocator_->turn_servers(), 0, @@ -1673,9 +1669,7 @@ TEST_F(BasicPortAllocatorTest, TestSharedSocketWithNat) { TEST_F(BasicPortAllocatorTest, TestSharedSocketWithoutNatUsingTurn) { turn_server_.AddInternalSocket(kTurnTcpIntAddr, PROTO_TCP); AddInterface(kClientAddr); - allocator_.reset(new BasicPortAllocator( - &network_manager_, - std::make_unique(fss_.get()))); + allocator_.reset(new BasicPortAllocator(&network_manager_, &socket_factory_)); allocator_->Initialize(); AddTurnServers(kTurnUdpIntAddr, kTurnTcpIntAddr); @@ -1810,9 +1804,7 @@ TEST_F(BasicPortAllocatorTestWithRealClock, turn_server_.AddInternalSocket(rtc::SocketAddress("127.0.0.1", 3478), PROTO_UDP); AddInterface(kClientAddr); - allocator_.reset(new BasicPortAllocator( - &network_manager_, - std::make_unique(fss_.get()))); + allocator_.reset(new BasicPortAllocator(&network_manager_, &socket_factory_)); allocator_->Initialize(); RelayServerConfig turn_server; RelayCredentials credentials(kTurnUsername, kTurnPassword); diff --git a/pc/peer_connection_bundle_unittest.cc b/pc/peer_connection_bundle_unittest.cc index 715a8002a2..e5ef16ff8a 100644 --- a/pc/peer_connection_bundle_unittest.cc +++ b/pc/peer_connection_bundle_unittest.cc @@ -213,6 +213,7 @@ class PeerConnectionBundleBaseTest : public ::testing::Test { explicit PeerConnectionBundleBaseTest(SdpSemantics sdp_semantics) : vss_(new rtc::VirtualSocketServer()), + socket_factory_(new rtc::BasicPacketSocketFactory(vss_.get())), main_(vss_.get()), sdp_semantics_(sdp_semantics) { #ifdef WEBRTC_ANDROID @@ -238,8 +239,7 @@ class PeerConnectionBundleBaseTest : public ::testing::Test { WrapperPtr CreatePeerConnection(const RTCConfiguration& config) { auto* fake_network = NewFakeNetwork(); auto port_allocator = std::make_unique( - fake_network, - std::make_unique(vss_.get())); + fake_network, socket_factory_.get()); port_allocator->set_flags(cricket::PORTALLOCATOR_DISABLE_TCP | cricket::PORTALLOCATOR_DISABLE_RELAY); port_allocator->set_step_delay(cricket::kMinimumStepDelay); @@ -297,6 +297,7 @@ class PeerConnectionBundleBaseTest : public ::testing::Test { } std::unique_ptr vss_; + std::unique_ptr socket_factory_; rtc::AutoSocketServerThread main_; rtc::scoped_refptr pc_factory_; std::vector> fake_networks_; diff --git a/pc/peer_connection_histogram_unittest.cc b/pc/peer_connection_histogram_unittest.cc index 0efc1c5744..68a4dbc361 100644 --- a/pc/peer_connection_histogram_unittest.cc +++ b/pc/peer_connection_histogram_unittest.cc @@ -242,7 +242,9 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { WrapperPtr; PeerConnectionUsageHistogramTest() - : vss_(new rtc::VirtualSocketServer()), main_(vss_.get()) { + : vss_(new rtc::VirtualSocketServer()), + socket_factory_(new rtc::BasicPacketSocketFactory(vss_.get())), + main_(vss_.get()) { webrtc::metrics::Reset(); } @@ -270,9 +272,7 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { fake_network->AddInterface(NextLocalAddress()); std::unique_ptr port_allocator( - new cricket::BasicPortAllocator( - fake_network, - std::make_unique(vss_.get()))); + new cricket::BasicPortAllocator(fake_network, socket_factory_.get())); deps.async_dns_resolver_factory = std::move(resolver_factory); deps.allocator = std::move(port_allocator); @@ -295,8 +295,7 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { fake_network->AddInterface(kPrivateLocalAddress); auto port_allocator = std::make_unique( - fake_network, - std::make_unique(vss_.get())); + fake_network, socket_factory_.get()); RTCConfiguration config; config.sdp_semantics = SdpSemantics::kUnifiedPlan; return CreatePeerConnection(config, @@ -310,8 +309,7 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { fake_network->AddInterface(kPrivateIpv6LocalAddress); auto port_allocator = std::make_unique( - fake_network, - std::make_unique(vss_.get())); + fake_network, socket_factory_.get()); RTCConfiguration config; config.sdp_semantics = SdpSemantics::kUnifiedPlan; @@ -344,8 +342,7 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { auto fake_network = NewFakeNetwork(); fake_network->AddInterface(NextLocalAddress()); deps.allocator = std::make_unique( - fake_network, - std::make_unique(vss_.get())); + fake_network, socket_factory_.get()); } auto observer = std::make_unique(); @@ -388,6 +385,7 @@ class PeerConnectionUsageHistogramTest : public ::testing::Test { std::vector> fake_networks_; int next_local_address_ = 0; std::unique_ptr vss_; + std::unique_ptr socket_factory_; rtc::AutoSocketServerThread main_; }; diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc index 6e823c8934..532583f307 100644 --- a/pc/peer_connection_ice_unittest.cc +++ b/pc/peer_connection_ice_unittest.cc @@ -149,6 +149,7 @@ class PeerConnectionIceBaseTest : public ::testing::Test { explicit PeerConnectionIceBaseTest(SdpSemantics sdp_semantics) : vss_(new rtc::VirtualSocketServer()), + socket_factory_(new rtc::BasicPacketSocketFactory(vss_.get())), main_(vss_.get()), sdp_semantics_(sdp_semantics) { #ifdef WEBRTC_ANDROID @@ -174,8 +175,7 @@ class PeerConnectionIceBaseTest : public ::testing::Test { WrapperPtr CreatePeerConnection(const RTCConfiguration& config) { auto* fake_network = NewFakeNetwork(); auto port_allocator = std::make_unique( - fake_network, - std::make_unique(vss_.get())); + fake_network, socket_factory_.get()); port_allocator->set_flags(cricket::PORTALLOCATOR_DISABLE_TCP | cricket::PORTALLOCATOR_DISABLE_RELAY); port_allocator->set_step_delay(cricket::kMinimumStepDelay); @@ -330,6 +330,7 @@ class PeerConnectionIceBaseTest : public ::testing::Test { } std::unique_ptr vss_; + std::unique_ptr socket_factory_; rtc::AutoSocketServerThread main_; rtc::scoped_refptr pc_factory_; std::vector> fake_networks_; diff --git a/pc/peer_connection_rampup_tests.cc b/pc/peer_connection_rampup_tests.cc index 34b61b4e14..545a1d53d0 100644 --- a/pc/peer_connection_rampup_tests.cc +++ b/pc/peer_connection_rampup_tests.cc @@ -160,6 +160,8 @@ class PeerConnectionRampUpTest : public ::testing::Test { virtual_socket_server_(new rtc::VirtualSocketServer()), firewall_socket_server_( new rtc::FirewallSocketServer(virtual_socket_server_.get())), + firewall_socket_factory_( + new rtc::BasicPacketSocketFactory(firewall_socket_server_.get())), network_thread_(new rtc::Thread(firewall_socket_server_.get())), worker_thread_(rtc::Thread::Create()) { network_thread_->SetName("PCNetworkThread", this); @@ -201,10 +203,9 @@ class PeerConnectionRampUpTest : public ::testing::Test { auto observer = std::make_unique(); webrtc::PeerConnectionDependencies dependencies(observer.get()); cricket::BasicPortAllocator* port_allocator = - new cricket::BasicPortAllocator( - fake_network_manager, - std::make_unique( - firewall_socket_server_.get())); + new cricket::BasicPortAllocator(fake_network_manager, + firewall_socket_factory_.get()); + port_allocator->set_step_delay(cricket::kDefaultStepDelay); dependencies.allocator = std::unique_ptr(port_allocator); @@ -344,6 +345,8 @@ class PeerConnectionRampUpTest : public ::testing::Test { // up queue. std::unique_ptr virtual_socket_server_; std::unique_ptr firewall_socket_server_; + std::unique_ptr firewall_socket_factory_; + std::unique_ptr network_thread_; std::unique_ptr worker_thread_; // The `pc_factory` uses `network_thread_` & `worker_thread_`, so it must be diff --git a/pc/test/integration_test_helpers.h b/pc/test/integration_test_helpers.h index c450ccba26..43033d2a5c 100644 --- a/pc/test/integration_test_helpers.h +++ b/pc/test/integration_test_helpers.h @@ -757,10 +757,11 @@ class PeerConnectionIntegrationWrapper : public webrtc::PeerConnectionObserver, fake_network_manager_.reset(new rtc::FakeNetworkManager()); fake_network_manager_->AddInterface(kDefaultLocalAddress); + socket_factory_.reset(new rtc::BasicPacketSocketFactory(socket_server)); + std::unique_ptr port_allocator( - new cricket::BasicPortAllocator( - fake_network_manager_.get(), - std::make_unique(socket_server))); + new cricket::BasicPortAllocator(fake_network_manager_.get(), + socket_factory_.get())); port_allocator_ = port_allocator.get(); fake_audio_capture_module_ = FakeAudioCaptureModule::Create(); if (!fake_audio_capture_module_) { @@ -1173,6 +1174,7 @@ class PeerConnectionIntegrationWrapper : public webrtc::PeerConnectionObserver, std::string debug_name_; std::unique_ptr fake_network_manager_; + std::unique_ptr socket_factory_; // Reference to the mDNS responder owned by `fake_network_manager_` after set. webrtc::FakeMdnsResponder* mdns_responder_ = nullptr;