Minor refactoring of the session classes.
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
diff --git a/talk/p2p/base/session.cc b/talk/p2p/base/session.cc
index a48f3cb..0eefe6c 100644
--- a/talk/p2p/base/session.cc
+++ b/talk/p2p/base/session.cc
@@ -64,7 +64,7 @@
}
}
-std::string TransportProxy::type() const {
+const std::string& TransportProxy::type() const {
return transport_->get()->type();
}
@@ -396,8 +396,6 @@
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 @@
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 @@
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 @@
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 @@
<< " Transport:" << transport_type();
}
+// static
bool BaseSession::GetTransportDescription(const SessionDescription* description,
const std::string& content_name,
TransportDescription* tdesc) {
@@ -928,7 +956,7 @@
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::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 504187f..4f99f16 100644
--- a/talk/p2p/base/session.h
+++ b/talk/p2p/base/session.h
@@ -110,12 +110,12 @@
}
~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 @@
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<TransportWrapper> transport_;
bool connecting_;
bool negotiated_;
@@ -275,9 +275,10 @@
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 @@
// 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 @@
// 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 @@
// 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<const SessionDescription> local_description_;
+ talk_base::scoped_ptr<SessionDescription> 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 c22361d..cb0bdc0 100644
--- a/talk/session/media/channel_unittest.cc
+++ b/talk/session/media/channel_unittest.cc
@@ -1597,12 +1597,12 @@
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 @@
// 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 @@
// 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 @@
// 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 @@
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 @@
// 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 @@
// 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 @@
// 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 @@
// 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 @@
// 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 @@
// 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());
