diff --git a/talk/p2p/base/session.cc b/talk/p2p/base/session.cc index a48f3cb0f6..0eefe6c2e2 100644 --- a/talk/p2p/base/session.cc +++ b/talk/p2p/base/session.cc @@ -64,7 +64,7 @@ TransportProxy::~TransportProxy() { } } -std::string TransportProxy::type() const { +const std::string& TransportProxy::type() const { return transport_->get()->type(); } @@ -396,8 +396,6 @@ BaseSession::BaseSession(talk_base::Thread* signaling_thread, transport_type_(NS_GINGLE_P2P), initiator_(initiator), identity_(NULL), - local_description_(NULL), - remote_description_(NULL), ice_tiebreaker_(talk_base::CreateRandomId64()), role_switch_(false) { ASSERT(signaling_thread->IsCurrent()); @@ -415,9 +413,38 @@ BaseSession::~BaseSession() { iter != transports_.end(); ++iter) { delete iter->second; } +} - delete remote_description_; - delete local_description_; +const SessionDescription* BaseSession::local_description() const { + // TODO(tommi): Assert on thread correctness. + return local_description_.get(); +} + +const SessionDescription* BaseSession::remote_description() const { + // TODO(tommi): Assert on thread correctness. + return remote_description_.get(); +} + +SessionDescription* BaseSession::remote_description() { + // TODO(tommi): Assert on thread correctness. + return remote_description_.get(); +} + +void BaseSession::set_local_description(const SessionDescription* sdesc) { + // TODO(tommi): Assert on thread correctness. + if (sdesc != local_description_.get()) + local_description_.reset(sdesc); +} + +void BaseSession::set_remote_description(SessionDescription* sdesc) { + // TODO(tommi): Assert on thread correctness. + if (sdesc != remote_description_) + remote_description_.reset(sdesc); +} + +const SessionDescription* BaseSession::initiator_description() const { + // TODO(tommi): Assert on thread correctness. + return initiator_ ? local_description_.get() : remote_description_.get(); } bool BaseSession::SetIdentity(talk_base::SSLIdentity* identity) { @@ -435,11 +462,11 @@ bool BaseSession::PushdownTransportDescription(ContentSource source, ContentAction action, std::string* error_desc) { if (source == CS_LOCAL) { - return PushdownLocalTransportDescription(local_description_, + return PushdownLocalTransportDescription(local_description(), action, error_desc); } - return PushdownRemoteTransportDescription(remote_description_, + return PushdownRemoteTransportDescription(remote_description(), action, error_desc); } @@ -509,8 +536,8 @@ TransportChannel* BaseSession::GetChannel(const std::string& content_name, TransportProxy* transproxy = GetTransportProxy(content_name); if (transproxy == NULL) return NULL; - else - return transproxy->GetChannel(component); + + return transproxy->GetChannel(component); } void BaseSession::DestroyChannel(const std::string& content_name, @@ -819,6 +846,7 @@ void BaseSession::LogState(State old_state, State new_state) { << " Transport:" << transport_type(); } +// static bool BaseSession::GetTransportDescription(const SessionDescription* description, const std::string& content_name, TransportDescription* tdesc) { @@ -928,7 +956,7 @@ Session::~Session() { delete transport_parser_; } -bool Session::Initiate(const std::string &to, +bool Session::Initiate(const std::string& to, const SessionDescription* sdesc) { ASSERT(signaling_thread()->IsCurrent()); SessionError error; @@ -1260,10 +1288,10 @@ void Session::OnIncomingResponse(const buzz::XmlElement* orig_stanza, } void Session::OnInitiateAcked() { - // TODO: This is to work around server re-ordering - // messages. We send the candidates once the session-initiate - // is acked. Once we have fixed the server to guarantee message - // order, we can remove this case. + // TODO: This is to work around server re-ordering + // messages. We send the candidates once the session-initiate + // is acked. Once we have fixed the server to guarantee message + // order, we can remove this case. if (!initiate_acked_) { initiate_acked_ = true; SessionError error; diff --git a/talk/p2p/base/session.h b/talk/p2p/base/session.h index 504187f621..4f99f163f7 100644 --- a/talk/p2p/base/session.h +++ b/talk/p2p/base/session.h @@ -110,12 +110,12 @@ class TransportProxy : public sigslot::has_slots<>, } ~TransportProxy(); - std::string content_name() const { return content_name_; } + const std::string& content_name() const { return content_name_; } // TODO(juberti): It's not good form to expose the object you're wrapping, // since callers can mutate it. Can we make this return a const Transport*? Transport* impl() const { return transport_->get(); } - std::string type() const; + const std::string& type() const; bool negotiated() const { return negotiated_; } const Candidates& sent_candidates() const { return sent_candidates_; } const Candidates& unsent_candidates() const { return unsent_candidates_; } @@ -195,9 +195,9 @@ class TransportProxy : public sigslot::has_slots<>, void ReplaceChannelProxyImpl_w(TransportChannelProxy* proxy, TransportChannelImpl* impl); - talk_base::Thread* worker_thread_; - std::string sid_; - std::string content_name_; + talk_base::Thread* const worker_thread_; + const std::string sid_; + const std::string content_name_; talk_base::scoped_refptr transport_; bool connecting_; bool negotiated_; @@ -275,9 +275,10 @@ class BaseSession : public sigslot::has_slots<>, bool initiator); virtual ~BaseSession(); - talk_base::Thread* signaling_thread() { return signaling_thread_; } - talk_base::Thread* worker_thread() { return worker_thread_; } - PortAllocator* port_allocator() { return port_allocator_; } + // These are const to allow them to be called from const methods. + talk_base::Thread* signaling_thread() const { return signaling_thread_; } + talk_base::Thread* worker_thread() const { return worker_thread_; } + PortAllocator* port_allocator() const { return port_allocator_; } // The ID of this session. const std::string& id() const { return sid_; } @@ -294,43 +295,21 @@ class BaseSession : public sigslot::has_slots<>, // Returns the application-level description given by our client. // If we are the recipient, this will be NULL until we send an accept. - const SessionDescription* local_description() const { - return local_description_; - } + const SessionDescription* local_description() const; + // Returns the application-level description given by the other client. // If we are the initiator, this will be NULL until we receive an accept. - const SessionDescription* remote_description() const { - return remote_description_; - } - SessionDescription* remote_description() { - return remote_description_; - } + const SessionDescription* remote_description() const; + + SessionDescription* remote_description(); // Takes ownership of SessionDescription* - bool set_local_description(const SessionDescription* sdesc) { - if (sdesc != local_description_) { - delete local_description_; - local_description_ = sdesc; - } - return true; - } + void set_local_description(const SessionDescription* sdesc); // Takes ownership of SessionDescription* - bool set_remote_description(SessionDescription* sdesc) { - if (sdesc != remote_description_) { - delete remote_description_; - remote_description_ = sdesc; - } - return true; - } + void set_remote_description(SessionDescription* sdesc); - const SessionDescription* initiator_description() const { - if (initiator_) { - return local_description_; - } else { - return remote_description_; - } - } + const SessionDescription* initiator_description() const; // Returns the current state of the session. See the enum above for details. // Each time the state changes, we will fire this signal. @@ -515,9 +494,9 @@ class BaseSession : public sigslot::has_slots<>, // Returns true and the TransportInfo of the given |content_name| // from |description|. Returns false if it's not available. - bool GetTransportDescription(const SessionDescription* description, - const std::string& content_name, - TransportDescription* info); + static bool GetTransportDescription(const SessionDescription* description, + const std::string& content_name, + TransportDescription* info); // Fires the new description signal according to the current state. void SignalNewDescription(); @@ -525,16 +504,16 @@ class BaseSession : public sigslot::has_slots<>, // Gets the ContentAction and ContentSource according to the session state. bool GetContentAction(ContentAction* action, ContentSource* source); - talk_base::Thread* signaling_thread_; - talk_base::Thread* worker_thread_; - PortAllocator* port_allocator_; - std::string sid_; - std::string content_type_; - std::string transport_type_; + talk_base::Thread* const signaling_thread_; + talk_base::Thread* const worker_thread_; + PortAllocator* const port_allocator_; + const std::string sid_; + const std::string content_type_; + const std::string transport_type_; bool initiator_; talk_base::SSLIdentity* identity_; - const SessionDescription* local_description_; - SessionDescription* remote_description_; + talk_base::scoped_ptr local_description_; + talk_base::scoped_ptr remote_description_; uint64 ice_tiebreaker_; // This flag will be set to true after the first role switch. This flag // will enable us to stop any role switch during the call. diff --git a/talk/session/media/channel_unittest.cc b/talk/session/media/channel_unittest.cc index c22361d657..cb0bdc05b0 100644 --- a/talk/session/media/channel_unittest.cc +++ b/talk/session/media/channel_unittest.cc @@ -1597,12 +1597,12 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { new cricket::AudioContentDescription()); sdesc_loc->AddContent(cricket::CN_VIDEO, cricket::NS_JINGLE_RTP, new cricket::VideoContentDescription()); - EXPECT_TRUE(session1_.set_local_description(sdesc_loc)); + session1_.set_local_description(sdesc_loc); sdesc_rem->AddContent(cricket::CN_AUDIO, cricket::NS_JINGLE_RTP, new cricket::AudioContentDescription()); sdesc_rem->AddContent(cricket::CN_VIDEO, cricket::NS_JINGLE_RTP, new cricket::VideoContentDescription()); - EXPECT_TRUE(session1_.set_remote_description(sdesc_rem)); + session1_.set_remote_description(sdesc_rem); // Test failures in SetLocalContent. media_channel1_->set_fail_set_recv_codecs(true); @@ -1630,7 +1630,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { // Set up the initial session description. cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1); - EXPECT_TRUE(session1_.set_local_description(sdesc)); + session1_.set_local_description(sdesc); session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); session1_.SetState(cricket::Session::STATE_SENTINITIATE); @@ -1639,7 +1639,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { // Update the local description and set the state again. sdesc = CreateSessionDescriptionWithStream(2); - EXPECT_TRUE(session1_.set_local_description(sdesc)); + session1_.set_local_description(sdesc); session1_.SetState(cricket::Session::STATE_SENTINITIATE); EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); @@ -1652,7 +1652,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { // Set up the initial session description. cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1); - EXPECT_TRUE(session1_.set_remote_description(sdesc)); + session1_.set_remote_description(sdesc); session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); session1_.SetState(cricket::Session::STATE_RECEIVEDINITIATE); @@ -1660,7 +1660,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { EXPECT_TRUE(media_channel1_->HasRecvStream(1)); sdesc = CreateSessionDescriptionWithStream(2); - EXPECT_TRUE(session1_.set_remote_description(sdesc)); + session1_.set_remote_description(sdesc); session1_.SetState(cricket::Session::STATE_RECEIVEDINITIATE); EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); EXPECT_FALSE(media_channel1_->HasRecvStream(1)); @@ -1672,7 +1672,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { // Set up the initial session description. cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1); - EXPECT_TRUE(session1_.set_remote_description(sdesc)); + session1_.set_remote_description(sdesc); session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); session1_.SetState(cricket::Session::STATE_RECEIVEDINITIATE); @@ -1681,7 +1681,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { // Send PRANSWER sdesc = CreateSessionDescriptionWithStream(2); - EXPECT_TRUE(session1_.set_local_description(sdesc)); + session1_.set_local_description(sdesc); session1_.SetState(cricket::Session::STATE_SENTPRACCEPT); EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); @@ -1690,7 +1690,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { // Send ACCEPT sdesc = CreateSessionDescriptionWithStream(3); - EXPECT_TRUE(session1_.set_local_description(sdesc)); + session1_.set_local_description(sdesc); session1_.SetState(cricket::Session::STATE_SENTACCEPT); EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); @@ -1704,7 +1704,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { // Set up the initial session description. cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1); - EXPECT_TRUE(session1_.set_local_description(sdesc)); + session1_.set_local_description(sdesc); session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); session1_.SetState(cricket::Session::STATE_SENTINITIATE); @@ -1713,7 +1713,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { // Receive PRANSWER sdesc = CreateSessionDescriptionWithStream(2); - EXPECT_TRUE(session1_.set_remote_description(sdesc)); + session1_.set_remote_description(sdesc); session1_.SetState(cricket::Session::STATE_RECEIVEDPRACCEPT); EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); @@ -1722,7 +1722,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { // Receive ACCEPT sdesc = CreateSessionDescriptionWithStream(3); - EXPECT_TRUE(session1_.set_remote_description(sdesc)); + session1_.set_remote_description(sdesc); session1_.SetState(cricket::Session::STATE_RECEIVEDACCEPT); EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error());