DtlsIdentityStore[Interface/Impl] updated, DtlsIdentityService to be removed

DtlsIdentityStoreInterface added and the implementation is called DtlsIdentityStoreImpl (previously named without the -Impl bit and without an interface).

DtlsIdentityStoreImpl is updated to take KeyType into account, something which will be relevant after this CL lands:
https://codereview.webrtc.org/1189583002

The DtlsIdentityService[Interface] classes are about to be removed (to be removed when Chromium no longer implements and uses the interface). This was an unnecessary layer of complexity. The FakeIdentityService is now instead a FakeDtlsIdentityStore.
Where a service was previously passed around, a store is now passed around.

Identity generation is now commonly performed using DtlsIdentityStoreInterface. Previously, if a service was not specified, WebRtcSessionDescriptionFactory could fall back on its own generation code. Now, a store has to be provided for generation to occur.

For more information about the steps being taken to land this without breaking Chromium, see referenced bug.

BUG=webrtc:4899
R=magjed@webrtc.org, tommi@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/5e56c5927e097f095aef2e9f7be49fd3d59221e1

[hbos, 2015-06-15 10:10:34]

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

[hbos, 2015-06-15 10:10:34]

The DtlsIdentityStore-related changes of
https://codereview.webrtc.org/1151943005/.

https://codereview.webrtc.org/1176383004/diff/2/talk/app/webrtc/dtlsidentitys...
File talk/app/webrtc/dtlsidentitystore.cc (right):

https://codereview.webrtc.org/1176383004/diff/2/talk/app/webrtc/dtlsidentitys...
talk/app/webrtc/dtlsidentitystore.cc:76: // to generate one.
In the old code, |free_identity_| was checked when RequestIdentity was called
and if we had none then we generated a new one. This has the flaw that if we are
currently generating an identity for |free_identity_|, but we have not finished
yet, the old code would  signal to "unnecessarily" generate another identity.

This seemed like a problem to me and I added store_->ReleaseFreeRSAIdentity here
to abort generating another one, but at the cost of having to use a lock around
|free_(rsa_)identity_|.

As I'm writing this though I realized that generating an extra identity
"unnecessarily" would not have been a practical problem, since we'd want to
queue a preemptive RSA generation in the background as soon as |free_identity_|
was released anyway. The end result being the same as long as |worker_thread_|
!= |signaling_thread_|.

I'm sure you'll object to me adding this lock so we can discuss me changing this
up a bit.

https://codereview.webrtc.org/1176383004/diff/2/talk/app/webrtc/dtlsidentitys...
talk/app/webrtc/dtlsidentitystore.cc:199: // Enqueue the request.
I made it simple by always queuing a request and handling them the same way.
However, ECDA being cheap we could generate those on the signaling thread and
avoid a couple of context switches. (Would still have to do Post though so that
the result is returned async.)

We could also avoid a couple of context switches by checking if we have a
|free_rsa_identity_| already here like the old code.

https://codereview.webrtc.org/1176383004/diff/2/talk/app/webrtc/dtlsidentitys...
File talk/app/webrtc/dtlsidentitystore.h (right):

