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

content/browser/media/dtls_identity_store.h

+// 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.
+
+#ifndef CONTENT_BROWSER_MEDIA_DTLS_IDENTITY_STORE_H_
+#define CONTENT_BROWSER_MEDIA_DTLS_IDENTITY_STORE_H_
+
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/singleton.h"
+
+class GURL;
+
+namespace base {
+class TaskRunner;
+}  // namespace base
+
+namespace content {
+
+// This is a singleton class called in the browser UI thread.
+// It dispatches DTLS identity requests to the background worker thread.
+class DTLSIdentityStore {
+ public:
+  static DTLSIdentityStore* GetInstance();

[Ryan Sleevi, 2013/06/04 19:13:52]

DESIGN: No new singletons.
DESIGN: You can't mix the guarantees of a singleton while still having public
constructors. It's a design contract that cannot be met.

[jiayl, 2013/06/05 00:08:09]

What's the alternative to singleton? I tried to find a global-ish object to own
this object and didn't find a good place. ServerBoundCertService lives in the IO
thread globals in src/chrome, but it cannot be used in Content.

[Ryan Sleevi, 2013/06/06 23:57:37]

The alternative to a Singleton is not using a Singleton, and creating an
instance variable.

Minimally, it's a given that this Singleton design will not scale the moment you
move to persisting, because persistence is going to be on a per-Profile basis.

You need to work through your initialization layers to find an appropriate place
to hang this object off of - which is almost certainly going to be on the
Profile or related - I think the "new hotness" in content/ is StoragePartition
(by way of BrowserContext -> SiteInstance).

Either way, this code should not land with a Singleton, and any code that
assumes this is a Singleton is making a bad design assumption.

[jiayl, 2013/06/07 00:31:25]

jam@, could you give some suggestion on who should own this object in
content/browser?

On 2013/06/06 23:57:37, Ryan Sleevi wrote:

[jam, 2013/06/07 00:46:49]

if this code is per profile, then it should either be per BrowserContext (i.e.
content's version of Profile), or StoragePartition. ajwong or creis can tell you
which one is appropriate in this case as I've forgotten the nuances.

if you put this per BrowserContext, then make this class derive from
base::SupportsUserData::Data. you can then attach it to BC since that one
derives from SupportsUserData.

if you put this per StoragePartition, then you can hang it off
StoragePartitionImpl

+  DTLSIdentityStore();
+  DTLSIdentityStore(const scoped_refptr<base::TaskRunner>& task_runner);

[Ryan Sleevi, 2013/06/04 19:13:52]

STYLE: explicit

[jiayl, 2013/06/06 05:10:24]

Done.

+  ~DTLSIdentityStore();
+
+  typedef base::Callback<void(const std::string&, const std::string&)>
+      OnCompleteCallback;
+
+  // Get the DTLS identity for the given domain of |url|, or generate a new one
+  // if not existent. Asynchronous. Called on the browser UI thread.
+  // |url| is the frame URL of the PeerConnection requesting the identity;

[Ryan Sleevi, 2013/06/04 19:13:52]

STYLE: terms like "frame URL" don't really fit here. What if the page isn't
using frames? It's better to express what it is (and perhaps rename the
variable) - it's the origin, is it not?

[jiayl, 2013/06/06 05:10:24]

Renamed to origin.

+  // |identity_name| is the name of the identity;
+  // |common_name| ???
+  // |callback| is the callback to return the result.
+  void GetOrGenerateIdentity(const GURL& url,
+                             const std::string& identity_name,
+                             const std::string& common_name,
+                             const OnCompleteCallback& callback

[Ryan Sleevi, 2013/06/04 19:13:52]

DESIGN: If a requester goes away while an identity request is being made, is
there any way to abort this request?

[jiayl, 2013/06/06 05:10:24]

How can we tell a requester goes away? It seems the only way is to send another
IPC message saying "cancel the previous request". Is it worth the extra
complexity?

[Ryan Sleevi, 2013/06/06 23:57:37]

Yes.

You're going to be performing expensive operations on a fixed worker pool, and
you're going to be enqueuing these in order to ensure they remain servicable.
You're going to have to consider the situation where something is requested of
the browser process, and then goes away.

Using WeakPtr with the callback is not sufficient.

This is again why I strongly encouraged you to follow the OriginBoundCertService
model, which looks at all of the sorts of issues you're going to have to solve
for this design as well.

[jiayl, 2013/06/07 00:24:07]

But ServerBoundCertService does not remove a cancelled job from the queue, but
runs the job without calling the callback.

On 2013/06/06 23:57:37, Ryan Sleevi wrote:

[jiayl, 2013/06/13 21:50:45]

OK, now the request is cancelled when the DTLSIdentityService and
DTLSIdentityServiceHost go away. Note "cancelled" means the callback will not be
called and the backward IPC will not be sent; the cert will still be generated,
just like ServerBoundCertService.

On 2013/06/07 00:24:07, jiayl wrote:

+                             );

[Ryan Sleevi, 2013/06/04 19:13:52]

STYLE: closing parens go on the same line as the last parameter.

[jiayl, 2013/06/06 05:10:24]

Done.

+ private:
+  friend struct DefaultSingletonTraits<DTLSIdentityStore>;
+
+  // The TaskRunner for doing work on a worker thread.
+  scoped_refptr<base::TaskRunner> task_runner_;
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_MEDIA_DTLS_IDENTITY_STORE_H_