Added DtlsCertificate, a ref counted object owning an SSLIdentity

--- Made obsolete by other CLs, see webrtc:4927 ---

Added DtlsCertificate, a ref counted object owning an SSLIdentity.

The use of DtlsCertificate allows the same SSLIdentity to be reused by multiple peer connections. The old CreatePeerConnection signature still exists, but a new CreatePeerConnection signature is added that takes a certificate instead of a DtlsIdentityStoreInterface.
This is for the latest WebRTC standard which says that you should optionally be able to generate certificates separately and supply them to CreatePeerConnection.

A scoped_refptr<DtlsCertificate> is passed through these layers (just like DtlsIdentityStoreInterface*)...
PeerConnectionFactory::CreatePeerConnection -> PeerConnection::Initialize -> WebRtcSession::Initialize -> WebRtcSessionDescriptionFactory::WebRtcSessionDescriptionFactory
(Edit: Should make the certificate(s) a part of RTCConfiguration, not a stand alone parameter of CreatePeerConnection)

These functions have been updated so that there is one version taking a store and one version taking a certificate. WebRtcSessionDescriptionFactory has been cleaned up a bit.

The majority of the files changed are updates from SSLIdentity* to scoped_refptr<DtlsCertificate> (Patch Set 11 and onwards).

BUG=webrtc:4902

[hbos, 2015-08-03 07:09:58]

Patchset #1 (id:1) has been deleted

[hbos, 2015-08-03 07:10:08]

Patchset #1 (id:20001) has been deleted

[hbos, 2015-08-04 11:47:08]

hbos@webrtc.org changed reviewers:
+ tommi@webrtc.org

[hbos, 2015-08-04 12:27:35]

hbos@webrtc.org changed reviewers:
+ henrikg@webrtc.org

[hbos, 2015-08-04 12:50:21]