https://codereview.webrtc.org/1176383004/diff/2/talk/app/webrtc/dtlsidentitys...
talk/app/webrtc/dtlsidentitystore.h:48: class DtlsIdentityRequestObserver :
public rtc::RefCountInterface {
Previously DTLSIdentityRequestObserver in peerconnectionfactor.h

https://codereview.webrtc.org/1176383004/diff/2/talk/app/webrtc/peerconnection.h
File talk/app/webrtc/peerconnection.h (left):

https://codereview.webrtc.org/1176383004/diff/2/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnection.h:164: PeerConnectionObserver* observer);
Was declared but not defined.

[hbos, 2015-06-16 07:26:25]

https://codereview.webrtc.org/1176383004/diff/2/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1176383004/diff/2/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnectioninterface.h:497: // This method takes the
ownership of |dtls_identity_service|.
Should say |dtls_identity_store| now.

Also, this method no longer takes ownership! I should either take ownership to
avoid having to update chromium's calling places or update this comment. Taking
ownership is thus probably easier but I would have to update unit tests to not
double-delete.

[hbos, 2015-06-17 09:27:02]

Patchset #3 (id:30001) has been deleted

[hbos, 2015-06-18 08:43:13]

PTAL tommi and rock on.

https://codereview.webrtc.org/1176383004/diff/2/talk/app/webrtc/dtlsidentitys...
File talk/app/webrtc/dtlsidentitystore.cc (right):

https://codereview.webrtc.org/1176383004/diff/2/talk/app/webrtc/dtlsidentitys...
talk/app/webrtc/dtlsidentitystore.cc:76: // to generate one.
On 2015/06/15 10:10:34, hbos wrote:
> In the old code, |free_identity_| was checked when RequestIdentity was called
> and if we had none then we generated a new one. This has the flaw that if we
are
> currently generating an identity for |free_identity_|, but we have not
finished
> yet, the old code would  signal to "unnecessarily" generate another identity.
> 
> This seemed like a problem to me and I added store_->ReleaseFreeRSAIdentity
here
> to abort generating another one, but at the cost of having to use a lock
around
> |free_(rsa_)identity_|.
> 
> As I'm writing this though I realized that generating an extra identity
> "unnecessarily" would not have been a practical problem, since we'd want to
> queue a preemptive RSA generation in the background as soon as
|free_identity_|
> was released anyway. The end result being the same as long as |worker_thread_|
> != |signaling_thread_|.
> 
> I'm sure you'll object to me adding this lock so we can discuss me changing
this
> up a bit.

In PS4 I changed the implementation quite a bit again...

https://codereview.webrtc.org/1176383004/diff/2/talk/app/webrtc/dtlsidentitys...
talk/app/webrtc/dtlsidentitystore.cc:199: // Enqueue the request.
On 2015/06/15 10:10:34, hbos wrote:
> I made it simple by always queuing a request and handling them the same way.
> However, ECDA being cheap we could generate those on the signaling thread and
> avoid a couple of context switches. (Would still have to do Post though so
that
> the result is returned async.)
> 
> We could also avoid a couple of context switches by checking if we have a
> |free_rsa_identity_| already here like the old code.

Addressed in PS4.

https://codereview.webrtc.org/1176383004/diff/2/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1176383004/diff/2/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnectioninterface.h:497: // This method takes the
ownership of |dtls_identity_service|.
On 2015/06/16 07:26:24, hbos wrote:
> Should say |dtls_identity_store| now.
> 
> Also, this method no longer takes ownership! I should either take ownership to
> avoid having to update chromium's calling places or update this comment.
Taking
> ownership is thus probably easier but I would have to update unit tests to not
> double-delete.

Taking ownership and making it explicit with use of scoped_ptr.

https://codereview.webrtc.org/1176383004/diff/70001/talk/app/webrtc/dtlsident...
File talk/app/webrtc/dtlsidentitystore.h (right):

https://codereview.webrtc.org/1176383004/diff/70001/talk/app/webrtc/dtlsident...
talk/app/webrtc/dtlsidentitystore.h:138: rtc::scoped_ptr<rtc::SSLIdentity>
free_identities_[rtc::KT_LAST];
(This makes it easy to support preemptive generation for other key types than
RSA in the future. Since we only do it for KT_RSA though this doesn't have to be
an array.)

[hbos, 2015-06-18 09:52:13]

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

[hbos, 2015-06-18 09:52:13]

PTAL magjed, maybe you can review and then help me land this? I'm OOO for the
next 1½ weeks.

[tommi, 2015-06-18 13:55:36]

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.cc (right):

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:77: new IdentityResult(key_type_,
identity.release()));
would be better to do identity.Pass() here I think (i.e. change the ctor)

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:78: signaling_thread_->Post(store_,
MSG_GENERATE_IDENTITY_RESULT, msg);
I think we can make the interface between the WorkerTask and store_ a little
cleaner and also avoid requiring a lock for store_.

* Here, don't Post() to store_.  Instead post to the instance itself
(WorkerTask).
* Make sure (DCHECK) that store_ is only touched on the signaling thread.
* In OnStoreDestroyed(), DCHECK that you're on the signaling thread and then
just set store_ to nullptr.
* in WorkerTask::OnMessage, handle MSG_GENERATE_IDENTITY_RESULT, DCHECK you're
on the signaling thread and check if store_ is nullptr.  If it's not null, call
OnIdentityGenerated() on the store_ with the results and delete the message.

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:110: // been so.
don't think we need this lock

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:113: rtc::KeyType key_type_;
const?

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:125: SignalDestroyed();
We should always be on the signaling thread here, right?

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:218:
free_identities_[key_type].reset(identity.release());
nit: swap()?

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:235: && (key_type == rtc::KT_RSA)        
// Only necessary for RSA.
&& at the end of the previous line

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.h (right):

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:56: virtual void OnSuccessWithIdentityObj(
Can this method not be called OnSuccess too?

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:69: typedef
rtc::scoped_refptr<webrtc::DtlsIdentityRequestObserver>
I don't think you need the webrtc:: prefix here.  I wonder if we really need
this typedef...

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:80: rtc::KeyType key_type,
ScopedRefPtrObserver observer) = 0;
const ScopedRefPtrObserver& observer
(or const rtc::scoped_refptr<DtlsIdentityRequestObserver>&)

