From 74db777d64d8d15b043c102bd5309755cf2106c2 Mon Sep 17 00:00:00 2001 From: guidou Date: Thu, 18 Feb 2016 01:57:49 -0800 Subject: [PATCH] Revert of Remove GetTransport() from TransportChannelImpl (patchset #3 id:40001 of https://codereview.webrtc.org/1691673002/ ) Reason for revert: This CL is breaking a lot of FYI bots. The specific change that breaks bots is the removal of a constructor parameter. See, for example: https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win%20Builder/builds/3572/steps/compile/logs/stdio Original issue's description: > Remove GetTransport() from TransportChannelImpl > > This appears to be dead code because GetTransport() is not used by WebRTC. It also adds dead code to DtlsTransportChannelWrapper and P2PTransportChannel. > > BUG= > > Committed: https://crrev.com/ee18220ddd783fad9812f1c1c195bf187a631c3a > Cr-Commit-Position: refs/heads/master@{#11662} TBR=pthatcher@webrtc.org,mikescarlett@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= Review URL: https://codereview.webrtc.org/1709953002 Cr-Commit-Position: refs/heads/master@{#11665} --- webrtc/p2p/base/dtlstransport.h | 2 +- webrtc/p2p/base/dtlstransportchannel.cc | 2 ++ webrtc/p2p/base/dtlstransportchannel.h | 9 ++++-- webrtc/p2p/base/faketransportcontroller.h | 9 ++++-- webrtc/p2p/base/p2ptransport.cc | 2 +- webrtc/p2p/base/p2ptransportchannel.cc | 2 ++ webrtc/p2p/base/p2ptransportchannel.h | 4 +++ .../p2p/base/p2ptransportchannel_unittest.cc | 28 +++++++++---------- webrtc/p2p/base/transportchannelimpl.h | 4 +++ webrtc/p2p/quic/quicsession_unittest.cc | 4 +-- 10 files changed, 44 insertions(+), 22 deletions(-) diff --git a/webrtc/p2p/base/dtlstransport.h b/webrtc/p2p/base/dtlstransport.h index 276b05f786..9f2903e1d7 100644 --- a/webrtc/p2p/base/dtlstransport.h +++ b/webrtc/p2p/base/dtlstransport.h @@ -199,7 +199,7 @@ class DtlsTransport : public Base { DtlsTransportChannelWrapper* CreateTransportChannel(int component) override { DtlsTransportChannelWrapper* channel = new DtlsTransportChannelWrapper( - Base::CreateTransportChannel(component)); + this, Base::CreateTransportChannel(component)); channel->SetSslMaxProtocolVersion(ssl_max_version_); return channel; } diff --git a/webrtc/p2p/base/dtlstransportchannel.cc b/webrtc/p2p/base/dtlstransportchannel.cc index 59ef2bc41e..d6b5bce723 100644 --- a/webrtc/p2p/base/dtlstransportchannel.cc +++ b/webrtc/p2p/base/dtlstransportchannel.cc @@ -89,8 +89,10 @@ bool StreamInterfaceChannel::OnPacketReceived(const char* data, size_t size) { } DtlsTransportChannelWrapper::DtlsTransportChannelWrapper( + Transport* transport, TransportChannelImpl* channel) : TransportChannelImpl(channel->transport_name(), channel->component()), + transport_(transport), worker_thread_(rtc::Thread::Current()), channel_(channel), downward_(NULL), diff --git a/webrtc/p2p/base/dtlstransportchannel.h b/webrtc/p2p/base/dtlstransportchannel.h index f396a57d30..955b963a36 100644 --- a/webrtc/p2p/base/dtlstransportchannel.h +++ b/webrtc/p2p/base/dtlstransportchannel.h @@ -82,8 +82,10 @@ class StreamInterfaceChannel : public rtc::StreamInterface { class DtlsTransportChannelWrapper : public TransportChannelImpl { public: // The parameters here are: + // transport -- the DtlsTransport that created us // channel -- the TransportChannel we are wrapping - explicit DtlsTransportChannelWrapper(TransportChannelImpl* channel); + DtlsTransportChannelWrapper(Transport* transport, + TransportChannelImpl* channel); ~DtlsTransportChannelWrapper() override; void SetIceRole(IceRole role) override { channel_->SetIceRole(role); } @@ -157,6 +159,8 @@ class DtlsTransportChannelWrapper : public TransportChannelImpl { } // TransportChannelImpl calls. + Transport* GetTransport() override { return transport_; } + TransportChannelState GetState() const override { return channel_->GetState(); } @@ -214,8 +218,9 @@ class DtlsTransportChannelWrapper : public TransportChannelImpl { void OnConnectionRemoved(TransportChannelImpl* channel); void Reconnect(); + Transport* transport_; // The transport_ that created us. rtc::Thread* worker_thread_; // Everything should occur on this thread. - // Underlying channel, not owned by this class. + // Underlying channel, owned by transport_. TransportChannelImpl* const channel_; rtc::scoped_ptr dtls_; // The DTLS stream StreamInterfaceChannel* downward_; // Wrapper for channel_, owned by dtls_. diff --git a/webrtc/p2p/base/faketransportcontroller.h b/webrtc/p2p/base/faketransportcontroller.h index 82f4ebdc7b..65c59be98d 100644 --- a/webrtc/p2p/base/faketransportcontroller.h +++ b/webrtc/p2p/base/faketransportcontroller.h @@ -45,9 +45,11 @@ struct PacketMessageData : public rtc::MessageData { class FakeTransportChannel : public TransportChannelImpl, public rtc::MessageHandler { public: - explicit FakeTransportChannel(const std::string& name, + explicit FakeTransportChannel(Transport* transport, + const std::string& name, int component) : TransportChannelImpl(name, component), + transport_(transport), dtls_fingerprint_("", nullptr, 0) {} ~FakeTransportChannel() { Reset(); } @@ -65,6 +67,8 @@ class FakeTransportChannel : public TransportChannelImpl, // synchronously "Send"-ing. void SetAsync(bool async) { async_ = async; } + Transport* GetTransport() override { return transport_; } + TransportChannelState GetState() const override { if (connection_count_ == 0) { return had_connection_ ? TransportChannelState::STATE_FAILED @@ -309,6 +313,7 @@ class FakeTransportChannel : public TransportChannelImpl, private: enum State { STATE_INIT, STATE_CONNECTING, STATE_CONNECTED }; + Transport* transport_; FakeTransportChannel* dest_ = nullptr; State state_ = STATE_INIT; bool async_ = false; @@ -410,7 +415,7 @@ class FakeTransport : public Transport { return nullptr; } FakeTransportChannel* channel = - new FakeTransportChannel(name(), component); + new FakeTransportChannel(this, name(), component); channel->set_ssl_max_protocol_version(ssl_max_version_); channel->SetAsync(async_); SetChannelDestination(component, channel); diff --git a/webrtc/p2p/base/p2ptransport.cc b/webrtc/p2p/base/p2ptransport.cc index 1ad2a6faa3..abc4c14504 100644 --- a/webrtc/p2p/base/p2ptransport.cc +++ b/webrtc/p2p/base/p2ptransport.cc @@ -28,7 +28,7 @@ P2PTransport::~P2PTransport() { } TransportChannelImpl* P2PTransport::CreateTransportChannel(int component) { - return new P2PTransportChannel(name(), component, port_allocator()); + return new P2PTransportChannel(name(), component, this, port_allocator()); } void P2PTransport::DestroyTransportChannel(TransportChannelImpl* channel) { diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 15d8f3d1bb..74f1392b1b 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -217,8 +217,10 @@ static const int MIN_CHECK_RECEIVING_DELAY = 50; // ms P2PTransportChannel::P2PTransportChannel(const std::string& transport_name, int component, + P2PTransport* transport, PortAllocator* allocator) : TransportChannelImpl(transport_name, component), + transport_(transport), allocator_(allocator), worker_thread_(rtc::Thread::Current()), incoming_only_(false), diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index 8e36144e97..fde00263d0 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -27,6 +27,7 @@ #include "webrtc/p2p/base/p2ptransport.h" #include "webrtc/p2p/base/portallocator.h" #include "webrtc/p2p/base/portinterface.h" +#include "webrtc/p2p/base/transport.h" #include "webrtc/p2p/base/transportchannelimpl.h" #include "webrtc/base/asyncpacketsocket.h" #include "webrtc/base/sigslot.h" @@ -66,10 +67,12 @@ class P2PTransportChannel : public TransportChannelImpl, public: P2PTransportChannel(const std::string& transport_name, int component, + P2PTransport* transport, PortAllocator* allocator); virtual ~P2PTransportChannel(); // From TransportChannelImpl: + Transport* GetTransport() override { return transport_; } TransportChannelState GetState() const override; void SetIceRole(IceRole role) override; IceRole GetIceRole() const override { return ice_role_; } @@ -262,6 +265,7 @@ class P2PTransportChannel : public TransportChannelImpl, : static_cast(remote_ice_parameters_.size() - 1); } + P2PTransport* transport_; PortAllocator* allocator_; rtc::Thread* worker_thread_; bool incoming_only_; diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index 74b96a3532..38f12fa7ff 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -306,7 +306,7 @@ class P2PTransportChannelTestBase : public testing::Test, const std::string& remote_ice_ufrag, const std::string& remote_ice_pwd) { cricket::P2PTransportChannel* channel = new cricket::P2PTransportChannel( - "test content name", component, GetAllocator(endpoint)); + "test content name", component, NULL, GetAllocator(endpoint)); channel->SignalCandidateGathered.connect( this, &P2PTransportChannelTestBase::OnCandidate); channel->SignalReadPacket.connect( @@ -1910,7 +1910,7 @@ class P2PTransportChannelPingTest : public testing::Test, TEST_F(P2PTransportChannelPingTest, TestTriggeredChecks) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); - cricket::P2PTransportChannel ch("trigger checks", 1, &pa); + cricket::P2PTransportChannel ch("trigger checks", 1, nullptr, &pa); PrepareChannel(&ch); ch.Connect(); ch.MaybeStartGathering(); @@ -1935,7 +1935,7 @@ TEST_F(P2PTransportChannelPingTest, TestTriggeredChecks) { TEST_F(P2PTransportChannelPingTest, TestNoTriggeredChecksWhenWritable) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); - cricket::P2PTransportChannel ch("trigger checks", 1, &pa); + cricket::P2PTransportChannel ch("trigger checks", 1, nullptr, &pa); PrepareChannel(&ch); ch.Connect(); ch.MaybeStartGathering(); @@ -1966,7 +1966,7 @@ TEST_F(P2PTransportChannelPingTest, TestNoTriggeredChecksWhenWritable) { // ufrag, its pwd and generation will be set properly. TEST_F(P2PTransportChannelPingTest, TestAddRemoteCandidateWithVariousUfrags) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); - cricket::P2PTransportChannel ch("add candidate", 1, &pa); + cricket::P2PTransportChannel ch("add candidate", 1, nullptr, &pa); PrepareChannel(&ch); ch.Connect(); ch.MaybeStartGathering(); @@ -2016,7 +2016,7 @@ TEST_F(P2PTransportChannelPingTest, TestAddRemoteCandidateWithVariousUfrags) { TEST_F(P2PTransportChannelPingTest, ConnectionResurrection) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); - cricket::P2PTransportChannel ch("connection resurrection", 1, &pa); + cricket::P2PTransportChannel ch("connection resurrection", 1, nullptr, &pa); PrepareChannel(&ch); ch.Connect(); ch.MaybeStartGathering(); @@ -2069,7 +2069,7 @@ TEST_F(P2PTransportChannelPingTest, ConnectionResurrection) { TEST_F(P2PTransportChannelPingTest, TestReceivingStateChange) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); - cricket::P2PTransportChannel ch("receiving state change", 1, &pa); + cricket::P2PTransportChannel ch("receiving state change", 1, nullptr, &pa); PrepareChannel(&ch); // Default receiving timeout and checking receiving delay should not be too // small. @@ -2097,7 +2097,7 @@ TEST_F(P2PTransportChannelPingTest, TestReceivingStateChange) { // "best connection". TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); - cricket::P2PTransportChannel ch("receiving state change", 1, &pa); + cricket::P2PTransportChannel ch("receiving state change", 1, nullptr, &pa); PrepareChannel(&ch); ch.SetIceRole(cricket::ICEROLE_CONTROLLED); ch.Connect(); @@ -2154,7 +2154,7 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { // a ping response and set the ICE pwd in the remote candidate appropriately. TEST_F(P2PTransportChannelPingTest, TestSelectConnectionFromUnknownAddress) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); - cricket::P2PTransportChannel ch("receiving state change", 1, &pa); + cricket::P2PTransportChannel ch("receiving state change", 1, nullptr, &pa); PrepareChannel(&ch); ch.SetIceRole(cricket::ICEROLE_CONTROLLED); ch.Connect(); @@ -2236,7 +2236,7 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionFromUnknownAddress) { // the "best connection". TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBasedOnMediaReceived) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); - cricket::P2PTransportChannel ch("receiving state change", 1, &pa); + cricket::P2PTransportChannel ch("receiving state change", 1, nullptr, &pa); PrepareChannel(&ch); ch.SetIceRole(cricket::ICEROLE_CONTROLLED); ch.Connect(); @@ -2294,7 +2294,7 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBasedOnMediaReceived) { // be pruned. Otherwise, lower-priority connections are kept. TEST_F(P2PTransportChannelPingTest, TestDontPruneWhenWeak) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); - cricket::P2PTransportChannel ch("test channel", 1, &pa); + cricket::P2PTransportChannel ch("test channel", 1, nullptr, &pa); PrepareChannel(&ch); ch.SetIceRole(cricket::ICEROLE_CONTROLLED); ch.Connect(); @@ -2332,7 +2332,7 @@ TEST_F(P2PTransportChannelPingTest, TestDontPruneWhenWeak) { // Test that GetState returns the state correctly. TEST_F(P2PTransportChannelPingTest, TestGetState) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); - cricket::P2PTransportChannel ch("test channel", 1, &pa); + cricket::P2PTransportChannel ch("test channel", 1, nullptr, &pa); PrepareChannel(&ch); ch.Connect(); ch.MaybeStartGathering(); @@ -2359,7 +2359,7 @@ TEST_F(P2PTransportChannelPingTest, TestGetState) { // right away, and it can become active and be pruned again. TEST_F(P2PTransportChannelPingTest, TestConnectionPrunedAgain) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); - cricket::P2PTransportChannel ch("test channel", 1, &pa); + cricket::P2PTransportChannel ch("test channel", 1, nullptr, &pa); PrepareChannel(&ch); ch.SetIceConfig(CreateIceConfig(1000, false)); ch.Connect(); @@ -2402,7 +2402,7 @@ TEST_F(P2PTransportChannelPingTest, TestConnectionPrunedAgain) { // will all be deleted. We use Prune to simulate write_time_out. TEST_F(P2PTransportChannelPingTest, TestDeleteConnectionsIfAllWriteTimedout) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); - cricket::P2PTransportChannel ch("test channel", 1, &pa); + cricket::P2PTransportChannel ch("test channel", 1, nullptr, &pa); PrepareChannel(&ch); ch.Connect(); ch.MaybeStartGathering(); @@ -2434,7 +2434,7 @@ TEST_F(P2PTransportChannelPingTest, TestDeleteConnectionsIfAllWriteTimedout) { // holds even if the transport channel did not lose the writability. TEST_F(P2PTransportChannelPingTest, TestStopPortAllocatorSessions) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); - cricket::P2PTransportChannel ch("test channel", 1, &pa); + cricket::P2PTransportChannel ch("test channel", 1, nullptr, &pa); PrepareChannel(&ch); ch.SetIceConfig(CreateIceConfig(2000, false)); ch.Connect(); diff --git a/webrtc/p2p/base/transportchannelimpl.h b/webrtc/p2p/base/transportchannelimpl.h index 1fa088a30d..8d4d4bb728 100644 --- a/webrtc/p2p/base/transportchannelimpl.h +++ b/webrtc/p2p/base/transportchannelimpl.h @@ -12,6 +12,7 @@ #define WEBRTC_P2P_BASE_TRANSPORTCHANNELIMPL_H_ #include +#include "webrtc/p2p/base/transport.h" #include "webrtc/p2p/base/transportchannel.h" namespace buzz { class XmlElement; } @@ -35,6 +36,9 @@ class TransportChannelImpl : public TransportChannel { int component) : TransportChannel(transport_name, component) {} + // Returns the transport that created this channel. + virtual Transport* GetTransport() = 0; + // For ICE channels. virtual IceRole GetIceRole() const = 0; virtual void SetIceRole(IceRole role) = 0; diff --git a/webrtc/p2p/quic/quicsession_unittest.cc b/webrtc/p2p/quic/quicsession_unittest.cc index 765a47f0c2..6733796127 100644 --- a/webrtc/p2p/quic/quicsession_unittest.cc +++ b/webrtc/p2p/quic/quicsession_unittest.cc @@ -286,9 +286,9 @@ class QuicSessionTest : public ::testing::Test, // establishes encryption with client. void QuicSessionTest::CreateClientAndServerSessions() { scoped_ptr channel1( - new FakeTransportChannel("channel1", 0)); + new FakeTransportChannel(nullptr, "channel1", 0)); scoped_ptr channel2( - new FakeTransportChannel("channel2", 0)); + new FakeTransportChannel(nullptr, "channel2", 0)); // Prevent channel1->OnReadPacket and channel2->OnReadPacket from calling // themselves in a loop, which causes to future packets to be recursively