PTAL tommi and henrikg.

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/dtlscert...
File talk/app/webrtc/dtlscertificate.cc (right):

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/dtlscert...
talk/app/webrtc/dtlscertificate.cc:38: return new
rtc::RefCountedObject<DtlsCertificate>(identity.release(), 0.0);
In blink it looked like double was used for dates and a javascript
RTCCertificate is supposed to have an expires field, I added the same here but
it's not really used yet, just initializing it to 0.0... I could also remove it
until it's needed but I think an expires() might be helpful when integrating
with blink work.

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:352:
rtc::scoped_refptr<webrtc::DtlsCertificate> certificate,
Already in webrtc namespace, I shall remove "webrtc::".

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.h (left):

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.h:166: PeerConnectionObserver* observer);
Declared but not defined, that's why I'm removing it.

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.cc:294: PeerConnectionObserver* observer)
{
This version of CreatePeerConnection uses the old function signature and exists
as to not break Chromium. But we want to remove it ASAP because it blocks until
a certificate has been generated.

We'll want to think twice before landing this.

[Henrik Grunell WebRTC, 2015-08-05 07:41:20]

First round - more high level reviewing.

Please file a bug. Link to the spec there.

Great that you described this thoroughly.

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:553: // new function signature.
Can that be done right away? If not, when is that planned to be done? Would be
nice to skip the intermediate step if possible.

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/test/fak...
File talk/app/webrtc/test/fakedtlsidentityservice.h (right):

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/test/fak...
talk/app/webrtc/test/fakedtlsidentityservice.h:144: static void
GenerateIdentity(
Make this an ordinary function in the implementation file instead of static.

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:525:
rtc::scoped_refptr<webrtc::DtlsCertificate> certificate,
Who keeps references to the certificate? (Why does it have to be ref counted?)

[hbos, 2015-08-05 08:13:00]

After talking with henrikg I realized we don't want to remove the passing around
of DTLSIdentityServiceInterface*. In the event of passing in a null certificate
we should let identity generation occur just like in the old version of the
code, allowing the use of Chromium's implementation of the service. In PS 1-7
passing in a null certificate would have meant fallback on the webrtc
implementation of the service, giving the caller no control of which service is
used.

Continuing to pass around DTLSIdentityServiceInterface* means less code changes,
backwards-compatibility (no need for that temp blocking version of
CreatePeerConnection) and better performance (in the event of not already having
generated a cert).

[hbos, 2015-08-06 12:11:34]

PTAL henrikg, tommi.

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:553: // new function signature.
On 2015/08/05 07:41:20, Henrik Grunell (webrtc) wrote:
> Can that be done right away? If not, when is that planned to be done? Would be
> nice to skip the intermediate step if possible.

Removing this function and the the blocking identity generation in favor of
allowing DTLSIdentityServiceInterface* to be used just like prior to this patch.
This CL will be backwards-compatible with how it was before, the new certificate
parameter being an optional parameter. Only if a cert is specified do we skip
identity generation in WebRtcSessionDescriptionFactory, otherwise it will
functionally be the same as before.

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/test/fak...
File talk/app/webrtc/test/fakedtlsidentityservice.h (right):

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/test/fak...
talk/app/webrtc/test/fakedtlsidentityservice.h:144: static void
GenerateIdentity(
On 2015/08/05 07:41:20, Henrik Grunell (webrtc) wrote:
> Make this an ordinary function in the implementation file instead of static.

We talked about this and decided it didn't matter (right?). Keeping it static
unless there is a reason to change it.

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:525:
rtc::scoped_refptr<webrtc::DtlsCertificate> certificate,
On 2015/08/05 07:41:20, Henrik Grunell (webrtc) wrote:
> Who keeps references to the certificate? (Why does it have to be ref counted?)

The certificate (and its identity) is used in a lot of places, reference
counting helps to ensure that it is not deleted whilst, for example, the
identity that it owns is still used somewhere.

Certificates are now reusable things, you might have multiple peer connections
using the same certificate, so you can't delete it in ~WebRtcSession like the
SSLIdentity* previously was.

If we don't make it reference counted we have to leave ownership up to the
caller of CreatePeerConnection. In practice I'm guessing the owner of such a
DtlsCertificate* pointer would be RTCCertificate (blink wrapper version of
DtlsCertificate to be added in future patches). But I'm thinking blink might be
able to decide to delete the RTCCertificate before it deletes the
RTCPeerConnection (and friend objects, some of which are using
DtlsCertificate/SSLIdentity), and then the certificate would be deleted too
soon.

It's probably easier to have it ref counted, guarding it from premature
deletion.

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:575: PeerConnectionObserver* observer)
= 0;
I should add a comment saying what |dtls_identity_service| and |certificate| is
used for here and in WebRtcSessionDescriptionFactory.

Like before, the service is used to generate an identity (a certificate). If a
null service is provided a default one is used (the webrtc implementation).
Both the service and the certificate are optional parameters (can be null),
however if a certificate is provided then there is no need to use the service.
In that case the service is as good as ignored, except for the fact that this
function takes ownership of it.

[Henrik Grunell WebRTC, 2015-08-06 14:06:52]

Another round.

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:553: // new function signature.
On 2015/08/06 12:11:34, hbos wrote:
> On 2015/08/05 07:41:20, Henrik Grunell (webrtc) wrote:
> > Can that be done right away? If not, when is that planned to be done? Would
be
> > nice to skip the intermediate step if possible.
> 
> Removing this function and the the blocking identity generation in favor of
> allowing DTLSIdentityServiceInterface* to be used just like prior to this
patch.
> This CL will be backwards-compatible with how it was before, the new
certificate
> parameter being an optional parameter. Only if a cert is specified do we skip
> identity generation in WebRtcSessionDescriptionFactory, otherwise it will
> functionally be the same as before.

Acknowledged.

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/test/fak...
File talk/app/webrtc/test/fakedtlsidentityservice.h (right):

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/test/fak...
talk/app/webrtc/test/fakedtlsidentityservice.h:144: static void
GenerateIdentity(
On 2015/08/06 12:11:34, hbos wrote:
> On 2015/08/05 07:41:20, Henrik Grunell (webrtc) wrote:
> > Make this an ordinary function in the implementation file instead of static.
> 
> We talked about this and decided it didn't matter (right?). Keeping it static
> unless there is a reason to change it.

Acknowledged.

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1269843005/diff/140001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:525:
rtc::scoped_refptr<webrtc::DtlsCertificate> certificate,
On 2015/08/06 12:11:34, hbos wrote:
> On 2015/08/05 07:41:20, Henrik Grunell (webrtc) wrote:
> > Who keeps references to the certificate? (Why does it have to be ref
counted?)
> 
> The certificate (and its identity) is used in a lot of places, reference
> counting helps to ensure that it is not deleted whilst, for example, the
> identity that it owns is still used somewhere.
> 
> Certificates are now reusable things, you might have multiple peer connections
> using the same certificate, so you can't delete it in ~WebRtcSession like the
> SSLIdentity* previously was.
> 
> If we don't make it reference counted we have to leave ownership up to the
> caller of CreatePeerConnection. In practice I'm guessing the owner of such a
> DtlsCertificate* pointer would be RTCCertificate (blink wrapper version of
> DtlsCertificate to be added in future patches). But I'm thinking blink might
be
> able to decide to delete the RTCCertificate before it deletes the
> RTCPeerConnection (and friend objects, some of which are using
> DtlsCertificate/SSLIdentity), and then the certificate would be deleted too
> soon.
> 
> It's probably easier to have it ref counted, guarding it from premature
> deletion.

Acknowledged.

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactory.h (right):

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.h:55: rtc::scoped_refptr<DtlsCertificate>
certificate,
Create a new function instead that takes a certificate and no identity service.
(All the way to session.) Let the current function behave exactly like it does
now.

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:170:
signaling_thread_->Post(this, MSG_USE_CONSTRUCTOR_CERTIFICATE, NULL);
As discussed, pass the certificate here.

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:174: // Create observer and
connect callback functions.
Remove this comment, its clear from the code what is done.

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsessiondescriptionfactory.h (right):

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:162:
rtc::scoped_refptr<DtlsCertificate> certificate_;
As discussed offline, move reference to transport description factory where it's
actually used.

[hbos, 2015-08-10 13:36:39]

Patchset #11 (id:240001) has been deleted

[hbos, 2015-08-10 13:36:54]

Patchset #11 (id:260001) has been deleted

[hbos, 2015-08-10 14:30:07]

Patchset #11 (id:280001) has been deleted

[hbos, 2015-08-10 15:10:18]

PTAL grunell.

Passing scoped_refptr<DtlsCertificate> around instead of SSLIdentity* wound up
affecting a lot of files I'm afraid.

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactory.h (right):

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.h:55: rtc::scoped_refptr<DtlsCertificate>
certificate,
On 2015/08/06 14:06:52, Henrik Grunell (webrtc) wrote:
> Create a new function instead that takes a certificate and no identity
service.
> (All the way to session.) Let the current function behave exactly like it does
> now.

Acknowledged.

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:170:
signaling_thread_->Post(this, MSG_USE_CONSTRUCTOR_CERTIFICATE, NULL);
On 2015/08/06 14:06:52, Henrik Grunell (webrtc) wrote:
> As discussed, pass the certificate here.

Acknowledged.

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:174: // Create observer and
connect callback functions.
On 2015/08/06 14:06:52, Henrik Grunell (webrtc) wrote:
> Remove this comment, its clear from the code what is done.

Acknowledged.

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsessiondescriptionfactory.h (right):

https://codereview.webrtc.org/1269843005/diff/200001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:162:
rtc::scoped_refptr<DtlsCertificate> certificate_;
On 2015/08/06 14:06:52, Henrik Grunell (webrtc) wrote:
> As discussed offline, move reference to transport description factory where
it's
> actually used.

Acknowledged.

https://codereview.webrtc.org/1269843005/diff/300001/talk/app/webrtc/dtlscert...
File talk/app/webrtc/dtlscertificate.h (right):

https://codereview.webrtc.org/1269843005/diff/300001/talk/app/webrtc/dtlscert...
talk/app/webrtc/dtlscertificate.h:38: namespace webrtc {
Hmh... should I use the webrtc or rtc namespace for this? SSLIdentity is in rtc.
Not sure what the namespaces imply.

A question along the same lines is whether this belongs here, in
talk/app/webrtc/, or in webrtc/base/ where SSLIdentity is located. I placed it
in talk/app/webrtc and used the webrtc namespace simply because that's where the
dtlsidentitystore & -service was located.

https://codereview.webrtc.org/1269843005/diff/300001/talk/app/webrtc/proxy.h
File talk/app/webrtc/proxy.h (right):

https://codereview.webrtc.org/1269843005/diff/300001/talk/app/webrtc/proxy.h#...
talk/app/webrtc/proxy.h:421: 
Added PROXY_METHOD6 and the likes when I needed them. If I end up not needing
them (such as in this PS), you think I should remove them from this CL?

https://codereview.webrtc.org/1269843005/diff/300001/talk/libjingle.gyp
File talk/libjingle.gyp (right):

https://codereview.webrtc.org/1269843005/diff/300001/talk/libjingle.gyp#newco...
talk/libjingle.gyp:793: },  # target libjingle_dtls
dtlscertificate is used by both libjingle_peerconnection and
webrtc/base/base.gyp:rtc_base. I created a new build target to avoid .gyp
issues.

Details as remembered:

Having dtlscertificate in libjingle_peerconnection's sources works for making
libjingle_peerconnection and related stuff compile, but compiling rtc_base would
not work with just that.
Making rtc_base depend on libjingle_peerconnection causes a circular dependency
and does not compile (I drew a graph :) ).
I tried to also add it to rtc_base's sources but then dtlscertificate would be
compiled multiple times causing "defined multiple times" errors.
Having dtlscertificate be in rtc_base's sources alone, and not in
libjingle_peerconnection's sources, compiled locally but that caused a presubmit
error saying something like you're not allowed to have source files that are not
in the .gyp file's folder or subfolders.

This stand-alone build target can be referenced by both libjingle_peerconnection
and rtc_base without the above mentioned issues. Hope that's OK with the gyp
gurus.

[tommi (sloooow) - chröme, 2015-08-10 16:10:15]

tommi@chromium.org changed reviewers:
+ tommi@chromium.org

[tommi (sloooow) - chröme, 2015-08-10 16:10:16]

https://codereview.webrtc.org/1269843005/diff/320001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1269843005/diff/320001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:512:
rtc::scoped_refptr<DtlsCertificate> certificate) {
this should be passed by const &

https://codereview.webrtc.org/1269843005/diff/320001/webrtc/p2p/base/rawtrans...
File webrtc/p2p/base/rawtransportchannel.h (right):

https://codereview.webrtc.org/1269843005/diff/320001/webrtc/p2p/base/rawtrans...
webrtc/p2p/base/rawtransportchannel.h:149:
rtc::scoped_refptr<webrtc::DtlsCertificate> certificate) override {
const &

(please fix throughout)

[tommi (sloooow) - chröme, 2015-08-10 16:10:54]

https://codereview.webrtc.org/1269843005/diff/320001/webrtc/p2p/base/session.h
File webrtc/p2p/base/session.h (right):

https://codereview.webrtc.org/1269843005/diff/320001/webrtc/p2p/base/session....
webrtc/p2p/base/session.h:320: rtc::scoped_refptr<webrtc::DtlsCertificate>
certificate() {
also return by const ref

[hbos, 2015-08-12 08:55:15]

https://codereview.webrtc.org/1269843005/diff/320001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1269843005/diff/320001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:512:
rtc::scoped_refptr<DtlsCertificate> certificate) {
On 2015/08/10 16:10:16, tommi wrote:
> this should be passed by const &

Acknowledged.

https://codereview.webrtc.org/1269843005/diff/320001/webrtc/p2p/base/rawtrans...
File webrtc/p2p/base/rawtransportchannel.h (right):

https://codereview.webrtc.org/1269843005/diff/320001/webrtc/p2p/base/rawtrans...
webrtc/p2p/base/rawtransportchannel.h:149:
rtc::scoped_refptr<webrtc::DtlsCertificate> certificate) override {
On 2015/08/10 16:10:16, tommi wrote:
> const &
> 
> (please fix throughout)

Acknowledged.

https://codereview.webrtc.org/1269843005/diff/320001/webrtc/p2p/base/session.h
File webrtc/p2p/base/session.h (right):

https://codereview.webrtc.org/1269843005/diff/320001/webrtc/p2p/base/session....
webrtc/p2p/base/session.h:320: rtc::scoped_refptr<webrtc::DtlsCertificate>
certificate() {
On 2015/08/10 16:10:54, tommi wrote:
> also return by const ref

Acknowledged.

[Henrik Grunell WebRTC, 2015-08-12 14:46:30]

Another round. Generally, should we add new tests? And/or should we test with
"pre-generated" cert and without by using a parameter?

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/dtlscert...
File talk/app/webrtc/dtlscertificate.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/dtlscert...
talk/app/webrtc/dtlscertificate.cc:37: // date?), and maybe tied to the identity
generation?
Do you plan on fixing the todo before landing? Not sure about time handling in
webrtc, does timeutils.h give any hints?

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/dtlscert...
talk/app/webrtc/dtlscertificate.cc:38: return new
rtc::RefCountedObject<DtlsCertificate>(identity.release(), 0.0);
identity.Pass()

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/dtlscert...
File talk/app/webrtc/dtlscertificate.h (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/dtlscert...
talk/app/webrtc/dtlscertificate.h:2: * libjingle
I have no idea how the copyright header should look like nowadays. can you
verify this is correct?

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/dtlscert...
talk/app/webrtc/dtlscertificate.h:45: rtc::SSLIdentity* identity() const;
Put implementation here since the're simple getters.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:391: bool PeerConnection::InitializeCommon(
InitializeInternal() is slightly better imo.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:450: void
PeerConnection::InitializeSessionAfterInit() {
Maybe RegisterAsIceObserver()?

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.h (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.h:126: bool InitializeCommon(
I think these two can have clearer names. Try describing what they do at a high
level.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.cc:230: PeerConnectionObserver* observer)
{
Keep the dchecks here as well. And in the new function.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.cc:279:
PeerConnectionFactory::CreatePeerConnectionCommon(
This function should be called something so one knows it gets and sets up the
allocator factory to use.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactory.h (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.h:53: virtual
rtc::scoped_refptr<PeerConnectionInterface>
Remove virtual, it's implied from override.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.h:107: PortAllocatorFactoryInterface*
CreatePeerConnectionCommon(
Nit: I think ...Internal() is a slightly better name.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector.cc:706: &(dtlscert->identity()->certificate()));
This is confusing. What's the cert we get from the identity? Obviously not the
|dtlscert|.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector_unittest.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector_unittest.cc:672:
transport->SetCertificate(local_dtlscert);
As we talked about offline, it would be nice to split this CL in three that (1)
adds these new functions further down the chain (that are supposed to be
replaced), (2) starts using the new ones, and (3) removes the old functions. But
I'm OK with a big CL if it's too much work.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:742: port_allocator()->set_candidate_filter(
Was there any reason this was done after creating session desc factory? Is it OK
to do before now?

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1376: bool
WebRtcSession::waiting_for_certificate() const {
Rename to IsWaitingForCertificate since it's not a getter.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1380: rtc::scoped_refptr<DtlsCertificate>
WebRtcSession::get_certificate() const {
Move to header file.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:280: bool InitializeCommon(
Same here as other files, improve naming.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession_unittest.cc:3606: // Verifies that if a
certificate is provided it is the one that will be used.
I think we should run all test with parameters, one for testing with explicitly
generated certificate and one without. It could be extended later on to test
different types of certificates.

https://codereview.webrtc.org/1269843005/diff/380001/talk/libjingle.gyp
File talk/libjingle.gyp (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/libjingle.gyp#newco...
talk/libjingle.gyp:783: 'target_name': 'libjingle_dtls',
Any reason for putting this in its own target?

https://codereview.webrtc.org/1269843005/diff/380001/talk/libjingle_tests.gyp
File talk/libjingle_tests.gyp (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/libjingle_tests.gyp...
talk/libjingle_tests.gyp:149: 'libjingle.gyp:libjingle_peerconnection',
Seems like libjingle_dtls would be enough?

https://codereview.webrtc.org/1269843005/diff/380001/talk/session/media/chann...
File talk/session/media/channel_unittest.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/session/media/chann...
talk/session/media/channel_unittest.cc:124: class ChannelTest : public
testing::Test, public sigslot::has_slots<> {
Should we add new tests?

https://codereview.webrtc.org/1269843005/diff/380001/talk/session/media/media...
File talk/session/media/mediasession_unittest.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/session/media/media...
talk/session/media/mediasession_unittest.cc:479:
TEST_F(MediaSessionDescriptionFactoryTest, TestCreateAudioOffer) {
Should we add new tests?

https://codereview.webrtc.org/1269843005/diff/380001/webrtc/base/base.gyp
File webrtc/base/base.gyp (right):

https://codereview.webrtc.org/1269843005/diff/380001/webrtc/base/base.gyp#new...
webrtc/base/base.gyp:106: '<(webrtc_root)/../talk/libjingle.gyp:libjingle_dtls',
Hmm, should we really depend on anything in libjingle?

[hbos, 2015-08-14 13:37:25]

Patchset #17 (id:420001) has been deleted

[hbos, 2015-08-14 14:09:39]

Addressed most of grunell's comments, but have yet to look at if more
webrtcsession_unittest test cases are needed.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/dtlscert...
File talk/app/webrtc/dtlscertificate.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/dtlscert...
talk/app/webrtc/dtlscertificate.cc:37: // date?), and maybe tied to the identity
generation?
On 2015/08/12 14:46:29, Henrik Grunell (webrtc) wrote:
> Do you plan on fixing the todo before landing? Not sure about time handling in
> webrtc, does timeutils.h give any hints?

That hints for an integer type, but I think blink uses double so maybe a
conversion is needed if so. But considering it's just there as a dummy field for
now I'll delete it until when needed in the future.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/dtlscert...
talk/app/webrtc/dtlscertificate.cc:38: return new
rtc::RefCountedObject<DtlsCertificate>(identity.release(), 0.0);
On 2015/08/12 14:46:29, Henrik Grunell (webrtc) wrote:
> identity.Pass()

The DtlsCertificate constructor takes SSLIdentity*, not scoped_ptr<SSLIdentity>,
so I can't Pass().
The reason for the constructor not taking scoped_ptr<SSLIdentity> is that the
RefCountedObject<T> constructor invokes the DtlsCertificate constructor like so:
T(p1, p2), without using .Pass() (so it does not compile with p1 being
scoped_ptr).
The non-usage of scoped_ptr for our identity is limited to this protected
constructor though and not visible to the outside.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/dtlscert...
File talk/app/webrtc/dtlscertificate.h (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/dtlscert...
talk/app/webrtc/dtlscertificate.h:2: * libjingle
On 2015/08/12 14:46:29, Henrik Grunell (webrtc) wrote:
> I have no idea how the copyright header should look like nowadays. can you
> verify this is correct?

Acknowledged. It's how all the other "Copyright 2015" headers in talk/ look like
that I can find.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/dtlscert...
talk/app/webrtc/dtlscertificate.h:45: rtc::SSLIdentity* identity() const;
On 2015/08/12 14:46:29, Henrik Grunell (webrtc) wrote:
> Put implementation here since the're simple getters.

Done.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:391: bool PeerConnection::InitializeCommon(
On 2015/08/12 14:46:29, Henrik Grunell (webrtc) wrote:
> InitializeInternal() is slightly better imo.

Done.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:450: void
PeerConnection::InitializeSessionAfterInit() {
On 2015/08/12 14:46:29, Henrik Grunell (webrtc) wrote:
> Maybe RegisterAsIceObserver()?

Function not needed anymore.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.h (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.h:126: bool InitializeCommon(
On 2015/08/12 14:46:29, Henrik Grunell (webrtc) wrote:
> I think these two can have clearer names. Try describing what they do at a
high
> level.

Done. I merged the second helper function into the first one because those last
steps did not have to be ordered after session_->Initialize.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.cc:230: PeerConnectionObserver* observer)
{
On 2015/08/12 14:46:29, Henrik Grunell (webrtc) wrote:
> Keep the dchecks here as well. And in the new function.

Done.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.cc:279:
PeerConnectionFactory::CreatePeerConnectionCommon(
On 2015/08/12 14:46:29, Henrik Grunell (webrtc) wrote:
> This function should be called something so one knows it gets and sets up the
> allocator factory to use.

Done.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactory.h (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.h:53: virtual
rtc::scoped_refptr<PeerConnectionInterface>
On 2015/08/12 14:46:29, Henrik Grunell (webrtc) wrote:
> Remove virtual, it's implied from override.

Done.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.h:107: PortAllocatorFactoryInterface*
CreatePeerConnectionCommon(
On 2015/08/12 14:46:29, Henrik Grunell (webrtc) wrote:
> Nit: I think ...Internal() is a slightly better name.

Acknowledged.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector.cc:706: &(dtlscert->identity()->certificate()));
On 2015/08/12 14:46:29, Henrik Grunell (webrtc) wrote:
> This is confusing. What's the cert we get from the identity? Obviously not the
> |dtlscert|.

Yeah, two things called certificate is confusing... Not sure what the best way
is around it though. Perhaps there is a better name than DtlsCertificate or
perhaps certificate() could be called sslcertificate() but I would like to
minimize name changes in this already enormous CL. Hmmm...

SSLIdentity::certificate() gets an SSLCertificate which is a lower level concept
of a certificate (an abstract interface for "wrapping the SSL library's notion
of a certificate", it has various implementations just like there are various
SSLIdentity implementations). The specific implementations of SSLIdentity and
SSLCertificate interact with specific crypto libraries.
This goes to crypto stuff I don't understand.

The DtlsCertificate is a higher level concept of a certificate that just points
to the lower level stuff and is reference counted, not concerned about how the
crypto stuff works. It should be analogous to the RTCCertificate that the
specifications talk about. Something that can be passed around and which
specifies which underlying crypto magic to use.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector_unittest.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector_unittest.cc:672:
transport->SetCertificate(local_dtlscert);
On 2015/08/12 14:46:30, Henrik Grunell (webrtc) wrote:
> As we talked about offline, it would be nice to split this CL in three that
(1)
> adds these new functions further down the chain (that are supposed to be
> replaced), (2) starts using the new ones, and (3) removes the old functions.
But
> I'm OK with a big CL if it's too much work.

If I see this clearly I think adding support for both SSLIdentity* and
scoped_refptr<DtlsCertificate> at the same time with multiple functions in a
series of CLs is not only significantly more work, but would lead to more
difficult code changes and be more error prone as well and would not be a very
"clean" switch.

If the reviewers feel like this CL should be split up I instead suggest
replacing a few of the functions at a time per CL. This should be possible if
done in the correct order.

For example if we have a function call chain like so: A(SSLIdentity*) ->
B(SSLIdentity*) -> C(SSLIdentity*). One could replace A and B to take
scoped_refptr<DtlsCertificate> before updating C in a later CL, because the
DtlsCertificate contains the SSLIdentity* pointer.
Like so: B(scoped_refptr<DtlsCertificate> cert) { ... C(cert->identity(); ... }.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:742: port_allocator()->set_candidate_filter(
On 2015/08/12 14:46:30, Henrik Grunell (webrtc) wrote:
> Was there any reason this was done after creating session desc factory? Is it
OK
> to do before now?

No, looks like a simple variable setter unrelated to the
WebRtcSessionDescriptionFactory stuff. The factory doesn't use the port
allocator. Order should not matter.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1376: bool
WebRtcSession::waiting_for_certificate() const {
On 2015/08/12 14:46:30, Henrik Grunell (webrtc) wrote:
> Rename to IsWaitingForCertificate since it's not a getter.

Done.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:1380: rtc::scoped_refptr<DtlsCertificate>
WebRtcSession::get_certificate() const {
On 2015/08/12 14:46:30, Henrik Grunell (webrtc) wrote:
> Move to header file.

Done.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:280: bool InitializeCommon(
On 2015/08/12 14:46:30, Henrik Grunell (webrtc) wrote:
> Same here as other files, improve naming.

Done.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession_unittest.cc:3606: // Verifies that if a
certificate is provided it is the one that will be used.
On 2015/08/12 14:46:30, Henrik Grunell (webrtc) wrote:
> I think we should run all test with parameters, one for testing with
explicitly
> generated certificate and one without. It could be extended later on to test
> different types of certificates.

Hmm, will think about and address in the next PS...

https://codereview.webrtc.org/1269843005/diff/380001/talk/libjingle.gyp
File talk/libjingle.gyp (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/libjingle.gyp#newco...
talk/libjingle.gyp:783: 'target_name': 'libjingle_dtls',
On 2015/08/12 14:46:30, Henrik Grunell (webrtc) wrote:
> Any reason for putting this in its own target?

Yes, I wrote a comment about this that was hidden by other patch sets:
"dtlscertificate is used by both libjingle_peerconnection and
webrtc/base/base.gyp:rtc_base. I created a new build target to avoid .gyp
issues.

Details as remembered:

Having dtlscertificate in libjingle_peerconnection's sources works for making
libjingle_peerconnection and related stuff compile, but compiling rtc_base would
not work with just that.
Making rtc_base depend on libjingle_peerconnection causes a circular dependency
and does not compile (I drew a graph :) ).
I tried to also add it to rtc_base's sources but then dtlscertificate would be
compiled multiple times causing "defined multiple times" errors. Having
dtlscertificate be in rtc_base's sources alone, and not in
libjingle_peerconnection's sources, compiled locally but that caused a presubmit
error saying something like you're not allowed to have source files that are not
in the .gyp file's folder or subfolders.

This stand-alone build target can be referenced by both libjingle_peerconnection
and rtc_base without the above mentioned issues. Hope that's OK with the gyp
gurus."

https://codereview.webrtc.org/1269843005/diff/380001/talk/libjingle_tests.gyp
File talk/libjingle_tests.gyp (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/libjingle_tests.gyp...
talk/libjingle_tests.gyp:149: 'libjingle.gyp:libjingle_peerconnection',
On 2015/08/12 14:46:30, Henrik Grunell (webrtc) wrote:
> Seems like libjingle_dtls would be enough?

Done. Nice catch.

https://codereview.webrtc.org/1269843005/diff/380001/talk/session/media/chann...
File talk/session/media/channel_unittest.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/session/media/chann...
talk/session/media/channel_unittest.cc:124: class ChannelTest : public
testing::Test, public sigslot::has_slots<> {
On 2015/08/12 14:46:30, Henrik Grunell (webrtc) wrote:
> Should we add new tests?

In this case the swap does not change or extend functionality. The identity ->
certificate change should not change what the tests need to cover.

https://codereview.webrtc.org/1269843005/diff/380001/talk/session/media/media...
File talk/session/media/mediasession_unittest.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/session/media/media...
talk/session/media/mediasession_unittest.cc:479:
TEST_F(MediaSessionDescriptionFactoryTest, TestCreateAudioOffer) {
On 2015/08/12 14:46:30, Henrik Grunell (webrtc) wrote:
> Should we add new tests?

Here, the identity -> certificate change should not change what the tests need
to cover.

https://codereview.webrtc.org/1269843005/diff/380001/webrtc/base/base.gyp
File webrtc/base/base.gyp (right):

https://codereview.webrtc.org/1269843005/diff/380001/webrtc/base/base.gyp#new...
webrtc/base/base.gyp:106: '<(webrtc_root)/../talk/libjingle.gyp:libjingle_dtls',
On 2015/08/12 14:46:30, Henrik Grunell (webrtc) wrote:
> Hmm, should we really depend on anything in libjingle?

Uhm, I don't know...?

This was necessary after DtlsCertificate was used in a lot of places where
SSLIdentity was previously used. Perhaps DtlsCertificate should be in the same
folder as SSLIdentity instead? This alternative would be to move DtlsCertificate
to rtc_base (libjingle already depends on it), place it in of webrtc/base/
instead of talk/app/webrtc/. And to update the namespace to rtc instead of
webrtc.

[hbos, 2015-08-18 08:56:35]

PTAL grunell.

Many unittests in webrtcsession_unittest.cc have been parameterized. Hmh,
perhaps not all of the ones I parameterized needed to be parameterized...

https://codereview.webrtc.org/1269843005/diff/460001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession_unittest.cc (left):

https://codereview.webrtc.org/1269843005/diff/460001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession_unittest.cc:3940: 
Two things wrong with this, which is why I removed it:
- A unittest should not do things that will have consequences outside of the
test being teared down. Quitting the thread has the lasting consequence that the
thread has stopped. If other tests run after it and for example try to do Invoke
they will not succeed. Adding TEST_P's changed the order of some tests and
exposed this issue. (Previously this was always the last test being run).
- Quit() does not look like it is actually process pending messages, it just
marks it stopped and wakes the thread up (but it's already awake?).

With messages not being processed anyway I did not replace this by a message
processing loop.

[Henrik Grunell WebRTC, 2015-08-18 14:25:11]

Looked at your comments and fixes for my last round. Will take another review
round later today.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector.cc:706: &(dtlscert->identity()->certificate()));
On 2015/08/14 14:09:39, hbos wrote:
> On 2015/08/12 14:46:29, Henrik Grunell (webrtc) wrote:
> > This is confusing. What's the cert we get from the identity? Obviously not
the
> > |dtlscert|.
> 
> Yeah, two things called certificate is confusing... Not sure what the best way
> is around it though. Perhaps there is a better name than DtlsCertificate or
> perhaps certificate() could be called sslcertificate() but I would like to
> minimize name changes in this already enormous CL. Hmmm...
> 
> SSLIdentity::certificate() gets an SSLCertificate which is a lower level
concept
> of a certificate (an abstract interface for "wrapping the SSL library's notion
> of a certificate", it has various implementations just like there are various
> SSLIdentity implementations). The specific implementations of SSLIdentity and
> SSLCertificate interact with specific crypto libraries.
> This goes to crypto stuff I don't understand.
> 
> The DtlsCertificate is a higher level concept of a certificate that just
points
> to the lower level stuff and is reference counted, not concerned about how the
> crypto stuff works. It should be analogous to the RTCCertificate that the
> specifications talk about. Something that can be passed around and which
> specifies which underlying crypto magic to use.

OK. If this is clear in the DtlsCertificate class description (in its header), I
think it's fine.

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector_unittest.cc (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector_unittest.cc:672:
transport->SetCertificate(local_dtlscert);
On 2015/08/14 14:09:39, hbos wrote:
> On 2015/08/12 14:46:30, Henrik Grunell (webrtc) wrote:
> > As we talked about offline, it would be nice to split this CL in three that
> (1)
> > adds these new functions further down the chain (that are supposed to be
> > replaced), (2) starts using the new ones, and (3) removes the old functions.
> But
> > I'm OK with a big CL if it's too much work.
> 
> If I see this clearly I think adding support for both SSLIdentity* and
> scoped_refptr<DtlsCertificate> at the same time with multiple functions in a
> series of CLs is not only significantly more work, but would lead to more
> difficult code changes and be more error prone as well and would not be a very
> "clean" switch.
> 
> If the reviewers feel like this CL should be split up I instead suggest
> replacing a few of the functions at a time per CL. This should be possible if
> done in the correct order.
> 
> For example if we have a function call chain like so: A(SSLIdentity*) ->
> B(SSLIdentity*) -> C(SSLIdentity*). One could replace A and B to take
> scoped_refptr<DtlsCertificate> before updating C in a later CL, because the
> DtlsCertificate contains the SSLIdentity* pointer.
> Like so: B(scoped_refptr<DtlsCertificate> cert) { ... C(cert->identity(); ...
}.

Acknowledged.

https://codereview.webrtc.org/1269843005/diff/380001/talk/libjingle.gyp
File talk/libjingle.gyp (right):

https://codereview.webrtc.org/1269843005/diff/380001/talk/libjingle.gyp#newco...
talk/libjingle.gyp:783: 'target_name': 'libjingle_dtls',
On 2015/08/14 14:09:39, hbos wrote:
> On 2015/08/12 14:46:30, Henrik Grunell (webrtc) wrote:
> > Any reason for putting this in its own target?
> 
> Yes, I wrote a comment about this that was hidden by other patch sets:
> "dtlscertificate is used by both libjingle_peerconnection and
> webrtc/base/base.gyp:rtc_base. I created a new build target to avoid .gyp
> issues.
> 
> Details as remembered:
> 
> Having dtlscertificate in libjingle_peerconnection's sources works for making
> libjingle_peerconnection and related stuff compile, but compiling rtc_base
would
> not work with just that.
> Making rtc_base depend on libjingle_peerconnection causes a circular
dependency
> and does not compile (I drew a graph :) ).
> I tried to also add it to rtc_base's sources but then dtlscertificate would be
> compiled multiple times causing "defined multiple times" errors. Having
> dtlscertificate be in rtc_base's sources alone, and not in
> libjingle_peerconnection's sources, compiled locally but that caused a
presubmit
> error saying something like you're not allowed to have source files that are
not
> in the .gyp file's folder or subfolders.
> 
> This stand-alone build target can be referenced by both
libjingle_peerconnection
> and rtc_base without the above mentioned issues. Hope that's OK with the gyp
> gurus."

Acknowledged.

https://codereview.webrtc.org/1269843005/diff/380001/webrtc/base/base.gyp
File webrtc/base/base.gyp (right):

https://codereview.webrtc.org/1269843005/diff/380001/webrtc/base/base.gyp#new...
webrtc/base/base.gyp:106: '<(webrtc_root)/../talk/libjingle.gyp:libjingle_dtls',
On 2015/08/14 14:09:39, hbos wrote:
> On 2015/08/12 14:46:30, Henrik Grunell (webrtc) wrote:
> > Hmm, should we really depend on anything in libjingle?
> 
> Uhm, I don't know...?
> 
> This was necessary after DtlsCertificate was used in a lot of places where
> SSLIdentity was previously used. Perhaps DtlsCertificate should be in the same
> folder as SSLIdentity instead? This alternative would be to move
DtlsCertificate
> to rtc_base (libjingle already depends on it), place it in of webrtc/base/
> instead of talk/app/webrtc/. And to update the namespace to rtc instead of
> webrtc.

I'm not up to date with webrtc-libjingle dependency status. I defer to Tommi or
other reviewer to review this file.

[tommi (sloooow) - chröme, 2015-08-18 14:49:36]

is there a way to break this cl up into smaller more easily reviewable
changelists?

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/dtlscert...
File talk/app/webrtc/dtlscertificate.h (right):

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/dtlscert...
talk/app/webrtc/dtlscertificate.h:49: explicit DtlsCertificate(rtc::SSLIdentity*
identity);
use scoped_ptr here too?

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1380:
rtc::scoped_ptr<webrtc::DtlsIdentityStoreInterface>(nullptr).Pass(),
hmm... could you just pass nullptr here?

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.cc (right):

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:361: if
(!session_->Initialize(factory_->options(), constraints,
return session_->Initialize(...);

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:363: return false;
if we hit this, do we have to undo some of the work already done by
InitializeInternal?

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.cc:382: if
(!session_->Initialize(factory_->options(), constraints,
nit:

return session_->Initialize(...);

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnection.h (right):

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnection.h:61: // This method takes the ownership of
|dtls_identity_service|.
dtls_identity_store?

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactory.h (right):

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.h:66: const
rtc::scoped_refptr<DtlsCertificate>& certificate,
should the certificate be passed in here? I was expecting it to be a part of the
configuration (and then possibly a list/vector).

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/proxy.h
File talk/app/webrtc/proxy.h (right):

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/proxy.h#...
talk/app/webrtc/proxy.h:84: template<typename C, typename M, typename T1,
typename T2, typename T3,
do you want to land this change in a separate cl for simplicity?

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/statscol...
File talk/app/webrtc/statscollector.cc (right):

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/statscol...
talk/app/webrtc/statscollector.cc:703:
rtc::scoped_refptr<webrtc::DtlsCertificate> dtlscert;
nit: dtls_cert

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:566: dtls_enabled_ = true;
what is it initialized to in the ctor?

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.h (right):

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.h:263: rtc::scoped_refptr<DtlsCertificate>
certificate() const {
const scoped_refptr<>&

Can this be called safely from any thread?

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1269843005/diff/480001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:185:
dtls_identity_store_.reset(dtls_identity_store.release());
dtls_identity_store_(dtls_identity_store.Pass()) in the initializer list?  can
dtls_identity_store_ then be made const?

https://codereview.webrtc.org/1269843005/diff/480001/webrtc/base/sslstreamada...
File webrtc/base/sslstreamadapter_unittest.cc (right):

https://codereview.webrtc.org/1269843005/diff/480001/webrtc/base/sslstreamada...
webrtc/base/sslstreamadapter_unittest.cc:168: rtc::KeyType server_key_type =
rtc::KT_DEFAULT) :
nit: no need to change this

https://codereview.webrtc.org/1269843005/diff/480001/webrtc/p2p/base/dtlstran...
File webrtc/p2p/base/dtlstransport.h (right):

https://codereview.webrtc.org/1269843005/diff/480001/webrtc/p2p/base/dtlstran...
webrtc/p2p/base/dtlstransport.h:45: certificate_ = certificate;
would be good to have thread checks for methods that touch this variable.

would also be good to have the implementation in the cc file :-/

[Henrik Grunell WebRTC, 2015-08-18 19:34:25]

On 2015/08/18 14:49:36, tommi wrote:
> is there a way to break this cl up into smaller more easily reviewable
> changelists?

+1
I had that comment before, did you see Henrik's reply? (Patch set #15 I think.)

[hbos, 2015-08-19 08:43:57]

On 2015/08/18 19:34:25, Henrik Grunell (webrtc) wrote:
> On 2015/08/18 14:49:36, tommi wrote:
> > is there a way to break this cl up into smaller more easily reviewable
> > changelists?
> 
> +1
> I had that comment before, did you see Henrik's reply? (Patch set #15 I
think.)

Link to that comment exchange:
https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/statscol...

[Henrik Grunell WebRTC, 2015-08-19 09:04:27]

On 2015/08/19 08:43:57, hbos wrote:
> On 2015/08/18 19:34:25, Henrik Grunell (webrtc) wrote:
> > On 2015/08/18 14:49:36, tommi wrote:
> > > is there a way to break this cl up into smaller more easily reviewable
> > > changelists?
> > 
> > +1
> > I had that comment before, did you see Henrik's reply? (Patch set #15 I
> think.)
> 
> Link to that comment exchange:
>
https://codereview.webrtc.org/1269843005/diff/380001/talk/app/webrtc/statscol...

After talking offline with Henrik B, I think his suggestion in that comment
exchange is OK. Tommi, any input before he goes ahead and splits it that way?

[tommi (sloooow) - chröme, 2015-08-31 17:19:42]

should we close this cl then?

[hbos_chromium, 2015-09-01 08:07:26]

On 2015/08/31 17:19:42, tommi wrote:
> should we close this cl then?

Yes! Not sure why I kept it open after editing it as obsolete, was going to
close it shortly after but forgot.