Replaces SSLIdentity* with scoped_refptr<RTCCertificate> in cricket::Transport layer.

webrtc/p2p/base/session.cc

Index: webrtc/p2p/base/session.cc
diff --git a/webrtc/p2p/base/session.cc b/webrtc/p2p/base/session.cc
index 6c185113f9226e0d5bc9b4e48832fe7cc2805aa4..c564d4e0788e348c9fddca9d23f405eca1583637 100644
--- a/webrtc/p2p/base/session.cc
+++ b/webrtc/p2p/base/session.cc
@@ -277,9 +277,9 @@ bool TransportProxy::OnRemoteCandidates(const Candidates& candidates,
   return true;
 }
 
-void TransportProxy::SetIdentity(
-    rtc::SSLIdentity* identity) {
-  transport_->get()->SetIdentity(identity);
+void TransportProxy::SetCertificate(
+    const rtc::scoped_refptr<rtc::RTCCertificate>& certificate) {
+  transport_->get()->SetCertificate(certificate);

[tommi, 2015/08/25 10:28:08]

thread safe? (ok to touch transport_ on any thread?)

[hbos, 2015/08/25 15:45:44]

The |transport_| object's SetCertificate posts to the worker thread and should
be safe. The |transport_| variable itself is set and read without locks so
TransportProxy should either be used single threadedly or with care,
|transport_| is only set in SetupMux...

 }
 
 std::string BaseSession::StateToString(State state) {
@@ -336,7 +336,6 @@ BaseSession::BaseSession(rtc::Thread* signaling_thread,
       sid_(sid),
       content_type_(content_type),
       initiator_(initiator),
-      identity_(NULL),
       ssl_max_version_(rtc::SSL_PROTOCOL_DTLS_10),
       ice_tiebreaker_(rtc::CreateRandomId64()),
       role_switch_(false),
@@ -390,13 +389,14 @@ const SessionDescription* BaseSession::initiator_description() const {
   return initiator_ ? local_description_.get() : remote_description_.get();
 }
 
-bool BaseSession::SetIdentity(rtc::SSLIdentity* identity) {
-  if (identity_)
+bool BaseSession::SetCertificate(
+    const rtc::scoped_refptr<rtc::RTCCertificate>& certificate) {
+  if (certificate_ || !certificate)

[tommi, 2015/08/25 10:28:08]

I'm not sure I understand this check... is it not allowed to set a null
certificate or set one if a certificate has been set?
if so, then I think it would be slightly more readable as two separate if()
statements.

[hbos, 2015/08/25 15:45:44]

You could only set it to null if it was already null but if it already is then
why bother, pointless. The addition of this if statement (!certificate) could
arguably also be called pointless and I added it at a time when I had to do
certificate->identity() (and wanted to avoid a conditional operator) but that is
no longer the case. Also, I think this function is never called with a null
certificate. Pointless all around.

Anyway, I pointlessly made it two if statements for clarity.

[tommi, 2015/08/25 16:34:53]

point(less) taken

     return false;
-  identity_ = identity;
+  certificate_ = certificate;
   for (TransportMap::iterator iter = transports_.begin();
        iter != transports_.end(); ++iter) {
-    iter->second->SetIdentity(identity_);
+    iter->second->SetCertificate(certificate_);
   }
   return true;
 }
@@ -543,8 +543,8 @@ TransportProxy* BaseSession::GetOrCreateTransportProxy(
                                   new TransportWrapper(transport));
   transproxy->SignalCandidatesReady.connect(
       this, &BaseSession::OnTransportProxyCandidatesReady);
-  if (identity_)
-    transproxy->SetIdentity(identity_);
+  if (certificate_)
+    transproxy->SetCertificate(certificate_);
   transports_[content_name] = transproxy;
 
   return transproxy;
@@ -575,7 +575,7 @@ void BaseSession::DestroyTransportProxy(
 Transport* BaseSession::CreateTransport(const std::string& content_name) {
   Transport* transport = new DtlsTransport<P2PTransport>(
       signaling_thread(), worker_thread(), content_name, port_allocator(),
-      identity_);
+      certificate_);
   transport->SetChannelReceivingTimeout(ice_receiving_timeout_);
   return transport;
 }