same elsewhere

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:90: static const char* common_name_;
should this be common_name_[] ?
Actually, since it's a global static constant, it should probably be
kCommonName[] or kIdentityName[] which I find slightly more descriptive.
Does it even have to be a member of the class? Can we have be just a static in
the cc file?

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:122: IdentityResult(rtc::KeyType key_type,
rtc::SSLIdentity* identity)
pass identity ownership via scoped_ptr

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:128: typedef
rtc::ScopedMessageData<IdentityResult> IdentityResultMessageData;
empty line above this one

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:138: // Only touch on the
|signaling_thred_|.
signaling_thread_

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:139: std::list<ScopedRefPtrObserver>
request_observers_[rtc::KT_LAST];
what about grouping these variables into a struct and then have rtc::KT_LAST
member variables of that struct?
It'll make loops etc a little nicer and if we need to pass this around.

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

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.h:58:
rtc::scoped_ptr<DtlsIdentityStoreInterface> dtls_identity_store,
const &

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactoryproxy.h (right):

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactoryproxy.h:66:
rtc::scoped_refptr<PeerConnectionInterface> CreatePeerConnection_ot(
Since you're overriding this method, you might as well use the proper parameter
names (same above)

[hbos, 2015-06-18 15:45:48]

I addressed almost all of the comments. Vacation mode activate!

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.cc (right):

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:77: new IdentityResult(key_type_,
identity.release()));
On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> would be better to do identity.Pass() here I think (i.e. change the ctor)

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:78: signaling_thread_->Post(store_,
MSG_GENERATE_IDENTITY_RESULT, msg);
On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> I think we can make the interface between the WorkerTask and store_ a little
> cleaner and also avoid requiring a lock for store_.
> 
> * Here, don't Post() to store_.  Instead post to the instance itself
> (WorkerTask).
> * Make sure (DCHECK) that store_ is only touched on the signaling thread.
> * In OnStoreDestroyed(), DCHECK that you're on the signaling thread and then
> just set store_ to nullptr.
> * in WorkerTask::OnMessage, handle MSG_GENERATE_IDENTITY_RESULT, DCHECK you're
> on the signaling thread and check if store_ is nullptr.  If it's not null,
call
> OnIdentityGenerated() on the store_ with the results and delete the message.

<This has not been addressed yet>

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:110: // been so.
On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> don't think we need this lock

<This has not been addressed yet>

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:113: rtc::KeyType key_type_;
On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> const?

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:125: SignalDestroyed();
On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> We should always be on the signaling thread here, right?

I think so, want a DCHECK thread check?
<This has not been addressed yet>

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:218:
free_identities_[key_type].reset(identity.release());
On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> nit: swap()?

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:235: && (key_type == rtc::KT_RSA)        
// Only necessary for RSA.
On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> && at the end of the previous line

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.h (right):

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:56: virtual void OnSuccessWithIdentityObj(
On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> Can this method not be called OnSuccess too?

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:69: typedef
rtc::scoped_refptr<webrtc::DtlsIdentityRequestObserver>
On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> I don't think you need the webrtc:: prefix here.  I wonder if we really need
> this typedef...

We don't but some lines get ridiculously long and I end up breaking the 80 char
limit. I'll remove it though, it's not clear from where it's defined when used
in child classes in other files.

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:80: rtc::KeyType key_type,
ScopedRefPtrObserver observer) = 0;
On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> const ScopedRefPtrObserver& observer
> (or const rtc::scoped_refptr<DtlsIdentityRequestObserver>&)
> 
> same elsewhere

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:90: static const char* common_name_;
On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> should this be common_name_[] ?
> Actually, since it's a global static constant, it should probably be
> kCommonName[] or kIdentityName[] which I find slightly more descriptive.
> Does it even have to be a member of the class? Can we have be just a static in
> the cc file?

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:122: IdentityResult(rtc::KeyType key_type,
rtc::SSLIdentity* identity)
On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> pass identity ownership via scoped_ptr

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:128: typedef
rtc::ScopedMessageData<IdentityResult> IdentityResultMessageData;
On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> empty line above this one

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:138: // Only touch on the
|signaling_thred_|.
On 2015/06/18 13:55:36, tommi (webrtc) wrote:
> signaling_thread_

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:139: std::list<ScopedRefPtrObserver>
request_observers_[rtc::KT_LAST];
On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> what about grouping these variables into a struct and then have rtc::KT_LAST
> member variables of that struct?
> It'll make loops etc a little nicer and if we need to pass this around.

Acknowledged.

[hbos, 2015-07-02 09:32:23]

PTAL tommi

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.cc (right):

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:78: signaling_thread_->Post(store_,
MSG_GENERATE_IDENTITY_RESULT, msg);
On 2015/06/18 15:45:48, hbos wrote:
> On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> > I think we can make the interface between the WorkerTask and store_ a little
> > cleaner and also avoid requiring a lock for store_.
> > 
> > * Here, don't Post() to store_.  Instead post to the instance itself
> > (WorkerTask).
> > * Make sure (DCHECK) that store_ is only touched on the signaling thread.
> > * In OnStoreDestroyed(), DCHECK that you're on the signaling thread and then
> > just set store_ to nullptr.
> > * in WorkerTask::OnMessage, handle MSG_GENERATE_IDENTITY_RESULT, DCHECK
you're
> > on the signaling thread and check if store_ is nullptr.  If it's not null,
> call
> > OnIdentityGenerated() on the store_ with the results and delete the message.
> 
> <This has not been addressed yet>

