Make PortInterface signals protected This flushes out downstream usage of these objects. "Protected" is used because the Subscribe and Notify functions are defined on Port, not PortInterface. Bug: webrtc:42222066 Change-Id: Icb6ec4e723d97f82b6eaee4221a05209834d4e86 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/421800 Reviewed-by: Danil Chapovalov <danilchap@webrtc.org> Commit-Queue: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#46125}
diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn index 6a7457d..4f4cf32 100644 --- a/p2p/BUILD.gn +++ b/p2p/BUILD.gn
@@ -674,6 +674,7 @@ "../rtc_base:dscp", "../rtc_base:net_helper", "../rtc_base:network", + "../rtc_base:sigslot_trampoline", "../rtc_base:socket", "../rtc_base:socket_address", "../rtc_base/network:sent_packet",
diff --git a/p2p/base/port.cc b/p2p/base/port.cc index 1b2d53c..9f7c06f 100644 --- a/p2p/base/port.cc +++ b/p2p/base/port.cc
@@ -109,9 +109,6 @@ shared_socket_(shared_socket), network_cost_(args.network->GetCost(env_.field_trials())), role_conflict_callback_(nullptr), - unknown_address_trampoline_(this), - read_packet_trampoline_(this), - sent_packet_trampoline_(this), weak_factory_(this) { RTC_DCHECK_RUN_ON(thread_); RTC_DCHECK(factory_ != nullptr);
diff --git a/p2p/base/port.h b/p2p/base/port.h index 0ab339e..5a00aa0 100644 --- a/p2p/base/port.h +++ b/p2p/base/port.h
@@ -43,7 +43,6 @@ #include "rtc_base/network.h" #include "rtc_base/network/received_packet.h" #include "rtc_base/network/sent_packet.h" -#include "rtc_base/sigslot_trampoline.h" #include "rtc_base/socket_address.h" #include "rtc_base/system/rtc_export.h" #include "rtc_base/third_party/sigslot/sigslot.h" @@ -585,10 +584,6 @@ // Signals and trampolines. These will eventually be removed and replaced // with straight CallbackLists (or simple callbacks). // TODO: https://issues.webrtc.org/42222066 - replace and delete. - - // Downstream code uses this signal. We will continue firing it along with the - // callback list. The signal can be deleted once all downstream usages are - // replaced with the new CallbackList implementation. sigslot::signal2<Port*, const Candidate&> SignalCandidateReady; sigslot::signal1<Port*> SignalPortComplete; // Downstream code uses this signal. We will continue firing it along with the @@ -596,13 +591,6 @@ // replaced with the new CallbackList implementation. sigslot::signal1<Port*> SignalPortError; - SignalTrampoline<PortInterface, &PortInterface::SignalUnknownAddress> - unknown_address_trampoline_; - SignalTrampoline<PortInterface, &PortInterface::SignalReadPacket> - read_packet_trampoline_; - SignalTrampoline<PortInterface, &PortInterface::SignalSentPacket> - sent_packet_trampoline_; - // Keep as the last member variable. WeakPtrFactory<Port> weak_factory_ RTC_GUARDED_BY(thread_); };
diff --git a/p2p/base/port_interface.cc b/p2p/base/port_interface.cc index b123fd2..d442ad4 100644 --- a/p2p/base/port_interface.cc +++ b/p2p/base/port_interface.cc
@@ -14,7 +14,10 @@ namespace webrtc { -PortInterface::PortInterface() = default; +PortInterface::PortInterface() + : unknown_address_trampoline_(this), + read_packet_trampoline_(this), + sent_packet_trampoline_(this) {} PortInterface::~PortInterface() = default;
diff --git a/p2p/base/port_interface.h b/p2p/base/port_interface.h index 55b6650..15141d4 100644 --- a/p2p/base/port_interface.h +++ b/p2p/base/port_interface.h
@@ -30,6 +30,7 @@ #include "rtc_base/net_helper.h" #include "rtc_base/network.h" #include "rtc_base/network/sent_packet.h" +#include "rtc_base/sigslot_trampoline.h" #include "rtc_base/socket.h" #include "rtc_base/socket_address.h" #include "rtc_base/third_party/sigslot/sigslot.h" @@ -97,13 +98,6 @@ // Indicates that we received a successful STUN binding request from an // address that doesn't correspond to any current connection. To turn this // into a real connection, call CreateConnection. - sigslot::signal6<PortInterface*, - const SocketAddress&, - ProtocolType, - IceMessage*, - const std::string&, - bool> - SignalUnknownAddress; virtual void SubscribeUnknownAddress( absl::AnyInvocable<void(PortInterface*, const SocketAddress&, @@ -131,8 +125,6 @@ std::function<void(PortInterface*)> callback) = 0; // Signaled when Port discovers ice role conflict with the peer. - // TODO: bugs.webrtc.org/42222066 - remove slot. - sigslot::signal1<PortInterface*> SignalRoleConflict; virtual void SubscribeRoleConflict(absl::AnyInvocable<void()> callback) = 0; virtual void NotifyRoleConflict() = 0; @@ -141,8 +133,6 @@ // through their respective connection and instead delivers every packet // through this port. virtual void EnablePortPackets() = 0; - sigslot::signal4<PortInterface*, const char*, size_t, const SocketAddress&> - SignalReadPacket; virtual void SubscribeReadPacket( absl::AnyInvocable< void(PortInterface*, const char*, size_t, const SocketAddress&)> @@ -153,7 +143,6 @@ const SocketAddress&) = 0; // Emitted each time a packet is sent on this port. - sigslot::signal1<const SentPacketInfo&> SignalSentPacket; virtual void SubscribeSentPacket( absl::AnyInvocable<void(const SentPacketInfo&)> callback) = 0; virtual void NotifySentPacket(const SentPacketInfo& packet) = 0; @@ -218,6 +207,26 @@ absl::string_view remote_ufrag) = 0; virtual int16_t network_cost() const = 0; + // Since the Subscribe and Notify methods are defined on subclasses' + // interfaces, these signals need to be "protected", not "private". + // TODO: https://issues.webrtc.org/42222066 - replace and delete. + sigslot::signal6<PortInterface*, + const SocketAddress&, + ProtocolType, + IceMessage*, + const std::string&, + bool> + SignalUnknownAddress; + sigslot::signal4<PortInterface*, const char*, size_t, const SocketAddress&> + SignalReadPacket; + sigslot::signal1<const SentPacketInfo&> SignalSentPacket; + SignalTrampoline<PortInterface, &PortInterface::SignalUnknownAddress> + unknown_address_trampoline_; + SignalTrampoline<PortInterface, &PortInterface::SignalReadPacket> + read_packet_trampoline_; + SignalTrampoline<PortInterface, &PortInterface::SignalSentPacket> + sent_packet_trampoline_; + sigslot::signal1<PortInterface*> SignalRoleConflict; // Connection and Port are entangled; functions exposed to Port only // should not be public.
diff --git a/p2p/base/port_unittest.cc b/p2p/base/port_unittest.cc index 341379f..3f1cb15 100644 --- a/p2p/base/port_unittest.cc +++ b/p2p/base/port_unittest.cc
@@ -3992,32 +3992,6 @@ EXPECT_TRUE(port->GetConnection(address) != nullptr); } -#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) -class DeathChannel : public sigslot::has_slots<> { - public: - explicit DeathChannel(std::unique_ptr<Port> port) : port_(std::move(port)) {} - void IgnoredSlot(PortInterface* /* port */) {} - void AddSignal() { - port_->SignalRoleConflict.connect(this, &DeathChannel::IgnoredSlot); - } - void AddCallback() { - port_->SubscribeRoleConflict([]() {}); - } - - private: - std::unique_ptr<Port> port_; -}; - -class PortDeathTest : public PortTest {}; - -TEST_F(PortDeathTest, AddSignalThenCallback) { - DeathChannel dc(CreateRawTestPort()); - dc.AddSignal(); - EXPECT_DEATH(dc.AddCallback(), ""); -} - -#endif // RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) - // TODO(webrtc:11463) : Move Connection tests into separate unit test // splitting out shared test code as needed.
diff --git a/p2p/base/tcp_port_unittest.cc b/p2p/base/tcp_port_unittest.cc index 1b8136e..e98f685 100644 --- a/p2p/base/tcp_port_unittest.cc +++ b/p2p/base/tcp_port_unittest.cc
@@ -249,7 +249,8 @@ class SentPacketCounter : public sigslot::has_slots<> { public: explicit SentPacketCounter(TCPPort* p) { - p->SignalSentPacket.connect(this, &SentPacketCounter::OnSentPacket); + p->SubscribeSentPacket( + [this](const webrtc::SentPacketInfo& info) { OnSentPacket(info); }); } int sent_packets() const { return sent_packets_; }