Generates the DTLS identity in browser process and returns it to render process.

content/browser/media/webrtc_identity_store.cc

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/browser/media/webrtc_identity_store.h"
+
+#include "base/bind.h"
+#include "base/callback_helpers.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/rand_util.h"
+#include "base/task_runner.h"
+#include "base/threading/worker_pool.h"
+#include "content/public/browser/browser_thread.h"
+#include "crypto/rsa_private_key.h"
+#include "googleurl/src/gurl.h"
+#include "net/base/net_errors.h"
+#include "net/cert/x509_certificate.h"
+
+namespace content {
+
+namespace {

[Ami GONE FROM CHROMIUM, 2013/06/27 20:05:18]

this isn't useful (and makes debug tooling sadder)
(media code generally prefers file-static to anonymous namespace, and the style
guide allows either; the struct is sufficiently specifically named that
collision within the content namespace seems unlikely)

+
+struct WebRTCIdentityRequestResult {
+  int error;
+  std::string certificate;
+  std::string private_key;
+};
+
+static void GenerateIdentityWorker(const std::string& common_name,
+                                   WebRTCIdentityRequestResult* result) {
+  result->error = net::OK;
+  std::string certificate;

[Ami GONE FROM CHROMIUM, 2013/06/27 20:05:18]

unused

+  std::vector<uint8> private_key_info;

[Ami GONE FROM CHROMIUM, 2013/06/27 20:05:18]

can move to just before first use (l.62).

+
+  int serial_number = base::RandInt(0, std::numeric_limits<int>::max());
+
+  scoped_ptr<crypto::RSAPrivateKey> key(crypto::RSAPrivateKey::Create(1024));
+  if (!key.get()) {
+    DLOG(ERROR) << "Unable to create key pair for client";
+    result->error = net::ERR_KEY_GENERATION_FAILED;
+    return;
+  }
+
+  scoped_refptr<net::X509Certificate> cert =
+      net::X509Certificate::CreateSelfSigned(key.get(),
+                                             "CN=" + common_name,
+                                             serial_number,
+                                             base::TimeDelta::FromDays(30));
+  if (!cert) {
+    DLOG(ERROR) << "Unable to create x509 cert for client";
+    result->error = net::ERR_SELF_SIGNED_CERT_GENERATION_FAILED;
+    return;
+  }
+  if (!net::X509Certificate::GetDEREncoded(cert->os_cert_handle(),
+                                           &result->certificate)) {
+    DLOG(ERROR) << "Unable to get the DER decoded data from the cert.";
+    result->error = net::ERR_SELF_SIGNED_CERT_GENERATION_FAILED;
+    return;
+  }
+
+  if (!key->ExportPrivateKey(&private_key_info)) {
+    DLOG(ERROR) << "Unable to export private key";
+    result->error = net::ERR_PRIVATE_KEY_EXPORT_FAILED;
+    return;
+  }
+
+  result->private_key =
+      std::string(private_key_info.begin(), private_key_info.end());
+}
+
+}  // namespace
+
+// The class represents an identity request internal to WebRTCIdentityStore.
+// It has a one-to-one mapping to the external version of the request
+// WebRTCIdentityRequestHandle, which is the target of the
+// WebRTCIdentityRequest's completion callback.
+// It's deleted automatically when the request is completed.
+class WebRTCIdentityRequest {
+ public:
+  WebRTCIdentityRequest(const WebRTCIdentityStore::CompletionCallback& callback)
+      : callback_(callback) {}
+
+ private:
+  friend class WebRTCIdentityStore;

[Ami GONE FROM CHROMIUM, 2013/06/27 20:05:18]

This is a strange combination of lines considering this whole class is only
known to this (impl) file.  

[jiayl, 2013/06/27 21:08:37]

This is to make sure WebRTCIdentityRequestHandle cannot access the methods that
should only be called from WebRTCIdentityStore. If you think it's unnecessary, I
can remove it and make all methods public.

+
+  void Cancel() { callback_.Reset(); }
+
+  void Post(WebRTCIdentityRequestResult* result) {
+    if (callback_.is_null())
+      return;
+    callback_.Run(result->error, result->certificate, result->private_key);
+    // "this" will be deleted after this point.
+  }
+
+  WebRTCIdentityStore::CompletionCallback callback_;
+};
+
+// The class represents an identity request which calls back to the external
+// client when the request completes.
+// Its lifetime is tied with the Callback held by the corresponding
+// WebRTCIdentityRequest.

[Ami GONE FROM CHROMIUM, 2013/06/27 20:05:18]

I'm confused as to why this class exists.  Why not simply bind Request's Cancel
for the cancel_callback in RequestIdentity?

[jiayl, 2013/06/27 21:08:37]

When the certificate caching is in place, we'll want combine multiple external
requests with the same origin/name into one internal request. It gives use more
flexibility to separate the external and internal representation of a request.

[Ami GONE FROM CHROMIUM, 2013/06/27 21:30:32]

I don't quite get this, and in general I prefer to not to land code because
it'll be useful in the future.  But you're much more involved with this code and
where it's going than I am so I leave it up to you to decide if you want to keep
it.

+class WebRTCIdentityRequestHandle {
+ public:
+  WebRTCIdentityRequestHandle(
+      WebRTCIdentityStore* store,
+      const WebRTCIdentityStore::CompletionCallback& callback)
+      : store_(store), request_(NULL), callback_(callback) {}
+
+ private:
+  friend class WebRTCIdentityStore;
+
+  // Cancel the request.  Does nothing if the request finished or was already
+  // cancelled.
+  void Cancel() {
+    if (!request_)
+      return;
+
+    callback_.Reset();
+    WebRTCIdentityRequest* request = request_;
+    request_ = NULL;
+    // "this" will be deleted after the following call, becuase "this" is

[Ami GONE FROM CHROMIUM, 2013/06/27 20:05:18]

typo: becuase

[jiayl, 2013/06/27 21:08:37]

Done.

+    // owned by the Callback held by |request|.
+    store_->CancelRequestInternal(request);

[Ami GONE FROM CHROMIUM, 2013/06/27 20:05:18]

Why go through store_ for this instead of simply calling
request->Cancel();
?

(you'd have to make WRIRequest::Cancel public in that class, of course)

+  }
+
+  void OnRequestStarted(WebRTCIdentityRequest* request) {
+    DCHECK(request);
+    request_ = request;
+  }
+
+  void OnRequestComplete(int error,
+                         const std::string& certificate,
+                         const std::string& private_key) {
+    DCHECK(request_);
+    request_ = NULL;
+    base::ResetAndReturn(&callback_).Run(error, certificate, private_key);
+  }
+
+  WebRTCIdentityStore* store_;
+  WebRTCIdentityRequest* request_;
+  WebRTCIdentityStore::CompletionCallback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(WebRTCIdentityRequestHandle);
+};
+
+WebRTCIdentityStore::WebRTCIdentityStore()
+    : task_runner_(base::WorkerPool::GetTaskRunner(true)) {}
+
+WebRTCIdentityStore::~WebRTCIdentityStore() {}
+
+bool WebRTCIdentityStore::RequestIdentity(const GURL& origin,
+                                          const std::string& identity_name,
+                                          const std::string& common_name,
+                                          const CompletionCallback& callback,
+                                          base::Closure* cancel_callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

[Ami GONE FROM CHROMIUM, 2013/06/27 20:05:18]

I think it is true that all the methods in this class other than
GenerateIdentityWorker want to have this DCHECK, but most don't.

[jiayl, 2013/06/27 21:08:37]

Done.

+
+  WebRTCIdentityRequestHandle* handle =
+      new WebRTCIdentityRequestHandle(this, callback);
+
+  WebRTCIdentityRequest* request = new WebRTCIdentityRequest(base::Bind(
+      &WebRTCIdentityRequestHandle::OnRequestComplete, base::Owned(handle)));
+  handle->OnRequestStarted(request);
+
+  WebRTCIdentityRequestResult* result = new WebRTCIdentityRequestResult;

[Ami GONE FROM CHROMIUM, 2013/06/27 20:05:18]

nit: always include the ()'s on a new expression.  There are semantic
differences (initialization style) between including them and excluding them for
some new expressions, though not here, but for consistency it's best to always
include them

[jiayl, 2013/06/27 21:08:37]

Done.

+  if (!task_runner_->PostTaskAndReply(
+          FROM_HERE,
+          base::Bind(&GenerateIdentityWorker, common_name, result),
+          base::Bind(&WebRTCIdentityRequest::Post,
+                     base::Owned(request),
+                     base::Owned(result))))
+    return false;
+
+  *cancel_callback = base::Bind(&WebRTCIdentityRequestHandle::Cancel,
+                                base::Unretained(handle));
+  return true;

[Ami GONE FROM CHROMIUM, 2013/06/27 20:05:18]

fwiw: since this method returns true iff *cancel_callback is set, could as well
return the Cancel closure (or Closure()) and have the caller test the return
value for is_null() to decide whether the request has been kicked off.  The
(tiny) advantage of doing this is that it makes the caller more likely to
examine the return value (as opposed to caller incorrectly forgetting to examine
the return value because it is focused on the output parameter).
Some examples of such forgetfulness are in the unittest for this class ;)

[jiayl, 2013/06/27 21:08:37]

Done.

+}
+
+WebRTCIdentityStore::WebRTCIdentityStore(
+    const scoped_refptr<base::TaskRunner>& task_runner)
+    : task_runner_(task_runner) {}
+
+void WebRTCIdentityStore::CancelRequestInternal(
+    WebRTCIdentityRequest* request) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  request->Cancel();
+}
+
+}  // namespace content