Done!

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:110: // been so.
On 2015/06/18 15:45:48, hbos wrote:
> On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> > don't think we need this lock
> 
> <This has not been addressed yet>

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/110001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:125: SignalDestroyed();
On 2015/06/18 15:45:47, hbos wrote:
> On 2015/06/18 13:55:35, tommi (webrtc) wrote:
> > We should always be on the signaling thread here, right?
> 
> I think so, want a DCHECK thread check?
> <This has not been addressed yet>

Added DCHECK.

[hbos, 2015-07-02 09:35:22]

Patchset #8 (id:150001) has been deleted

[tommi, 2015-07-02 11:23:00]

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.cc (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:68: void GenerateIdentity_w() {
can we dcheck thread correctness?

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:70: LOG(LS_INFO) << "Generating identity.
Key type (TODO(hbos): should use): "
will you remove this before committing?

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:123:
DtlsIdentityStoreImpl::DtlsIdentityStoreImpl(rtc::Thread* signaling_thread,
if the object is always constructed on a known thread (e.g. signaling thread),
then it would be good to dcheck that

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:139: // Only necessary for RSA.
I wonder if we should skip doing this at some point... a future topic I guess.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:207:
++request_info_[key_type].gen_in_progress_counts_;
nit: do this after or before using |msg|

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:218:
rtc::scoped_refptr<webrtc::DtlsIdentityRequestObserver> observer = nullptr;
initialization to nullptr not necessary

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:244: (key_type == rtc::KT_RSA) &&          
// Only necessary for RSA.
parens not needed

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.h (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:51: // TODO(jiayl): Unify the OnSuccess
method once Chrome code is updated.
can you assign this todo to you?

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:71: virtual void Initialize() = 0;
can we initialize in the ctor or is there a reason we need two steps?

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:77: const
rtc::scoped_refptr<webrtc::DtlsIdentityRequestObserver>&
we're already in the webrtc namespace, right?

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:78: observer) = 0;
4 space indent?

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:97: const
rtc::scoped_refptr<webrtc::DtlsIdentityRequestObserver>&
already in webrtc

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:138: RequestInfo() : request_observers_(),
nit: could probably do this in two lines

RequestInfo()
    : request_observers_(), gen_in_progress_counts_(0), free_identity_() {}

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

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.h:44: RefCountedDtlsIdentityStore;
4 spaces

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactoryproxy.h (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactoryproxy.h:50: a1, a2, a3, a4.release(), a5));
not .Passed() or hmm... I don't suppose we have base::Passed()...

I wonder if we could use templates instead for this and override it for
scoped_ptr... will think about it some more.

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

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:506: // TODO(mallinath) : Remove below
versions after clients are updated
can this be done now?  if not, can you take over this todo?

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/test/fak...
File talk/app/webrtc/test/fakedtlsidentitystore.h (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/test/fak...
talk/app/webrtc/test/fakedtlsidentitystore.h:89: if (should_fail_) {
nit:
rtc::Thread::Current()->Post(this, should_fail_ ? MSG_FAILURE : MSG_SUCCESS,
msg);

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

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:95: // TODO(jiayl): remove the
dependency on session once b/10226852 is fixed.
this bug has been closed now... can you check the bug status and possibly remove
this todo?

[hbos, 2015-07-02 12:28:28]

PTAL again, tommi

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.cc (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:68: void GenerateIdentity_w() {
On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> can we dcheck thread correctness?

That probably requires a pointer to the worker thread only to be used for the
DCHECK (can't check "not on signaling thread" because it's possible that
signaling thread == worker thread). You think I should add a DCHECK like that
anyway?

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:70: LOG(LS_INFO) << "Generating identity.
Key type (TODO(hbos): should use): "
On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> will you remove this before committing?

Think I should remove the LOG even if this lands before torbjorng's CL?

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:123:
DtlsIdentityStoreImpl::DtlsIdentityStoreImpl(rtc::Thread* signaling_thread,
On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> if the object is always constructed on a known thread (e.g. signaling thread),
> then it would be good to dcheck that

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:139: // Only necessary for RSA.
On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> I wonder if we should skip doing this at some point... a future topic I guess.

Yeah, this is especially a bad idea if we use 2048 bit RSA. For really expensive
generation we should either not do it at all or use a different thread than the
worker thread. Don't want to block it for seconds...
Future discussion.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:207:
++request_info_[key_type].gen_in_progress_counts_;
On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> nit: do this after or before using |msg|

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:218:
rtc::scoped_refptr<webrtc::DtlsIdentityRequestObserver> observer = nullptr;
On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> initialization to nullptr not necessary

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:244: (key_type == rtc::KT_RSA) &&          
// Only necessary for RSA.
On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> parens not needed

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.h (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:51: // TODO(jiayl): Unify the OnSuccess
method once Chrome code is updated.
On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> can you assign this todo to you?

Sure

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:71: virtual void Initialize() = 0;
On 2015/07/02 11:23:00, tommi (webrtc) wrote:
> can we initialize in the ctor or is there a reason we need two steps?

No reason that I can tell, removing.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:77: const
rtc::scoped_refptr<webrtc::DtlsIdentityRequestObserver>&
On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> we're already in the webrtc namespace, right?

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:78: observer) = 0;
On 2015/07/02 11:23:00, tommi (webrtc) wrote:
> 4 space indent?

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:97: const
rtc::scoped_refptr<webrtc::DtlsIdentityRequestObserver>&
On 2015/07/02 11:23:00, tommi (webrtc) wrote:
> already in webrtc

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:138: RequestInfo() : request_observers_(),
On 2015/07/02 11:23:00, tommi (webrtc) wrote:
> nit: could probably do this in two lines
> 
> RequestInfo()
>     : request_observers_(), gen_in_progress_counts_(0), free_identity_() {}

Acknowledged.

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

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactory.h:44: RefCountedDtlsIdentityStore;
On 2015/07/02 11:23:00, tommi (webrtc) wrote:
> 4 spaces

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactoryproxy.h (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactoryproxy.h:50: a1, a2, a3, a4.release(), a5));
On 2015/07/02 11:23:00, tommi (webrtc) wrote:
> not .Passed() or hmm... I don't suppose we have base::Passed()...
> 
> I wonder if we could use templates instead for this and override it for
> scoped_ptr... will think about it some more.

This code does make me feel dirty but I'm not sure what the alternatives are...

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

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:506: // TODO(mallinath) : Remove below
versions after clients are updated
On 2015/07/02 11:23:00, tommi (webrtc) wrote:
> can this be done now?  if not, can you take over this todo?

Acknowledged. Also "takes the ownership" comment unnecessary when we use
scoped_ptr, that makes it explicit anyway.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/test/fak...
File talk/app/webrtc/test/fakedtlsidentitystore.h (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/test/fak...
talk/app/webrtc/test/fakedtlsidentitystore.h:89: if (should_fail_) {
On 2015/07/02 11:23:00, tommi (webrtc) wrote:
> nit:
> rtc::Thread::Current()->Post(this, should_fail_ ? MSG_FAILURE : MSG_SUCCESS,
> msg);

Acknowledged.

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

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:95: // TODO(jiayl): remove the
dependency on session once b/10226852 is fixed.
It has been moved to https://code.google.com/p/webrtc/issues/detail?id=2264. I
don't see it having been fixed?
I could update the TODO to reference the webrtc tracker instead, what to type
instead of b/...?

[tommi, 2015-07-09 16:13:57]

looks good (nice work!). minor request for ThreadChecker.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.cc (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:68: void GenerateIdentity_w() {
On 2015/07/02 12:28:27, hbos wrote:
> On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> > can we dcheck thread correctness?
> 
> That probably requires a pointer to the worker thread only to be used for the
> DCHECK (can't check "not on signaling thread" because it's possible that
> signaling thread == worker thread). You think I should add a DCHECK like that
> anyway?

What about using ThreadChecker?

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:70: LOG(LS_INFO) << "Generating identity.
Key type (TODO(hbos): should use): "
On 2015/07/02 12:28:27, hbos wrote:
> On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> > will you remove this before committing?
> 
> Think I should remove the LOG even if this lands before torbjorng's CL?

nah, OK to keep in until then.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:139: // Only necessary for RSA.
On 2015/07/02 12:28:27, hbos wrote:
> On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> > I wonder if we should skip doing this at some point... a future topic I
guess.
> 
> Yeah, this is especially a bad idea if we use 2048 bit RSA. For really
expensive
> generation we should either not do it at all or use a different thread than
the
> worker thread. Don't want to block it for seconds...
> Future discussion.

Acknowledged.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactoryproxy.h (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactoryproxy.h:50: a1, a2, a3, a4.release(), a5));
On 2015/07/02 12:28:28, hbos wrote:
> On 2015/07/02 11:23:00, tommi (webrtc) wrote:
> > not .Passed() or hmm... I don't suppose we have base::Passed()...
> > 
> > I wonder if we could use templates instead for this and override it for
> > scoped_ptr... will think about it some more.
> 
> This code does make me feel dirty but I'm not sure what the alternatives
are...

Agreed. Can you leave a TODO here for you and/or me to add support for
scoped_ptr?

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

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:95: // TODO(jiayl): remove the
dependency on session once b/10226852 is fixed.
On 2015/07/02 12:28:28, hbos wrote:
> It has been moved to https://code.google.com/p/webrtc/issues/detail?id=2264. I
> don't see it having been fixed?
> I could update the TODO to reference the webrtc tracker instead, what to type
> instead of b/...?

just write bug 2264 :)

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

https://codereview.webrtc.org/1176383004/diff/210001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:505: // TODO(mallinath) : Remove below
versions after clients are updated
nit: assign to you? (or me)

[hbos, 2015-07-27 13:29:00]

PTAL magjed, tommi

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.cc (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:68: void GenerateIdentity_w() {
On 2015/07/09 16:13:56, tommi (webrtc) wrote:
> On 2015/07/02 12:28:27, hbos wrote:
> > On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> > > can we dcheck thread correctness?
> > 
> > That probably requires a pointer to the worker thread only to be used for
the
> > DCHECK (can't check "not on signaling thread" because it's possible that
> > signaling thread == worker thread). You think I should add a DCHECK like
that
> > anyway?
> 
> What about using ThreadChecker?

Hmh. But it's constructed on the signaling thread and then the OnMessage is
invoked on both the worker thread and the signaling thread (with appropriate
DCHECKs for the signaling thread cases). But the thing that is being invoked on
the worker thread, MSG_GENERATE_IDENTITY/GenerateIdentity_w is only invoked
once. The thread checker would always pass the first time (attaching to the
current thread) making this quite pointless.

If you want there to be a worker thread thread check I still think we need to
pass the worker thread pointer for the sole purpose of this DCHECK.

Up to you if I should add this, I don't have a strong opinion either way.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/peerconn...
File talk/app/webrtc/peerconnectionfactoryproxy.h (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectionfactoryproxy.h:50: a1, a2, a3, a4.release(), a5));
On 2015/07/09 16:13:56, tommi (webrtc) wrote:
> On 2015/07/02 12:28:28, hbos wrote:
> > On 2015/07/02 11:23:00, tommi (webrtc) wrote:
> > > not .Passed() or hmm... I don't suppose we have base::Passed()...
> > > 
> > > I wonder if we could use templates instead for this and override it for
> > > scoped_ptr... will think about it some more.
> > 
> > This code does make me feel dirty but I'm not sure what the alternatives
> are...
> 
> Agreed. Can you leave a TODO here for you and/or me to add support for
> scoped_ptr?

Acknowledged.

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

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:95: // TODO(jiayl): remove the
dependency on session once b/10226852 is fixed.
On 2015/07/09 16:13:56, tommi (webrtc) wrote:
> On 2015/07/02 12:28:28, hbos wrote:
> > It has been moved to https://code.google.com/p/webrtc/issues/detail?id=2264.
I
> > don't see it having been fixed?
> > I could update the TODO to reference the webrtc tracker instead, what to
type
> > instead of b/...?
> 
> just write bug 2264 :)

Acknowledged.

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

https://codereview.webrtc.org/1176383004/diff/210001/talk/app/webrtc/peerconn...
talk/app/webrtc/peerconnectioninterface.h:505: // TODO(mallinath) : Remove below
versions after clients are updated
On 2015/07/09 16:13:57, tommi (webrtc) wrote:
> nit: assign to you? (or me)

Acknowledged.

[magjed_webrtc, 2015-07-29 09:31:57]

I only have a shallow understanding of this, but it lgtm.

https://codereview.webrtc.org/1176383004/diff/250001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.h (right):

https://codereview.webrtc.org/1176383004/diff/250001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:134:
std::list<rtc::scoped_refptr<DtlsIdentityRequestObserver>>
Why not use a queue?

[tommi, 2015-08-04 10:47:20]

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.cc (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:68: void GenerateIdentity_w() {
On 2015/07/27 13:29:00, hbos wrote:
> On 2015/07/09 16:13:56, tommi (webrtc) wrote:
> > On 2015/07/02 12:28:27, hbos wrote:
> > > On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> > > > can we dcheck thread correctness?
> > > 
> > > That probably requires a pointer to the worker thread only to be used for
> the
> > > DCHECK (can't check "not on signaling thread" because it's possible that
> > > signaling thread == worker thread). You think I should add a DCHECK like
> that
> > > anyway?
> > 
> > What about using ThreadChecker?
> 
> Hmh. But it's constructed on the signaling thread and then the OnMessage is
> invoked on both the worker thread and the signaling thread (with appropriate
> DCHECKs for the signaling thread cases). But the thing that is being invoked
on
> the worker thread, MSG_GENERATE_IDENTITY/GenerateIdentity_w is only invoked
> once. The thread checker would always pass the first time (attaching to the
> current thread) making this quite pointless.
> 
> If you want there to be a worker thread thread check I still think we need to
> pass the worker thread pointer for the sole purpose of this DCHECK.
> 
> Up to you if I should add this, I don't have a strong opinion either way.

If I'm parsing GenerateIdentity() correctly, then this function can be called
more than once, so a dcheck wouldn't be completely pointless imo.
Will the worker and signaling threads ever be the same thread?  Maybe we could
compromise and check for !signaling?

[hbos, 2015-08-04 11:38:49]

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.cc (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:68: void GenerateIdentity_w() {
On 2015/08/04 10:47:20, tommi (webrtc) wrote:
> On 2015/07/27 13:29:00, hbos wrote:
> > On 2015/07/09 16:13:56, tommi (webrtc) wrote:
> > > On 2015/07/02 12:28:27, hbos wrote:
> > > > On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> > > > > can we dcheck thread correctness?
> > > > 
> > > > That probably requires a pointer to the worker thread only to be used
for
> > the
> > > > DCHECK (can't check "not on signaling thread" because it's possible that
> > > > signaling thread == worker thread). You think I should add a DCHECK like
> > that
> > > > anyway?
> > > 
> > > What about using ThreadChecker?
> > 
> > Hmh. But it's constructed on the signaling thread and then the OnMessage is
> > invoked on both the worker thread and the signaling thread (with appropriate
> > DCHECKs for the signaling thread cases). But the thing that is being invoked
> on
> > the worker thread, MSG_GENERATE_IDENTITY/GenerateIdentity_w is only invoked
> > once. The thread checker would always pass the first time (attaching to the
> > current thread) making this quite pointless.
> > 
> > If you want there to be a worker thread thread check I still think we need
to
> > pass the worker thread pointer for the sole purpose of this DCHECK.
> > 
> > Up to you if I should add this, I don't have a strong opinion either way.
> 
> If I'm parsing GenerateIdentity() correctly, then this function can be called
> more than once, so a dcheck wouldn't be completely pointless imo.
> Will the worker and signaling threads ever be the same thread?  Maybe we could
> compromise and check for !signaling?

I don't think so? While we may do multiple identity generations, each
GenerateIdentity() call constructs a new WorkerTask. There is only one post to
do MSG_GENERATE_IDENTITY and its there. When the WorkerTask has finished it
deletes itself.

As for if worker thread is ever == signaling thread, while the class was
designed to be able to handle that case and I kept that design aspect, by
looking at the code I believe that both in practice and in unit test, worker
thread is always != signaling thread.
I can change this design and add DCHECK(signaling_thread_ != worker_thread_) in
the store constructor and then DCHECK(!signaling_thread) here. You think I
should?

https://codereview.webrtc.org/1176383004/diff/250001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.h (right):

https://codereview.webrtc.org/1176383004/diff/250001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.h:134:
std::list<rtc::scoped_refptr<DtlsIdentityRequestObserver>>
On 2015/07/29 09:31:57, magjed_webrtc wrote:
> Why not use a queue?

At one point I thought I needed to iterate it so I made it a list instead.
Switched back to a queue, don't think it matters.

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

https://codereview.webrtc.org/1176383004/diff/270001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:128: IDENTITY_FAILED,
(IDENTITY_FAILED no longer used, will remove)

[hbos, 2015-08-04 12:16:19]

A question for ya, tommi.

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
File talk/app/webrtc/dtlsidentitystore.cc (right):

https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
talk/app/webrtc/dtlsidentitystore.cc:68: void GenerateIdentity_w() {
On 2015/08/04 11:38:48, hbos wrote:
> On 2015/08/04 10:47:20, tommi (webrtc) wrote:
> > On 2015/07/27 13:29:00, hbos wrote:
> > > On 2015/07/09 16:13:56, tommi (webrtc) wrote:
> > > > On 2015/07/02 12:28:27, hbos wrote:
> > > > > On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> > > > > > can we dcheck thread correctness?
> > > > > 
> > > > > That probably requires a pointer to the worker thread only to be used
> for
> > > the
> > > > > DCHECK (can't check "not on signaling thread" because it's possible
that
> > > > > signaling thread == worker thread). You think I should add a DCHECK
like
> > > that
> > > > > anyway?
> > > > 
> > > > What about using ThreadChecker?
> > > 
> > > Hmh. But it's constructed on the signaling thread and then the OnMessage
is
> > > invoked on both the worker thread and the signaling thread (with
appropriate
> > > DCHECKs for the signaling thread cases). But the thing that is being
invoked
> > on
> > > the worker thread, MSG_GENERATE_IDENTITY/GenerateIdentity_w is only
invoked
> > > once. The thread checker would always pass the first time (attaching to
the
> > > current thread) making this quite pointless.
> > > 
> > > If you want there to be a worker thread thread check I still think we need
> to
> > > pass the worker thread pointer for the sole purpose of this DCHECK.
> > > 
> > > Up to you if I should add this, I don't have a strong opinion either way.
> > 
> > If I'm parsing GenerateIdentity() correctly, then this function can be
called
> > more than once, so a dcheck wouldn't be completely pointless imo.
> > Will the worker and signaling threads ever be the same thread?  Maybe we
could
> > compromise and check for !signaling?
> 
> I don't think so? While we may do multiple identity generations, each
> GenerateIdentity() call constructs a new WorkerTask. There is only one post to
> do MSG_GENERATE_IDENTITY and its there. When the WorkerTask has finished it
> deletes itself.
> 
> As for if worker thread is ever == signaling thread, while the class was
> designed to be able to handle that case and I kept that design aspect, by
> looking at the code I believe that both in practice and in unit test, worker
> thread is always != signaling thread.
> I can change this design and add DCHECK(signaling_thread_ != worker_thread_)
in
> the store constructor and then DCHECK(!signaling_thread) here. You think I
> should?

Actually, there may be some test code that use the same thread in both cases,
not entirely sure. Can we do one of the following:
a) I enforce worker_thread_ != signaling_thread_ in a separate CL and wait with
adding a !signaling DCHECK until then.
b) I pass along the worker_thread_ pointer and add that type of DCHECK instead.
c) We don't add this DCHECK at all. YOLO.

[tommi, 2015-08-04 12:28:26]

On 2015/08/04 12:16:19, hbos wrote:
> A question for ya, tommi.
> 
>
https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
> File talk/app/webrtc/dtlsidentitystore.cc (right):
> 
>
https://codereview.webrtc.org/1176383004/diff/170001/talk/app/webrtc/dtlsiden...
> talk/app/webrtc/dtlsidentitystore.cc:68: void GenerateIdentity_w() {
> On 2015/08/04 11:38:48, hbos wrote:
> > On 2015/08/04 10:47:20, tommi (webrtc) wrote:
> > > On 2015/07/27 13:29:00, hbos wrote:
> > > > On 2015/07/09 16:13:56, tommi (webrtc) wrote:
> > > > > On 2015/07/02 12:28:27, hbos wrote:
> > > > > > On 2015/07/02 11:22:59, tommi (webrtc) wrote:
> > > > > > > can we dcheck thread correctness?
> > > > > > 
> > > > > > That probably requires a pointer to the worker thread only to be
used
> > for
> > > > the
> > > > > > DCHECK (can't check "not on signaling thread" because it's possible
> that
> > > > > > signaling thread == worker thread). You think I should add a DCHECK
> like
> > > > that
> > > > > > anyway?
> > > > > 
> > > > > What about using ThreadChecker?
> > > > 
> > > > Hmh. But it's constructed on the signaling thread and then the OnMessage
> is
> > > > invoked on both the worker thread and the signaling thread (with
> appropriate
> > > > DCHECKs for the signaling thread cases). But the thing that is being
> invoked
> > > on
> > > > the worker thread, MSG_GENERATE_IDENTITY/GenerateIdentity_w is only
> invoked
> > > > once. The thread checker would always pass the first time (attaching to
> the
> > > > current thread) making this quite pointless.
> > > > 
> > > > If you want there to be a worker thread thread check I still think we
need
> > to
> > > > pass the worker thread pointer for the sole purpose of this DCHECK.
> > > > 
> > > > Up to you if I should add this, I don't have a strong opinion either
way.
> > > 
> > > If I'm parsing GenerateIdentity() correctly, then this function can be
> called
> > > more than once, so a dcheck wouldn't be completely pointless imo.
> > > Will the worker and signaling threads ever be the same thread?  Maybe we
> could
> > > compromise and check for !signaling?
> > 
> > I don't think so? While we may do multiple identity generations, each
> > GenerateIdentity() call constructs a new WorkerTask. There is only one post
to
> > do MSG_GENERATE_IDENTITY and its there. When the WorkerTask has finished it
> > deletes itself.
> > 
> > As for if worker thread is ever == signaling thread, while the class was
> > designed to be able to handle that case and I kept that design aspect, by
> > looking at the code I believe that both in practice and in unit test, worker
> > thread is always != signaling thread.
> > I can change this design and add DCHECK(signaling_thread_ != worker_thread_)
> in
> > the store constructor and then DCHECK(!signaling_thread) here. You think I
> > should?
> 
> Actually, there may be some test code that use the same thread in both cases,
> not entirely sure. Can we do one of the following:
> a) I enforce worker_thread_ != signaling_thread_ in a separate CL and wait
with
> adding a !signaling DCHECK until then.
> b) I pass along the worker_thread_ pointer and add that type of DCHECK
instead.
> c) We don't add this DCHECK at all. YOLO.

OK, let's go for option c then for now.  lgtm

[hbos, 2015-08-11 08:33:33]

Committed patchset #14 (id:290001) manually as
5e56c5927e097f095aef2e9f7be49fd3d59221e1 (presubmit successful).