generic-library/webrtc
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Minor refactoring of the session classes.

Browse Source 
Make member variables that never change and are touched on multiple threads, const.
Move implementations of setters/getters of variables that can change, into the cc file in preparation of adding thread correctness checks.

This is a relanding of a cl already reviewed but got reverted by mistake.

TBR=xians@google.com

Review URL: https://webrtc-codereview.appspot.com/12979004

git-svn-id: http://webrtc.googlecode.com/svn/trunk@6676 4adac7df-926f-26a2-2b94-8c16560cd09d
This commit is contained in:
tommi@webrtc.org 2014-07-14 20:11:49 +00:00
parent d8524348bb
commit b5348c64bb
3 changed files with 82 additions and 75 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

56
talk/p2p/base/session.cc
View File

@ -64,7 +64,7 @@ TransportProxy::~TransportProxy() {
} }
} }
std::string TransportProxy::type() const { const std::string& TransportProxy::type() const {
return transport_->get()->type(); return transport_->get()->type();
} }
@ -396,8 +396,6 @@ BaseSession::BaseSession(talk_base::Thread* signaling_thread,
transport_type_(NS_GINGLE_P2P), transport_type_(NS_GINGLE_P2P),
initiator_(initiator), initiator_(initiator),
identity_(NULL), identity_(NULL),
local_description_(NULL),
remote_description_(NULL),
ice_tiebreaker_(talk_base::CreateRandomId64()), ice_tiebreaker_(talk_base::CreateRandomId64()),
role_switch_(false) { role_switch_(false) {
ASSERT(signaling_thread->IsCurrent()); ASSERT(signaling_thread->IsCurrent());
@ -415,9 +413,38 @@ BaseSession::~BaseSession() {
iter != transports_.end(); ++iter) { iter != transports_.end(); ++iter) {
delete iter->second; delete iter->second;
} }
}
delete remote_description_; const SessionDescription* BaseSession::local_description() const {
delete local_description_; // 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) { bool BaseSession::SetIdentity(talk_base::SSLIdentity* identity) {
@ -435,11 +462,11 @@ bool BaseSession::PushdownTransportDescription(ContentSource source,
ContentAction action, ContentAction action,
std::string* error_desc) { std::string* error_desc) {
if (source == CS_LOCAL) { if (source == CS_LOCAL) {
return PushdownLocalTransportDescription(local_description_, return PushdownLocalTransportDescription(local_description(),
action, action,
error_desc); error_desc);
} }
return PushdownRemoteTransportDescription(remote_description_, return PushdownRemoteTransportDescription(remote_description(),
action, action,
error_desc); error_desc);
} }
@ -509,8 +536,8 @@ TransportChannel* BaseSession::GetChannel(const std::string& content_name,
TransportProxy* transproxy = GetTransportProxy(content_name); TransportProxy* transproxy = GetTransportProxy(content_name);
if (transproxy == NULL) if (transproxy == NULL)
return NULL; return NULL;
else
return transproxy->GetChannel(component); return transproxy->GetChannel(component);
} }
void BaseSession::DestroyChannel(const std::string& content_name, void BaseSession::DestroyChannel(const std::string& content_name,
@ -819,6 +846,7 @@ void BaseSession::LogState(State old_state, State new_state) {
<< " Transport:" << transport_type(); << " Transport:" << transport_type();
} }
// static
bool BaseSession::GetTransportDescription(const SessionDescription* description, bool BaseSession::GetTransportDescription(const SessionDescription* description,
const std::string& content_name, const std::string& content_name,
TransportDescription* tdesc) { TransportDescription* tdesc) {
@ -928,7 +956,7 @@ Session::~Session() {
delete transport_parser_; delete transport_parser_;
} }
bool Session::Initiate(const std::string &to, bool Session::Initiate(const std::string& to,
const SessionDescription* sdesc) { const SessionDescription* sdesc) {
ASSERT(signaling_thread()->IsCurrent()); ASSERT(signaling_thread()->IsCurrent());
SessionError error; SessionError error;
@ -1260,10 +1288,10 @@ void Session::OnIncomingResponse(const buzz::XmlElement* orig_stanza,
} }
void Session::OnInitiateAcked() { void Session::OnInitiateAcked() {
// TODO: This is to work around server re-ordering // TODO: This is to work around server re-ordering
// messages. We send the candidates once the session-initiate // messages. We send the candidates once the session-initiate
// is acked. Once we have fixed the server to guarantee message // is acked. Once we have fixed the server to guarantee message
// order, we can remove this case. // order, we can remove this case.
if (!initiate_acked_) { if (!initiate_acked_) {
initiate_acked_ = true; initiate_acked_ = true;
SessionError error; SessionError error;

77
talk/p2p/base/session.h
View File

@ -110,12 +110,12 @@ class TransportProxy : public sigslot::has_slots<>,
} }
~TransportProxy(); ~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, // 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*? // since callers can mutate it. Can we make this return a const Transport*?
Transport* impl() const { return transport_->get(); } Transport* impl() const { return transport_->get(); }
std::string type() const; const std::string& type() const;
bool negotiated() const { return negotiated_; } bool negotiated() const { return negotiated_; }
const Candidates& sent_candidates() const { return sent_candidates_; } const Candidates& sent_candidates() const { return sent_candidates_; }
const Candidates& unsent_candidates() const { return unsent_candidates_; } const Candidates& unsent_candidates() const { return unsent_candidates_; }
@ -195,9 +195,9 @@ class TransportProxy : public sigslot::has_slots<>,
void ReplaceChannelProxyImpl_w(TransportChannelProxy* proxy, void ReplaceChannelProxyImpl_w(TransportChannelProxy* proxy,
TransportChannelImpl* impl); TransportChannelImpl* impl);
talk_base::Thread* worker_thread_; talk_base::Thread* const worker_thread_;
std::string sid_; const std::string sid_;
std::string content_name_; const std::string content_name_;
talk_base::scoped_refptr<TransportWrapper> transport_; talk_base::scoped_refptr<TransportWrapper> transport_;
bool connecting_; bool connecting_;
bool negotiated_; bool negotiated_;
@ -275,9 +275,10 @@ class BaseSession : public sigslot::has_slots<>,
bool initiator); bool initiator);
virtual ~BaseSession(); virtual ~BaseSession();
talk_base::Thread* signaling_thread() { return signaling_thread_; } // These are const to allow them to be called from const methods.
talk_base::Thread* worker_thread() { return worker_thread_; } talk_base::Thread* signaling_thread() const { return signaling_thread_; }
PortAllocator* port_allocator() { return port_allocator_; } talk_base::Thread* worker_thread() const { return worker_thread_; }
PortAllocator* port_allocator() const { return port_allocator_; }
// The ID of this session. // The ID of this session.
const std::string& id() const { return sid_; } 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. // Returns the application-level description given by our client.
// If we are the recipient, this will be NULL until we send an accept. // If we are the recipient, this will be NULL until we send an accept.
const SessionDescription* local_description() const { const SessionDescription* local_description() const;
return local_description_;
}
// Returns the application-level description given by the other client. // Returns the application-level description given by the other client.
// If we are the initiator, this will be NULL until we receive an accept. // If we are the initiator, this will be NULL until we receive an accept.
const SessionDescription* remote_description() const { const SessionDescription* remote_description() const;
return remote_description_;
} SessionDescription* remote_description();
SessionDescription* remote_description() {
return remote_description_;
}
// Takes ownership of SessionDescription* // Takes ownership of SessionDescription*
bool set_local_description(const SessionDescription* sdesc) { void set_local_description(const SessionDescription* sdesc);
if (sdesc != local_description_) {
delete local_description_;
local_description_ = sdesc;
}
return true;
}
// Takes ownership of SessionDescription* // Takes ownership of SessionDescription*
bool set_remote_description(SessionDescription* sdesc) { void set_remote_description(SessionDescription* sdesc);
if (sdesc != remote_description_) {
delete remote_description_;
remote_description_ = sdesc;
}
return true;
}
const SessionDescription* initiator_description() const { const SessionDescription* initiator_description() const;
if (initiator_) {
return local_description_;
} else {
return remote_description_;
}
}
// Returns the current state of the session. See the enum above for details. // Returns the current state of the session. See the enum above for details.
// Each time the state changes, we will fire this signal. // 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| // Returns true and the TransportInfo of the given |content_name|
// from |description|. Returns false if it's not available. // from |description|. Returns false if it's not available.
bool GetTransportDescription(const SessionDescription* description, static bool GetTransportDescription(const SessionDescription* description,
const std::string& content_name, const std::string& content_name,
TransportDescription* info); TransportDescription* info);
// Fires the new description signal according to the current state. // Fires the new description signal according to the current state.
void SignalNewDescription(); void SignalNewDescription();
@ -525,16 +504,16 @@ class BaseSession : public sigslot::has_slots<>,
// Gets the ContentAction and ContentSource according to the session state. // Gets the ContentAction and ContentSource according to the session state.
bool GetContentAction(ContentAction* action, ContentSource* source); bool GetContentAction(ContentAction* action, ContentSource* source);
talk_base::Thread* signaling_thread_; talk_base::Thread* const signaling_thread_;
talk_base::Thread* worker_thread_; talk_base::Thread* const worker_thread_;
PortAllocator* port_allocator_; PortAllocator* const port_allocator_;
std::string sid_; const std::string sid_;
std::string content_type_; const std::string content_type_;
std::string transport_type_; const std::string transport_type_;
bool initiator_; bool initiator_;
talk_base::SSLIdentity* identity_; talk_base::SSLIdentity* identity_;
const SessionDescription* local_description_; talk_base::scoped_ptr<const SessionDescription> local_description_;
SessionDescription* remote_description_; talk_base::scoped_ptr<SessionDescription> remote_description_;
uint64 ice_tiebreaker_; uint64 ice_tiebreaker_;
// This flag will be set to true after the first role switch. This flag // 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. // will enable us to stop any role switch during the call.

24
talk/session/media/channel_unittest.cc
View File

@ -1597,12 +1597,12 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> {
new cricket::AudioContentDescription()); new cricket::AudioContentDescription());
sdesc_loc->AddContent(cricket::CN_VIDEO, cricket::NS_JINGLE_RTP, sdesc_loc->AddContent(cricket::CN_VIDEO, cricket::NS_JINGLE_RTP,
new cricket::VideoContentDescription()); 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, sdesc_rem->AddContent(cricket::CN_AUDIO, cricket::NS_JINGLE_RTP,
new cricket::AudioContentDescription()); new cricket::AudioContentDescription());
sdesc_rem->AddContent(cricket::CN_VIDEO, cricket::NS_JINGLE_RTP, sdesc_rem->AddContent(cricket::CN_VIDEO, cricket::NS_JINGLE_RTP,
new cricket::VideoContentDescription()); new cricket::VideoContentDescription());
EXPECT_TRUE(session1_.set_remote_description(sdesc_rem)); session1_.set_remote_description(sdesc_rem);
// Test failures in SetLocalContent. // Test failures in SetLocalContent.
media_channel1_->set_fail_set_recv_codecs(true); 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. // Set up the initial session description.
cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1); cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1);
EXPECT_TRUE(session1_.set_local_description(sdesc)); session1_.set_local_description(sdesc);
session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); session1_.SetError(cricket::BaseSession::ERROR_NONE, "");
session1_.SetState(cricket::Session::STATE_SENTINITIATE); 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. // Update the local description and set the state again.
sdesc = CreateSessionDescriptionWithStream(2); sdesc = CreateSessionDescriptionWithStream(2);
EXPECT_TRUE(session1_.set_local_description(sdesc)); session1_.set_local_description(sdesc);
session1_.SetState(cricket::Session::STATE_SENTINITIATE); session1_.SetState(cricket::Session::STATE_SENTINITIATE);
EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); 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. // Set up the initial session description.
cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1); cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1);
EXPECT_TRUE(session1_.set_remote_description(sdesc)); session1_.set_remote_description(sdesc);
session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); session1_.SetError(cricket::BaseSession::ERROR_NONE, "");
session1_.SetState(cricket::Session::STATE_RECEIVEDINITIATE); 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)); EXPECT_TRUE(media_channel1_->HasRecvStream(1));
sdesc = CreateSessionDescriptionWithStream(2); sdesc = CreateSessionDescriptionWithStream(2);
EXPECT_TRUE(session1_.set_remote_description(sdesc)); session1_.set_remote_description(sdesc);
session1_.SetState(cricket::Session::STATE_RECEIVEDINITIATE); session1_.SetState(cricket::Session::STATE_RECEIVEDINITIATE);
EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error());
EXPECT_FALSE(media_channel1_->HasRecvStream(1)); 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. // Set up the initial session description.
cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1); cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1);
EXPECT_TRUE(session1_.set_remote_description(sdesc)); session1_.set_remote_description(sdesc);
session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); session1_.SetError(cricket::BaseSession::ERROR_NONE, "");
session1_.SetState(cricket::Session::STATE_RECEIVEDINITIATE); session1_.SetState(cricket::Session::STATE_RECEIVEDINITIATE);
@ -1681,7 +1681,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> {
// Send PRANSWER // Send PRANSWER
sdesc = CreateSessionDescriptionWithStream(2); sdesc = CreateSessionDescriptionWithStream(2);
EXPECT_TRUE(session1_.set_local_description(sdesc)); session1_.set_local_description(sdesc);
session1_.SetState(cricket::Session::STATE_SENTPRACCEPT); session1_.SetState(cricket::Session::STATE_SENTPRACCEPT);
EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error());
@ -1690,7 +1690,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> {
// Send ACCEPT // Send ACCEPT
sdesc = CreateSessionDescriptionWithStream(3); sdesc = CreateSessionDescriptionWithStream(3);
EXPECT_TRUE(session1_.set_local_description(sdesc)); session1_.set_local_description(sdesc);
session1_.SetState(cricket::Session::STATE_SENTACCEPT); session1_.SetState(cricket::Session::STATE_SENTACCEPT);
EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); 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. // Set up the initial session description.
cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1); cricket::SessionDescription* sdesc = CreateSessionDescriptionWithStream(1);
EXPECT_TRUE(session1_.set_local_description(sdesc)); session1_.set_local_description(sdesc);
session1_.SetError(cricket::BaseSession::ERROR_NONE, ""); session1_.SetError(cricket::BaseSession::ERROR_NONE, "");
session1_.SetState(cricket::Session::STATE_SENTINITIATE); session1_.SetState(cricket::Session::STATE_SENTINITIATE);
@ -1713,7 +1713,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> {
// Receive PRANSWER // Receive PRANSWER
sdesc = CreateSessionDescriptionWithStream(2); sdesc = CreateSessionDescriptionWithStream(2);
EXPECT_TRUE(session1_.set_remote_description(sdesc)); session1_.set_remote_description(sdesc);
session1_.SetState(cricket::Session::STATE_RECEIVEDPRACCEPT); session1_.SetState(cricket::Session::STATE_RECEIVEDPRACCEPT);
EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error());
@ -1722,7 +1722,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> {
// Receive ACCEPT // Receive ACCEPT
sdesc = CreateSessionDescriptionWithStream(3); sdesc = CreateSessionDescriptionWithStream(3);
EXPECT_TRUE(session1_.set_remote_description(sdesc)); session1_.set_remote_description(sdesc);
session1_.SetState(cricket::Session::STATE_RECEIVEDACCEPT); session1_.SetState(cricket::Session::STATE_RECEIVEDACCEPT);
EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error()); EXPECT_EQ(cricket::BaseSession::ERROR_NONE, session1_.error());