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

talk/app/webrtc/dtlsidentitystore.h

Index: talk/app/webrtc/dtlsidentitystore.h
diff --git a/talk/app/webrtc/dtlsidentitystore.h b/talk/app/webrtc/dtlsidentitystore.h
index b2a797462fbe43b747070db0223fca07775aae05..0ea6c521d48ef53d5040d42e7348bb29afe1e123 100644
--- a/talk/app/webrtc/dtlsidentitystore.h
+++ b/talk/app/webrtc/dtlsidentitystore.h
@@ -28,66 +28,117 @@
 #ifndef TALK_APP_WEBRTC_DTLSIDENTITYSTORE_H_
 #define TALK_APP_WEBRTC_DTLSIDENTITYSTORE_H_
 
-#include <queue>
+#include <list>
 #include <string>
 
-#include "talk/app/webrtc/peerconnectioninterface.h"
 #include "webrtc/base/messagehandler.h"
 #include "webrtc/base/messagequeue.h"
+#include "webrtc/base/refcount.h"
 #include "webrtc/base/scoped_ptr.h"
 #include "webrtc/base/scoped_ref_ptr.h"
+#include "webrtc/base/sslidentity.h"
+#include "webrtc/base/thread.h"
 
 namespace webrtc {
-class DTLSIdentityRequestObserver;
+
 class SSLIdentity;
 class Thread;
 
-// This class implements an in-memory DTLS identity store, which generates the
-// DTLS identity on the worker thread.
+// Used to receive callbacks of DTLS identity requests.
+class DtlsIdentityRequestObserver : public rtc::RefCountInterface {
+ public:
+  virtual void OnFailure(int error) = 0;
+  // TODO(jiayl): Unify the OnSuccess method once Chrome code is updated.
+  virtual void OnSuccess(const std::string& der_cert,
+                         const std::string& der_private_key) = 0;
+  // |identity| is a scoped_ptr because rtc::SSLIdentity is not copyable and the
+  // client has to get the ownership of the object to make use of it.
+  virtual void OnSuccessWithIdentityObj(

[tommi, 2015/06/18 13:55:35]

Can this method not be called OnSuccess too?

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

Acknowledged.

+      rtc::scoped_ptr<rtc::SSLIdentity> identity) = 0;
+
+ protected:
+  virtual ~DtlsIdentityRequestObserver() {}
+};
+
+// This interface defines an in-memory DTLS identity store, which generates DTLS
+// identities.
 // APIs calls must be made on the signaling thread and the callbacks are also
 // called on the signaling thread.
-class DtlsIdentityStore : public rtc::MessageHandler {
+class DtlsIdentityStoreInterface {
  public:
-  static const char kIdentityName[];
+  typedef rtc::scoped_refptr<webrtc::DtlsIdentityRequestObserver>

[tommi, 2015/06/18 13:55:35]

I don't think you need the webrtc:: prefix here.  I wonder if we really need
this typedef...

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

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.

+      ScopedRefPtrObserver;
 
-  DtlsIdentityStore(rtc::Thread* signaling_thread,
-                    rtc::Thread* worker_thread);
-  virtual ~DtlsIdentityStore();
+  virtual ~DtlsIdentityStoreInterface() { }
 
-  // Initialize will start generating the free identity in the background.
-  void Initialize();
+  // Initializes the store.
+  virtual void Initialize() = 0;
 
   // The |observer| will be called when the requested identity is ready, or when
   // identity generation fails.
-  void RequestIdentity(webrtc::DTLSIdentityRequestObserver* observer);
+  virtual void RequestIdentity(
+      rtc::KeyType key_type, ScopedRefPtrObserver observer) = 0;

[tommi, 2015/06/18 13:55:35]

const ScopedRefPtrObserver& observer
(or const rtc::scoped_refptr<DtlsIdentityRequestObserver>&)

same elsewhere

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

Acknowledged.

+};
+
+// The standard implementation of DtlsIdentityStoreInterface.
+// Identity generation is performed on the worker thread.
+class DtlsIdentityStoreImpl : public DtlsIdentityStoreInterface,
+                              public rtc::MessageHandler {
+ public:
+  // Passed to SSLIdentity::Generate, "WebRTC". Used for the certificates'
+  // subject and issuer name.
+  static const char* common_name_;

[tommi, 2015/06/18 13:55:35]

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?

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

Acknowledged.

+
+  DtlsIdentityStoreImpl(rtc::Thread* signaling_thread,
+                        rtc::Thread* worker_thread);
+  ~DtlsIdentityStoreImpl() override;
+
+  // webrtc::DtlsIdentityStoreInterface override;
+  // Initialize will start to preemptively generating an RSA identity in the
+  // background, if the worker thread is not the same as the signaling thread.
+  void Initialize() override;
+  // webrtc::DtlsIdentityStoreInterface override;
+  void RequestIdentity(rtc::KeyType key_type,
+                       ScopedRefPtrObserver observer) override;
 
   // rtc::MessageHandler override;
   void OnMessage(rtc::Message* msg) override;
 
-  // Returns true if there is a free identity, used for unit tests.
-  bool HasFreeIdentityForTesting() const;
+  // Returns true if there is a free RSA identity, used for unit tests.
+  bool HasFreeIdentityForTesting(rtc::KeyType key_type) const;
 
  private:
-  sigslot::signal0<> SignalDestroyed;
+  void GenerateIdentity(rtc::KeyType key_type,
+                        ScopedRefPtrObserver observer);
+  void OnIdentityGenerated(rtc::KeyType key_type,
+                           rtc::scoped_ptr<rtc::SSLIdentity> identity);
+
   class WorkerTask;
-  typedef rtc::ScopedMessageData<DtlsIdentityStore::WorkerTask>
-      IdentityTaskMessageData;
+  typedef rtc::ScopedMessageData<DtlsIdentityStoreImpl::WorkerTask>
+      WorkerTaskMessageData;
 
-  void GenerateIdentity();
-  void OnIdentityGenerated(rtc::scoped_ptr<rtc::SSLIdentity> identity);
-  void ReturnIdentity(rtc::scoped_ptr<rtc::SSLIdentity> identity);
+  // A key type-identity pair.
+  struct IdentityResult {
+    IdentityResult(rtc::KeyType key_type, rtc::SSLIdentity* identity)

[tommi, 2015/06/18 13:55:35]

pass identity ownership via scoped_ptr

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

Acknowledged.

+        : key_type_(key_type), identity_(identity) {}
 
-  void PostGenerateIdentityResult_w(rtc::scoped_ptr<rtc::SSLIdentity> identity);
+    rtc::KeyType key_type_;
+    rtc::scoped_ptr<rtc::SSLIdentity> identity_;
+  };
+  typedef rtc::ScopedMessageData<IdentityResult> IdentityResultMessageData;

[tommi, 2015/06/18 13:55:35]

empty line above this one

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

Acknowledged.

+
+  sigslot::signal0<> SignalDestroyed;
 
   rtc::Thread* const signaling_thread_;
+  // TODO(hbos): RSA generation is slow and would be VERY slow if we switch over
+  // to 2048, DtlsIdentityStore should use a new thread and not the "general
+  // purpose" worker thread.
   rtc::Thread* const worker_thread_;
 
-  // These members should be accessed on the signaling thread only.
-  int pending_jobs_;
-  rtc::scoped_ptr<rtc::SSLIdentity> free_identity_;
-  typedef std::queue<rtc::scoped_refptr<webrtc::DTLSIdentityRequestObserver>>
-      ObserverList;
-  ObserverList pending_observers_;
+  // Only touch on the |signaling_thred_|.

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

signaling_thread_

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

Acknowledged.

+  std::list<ScopedRefPtrObserver> request_observers_[rtc::KT_LAST];

[tommi, 2015/06/18 13:55:35]

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.

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

Acknowledged.

+  size_t gen_in_progress_counts_[rtc::KT_LAST];
+  rtc::scoped_ptr<rtc::SSLIdentity> free_identities_[rtc::KT_LAST];
 };
 
 }  // namespace webrtc