RTCCertificate, RTCPeerConnection.generateCertificate (WebRTC JavaScript) added.

RTCCertificate, RTCPeerConnection.generateCertificate (WebRTC JavaScript) added.
(This is a re-upload of https://codereview.chromium.org/1311853005/ after blink merged with chrome.)

Primary changes:

New Blink JavaScript (idl)
- blink::RTCCertificate added.
- RTCPeerConnection.generateCertificate added.
- RTCPeerConnection::parseConfiguration, used to parse RTCConfiguration dictionaries, updated to support optional sequence<RTCCertificate> certificates member.

Blink layer interfaces added (abstraction level between blink and chromium-using-webrtc)
- blink::WebRTCCertificate, the heart of a javascript blink::RTCCertificate.
- blink::WebRTCCertificateGenerator, something that generates a blink::WebRTCCertificate.

Chromium implementations
- content::RTCCertificate (impl blink::WebRTCCertificate), basically a wrapper/container for the WebRTC layer certificate rtc::RTCCertificate.
- content::RTCCertificateGenerator (impl blink::WebRTCCertificateGenerator), creates content::RTCCertificate. It generates the rtc::RTCCertificate using the already existing PeerConnectionIdentityStore.

Other changes:

- Platform gets createRTCCertificateGenerator(), RendererBlinkPlatformImpl implements it to create a new content::RTCCertificateGenerator. Briges blink with chromium implementation.
- blink::WebRTCCertificate added to blink::RTCConfiguration. When converted to WebRTC layer configuration the certificates are retreived, meaning the peerconnection will end up with the certificates as input.
- PeerConnectionIdentityStore calling WebRTC code (SSLIdentity::Generate) when request is made to generate non-RSA certificates. Previously only RSA was supported and non-RSA would DCHECK crash.

blink-dev@ intent to implement: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3Pps95Q6zRQ

BUG=528250, webrtc:4927
Committed: https://crrev.com/ef4ab6012fddc80992b8848cfc6700e731dc39b1
Cr-Commit-Position: refs/heads/master@{#355520}

[hbos_chromium, 2015-09-28 15:43:15]

hbos@chromium.org changed reviewers:
+ guidou@chromium.org, jochen@chromium.org, mkwst@chromium.org

[hbos_chromium, 2015-09-28 15:46:02]

PTAL jochen, guidou.
This is a re-upload of https://codereview.chromium.org/1311853005/ due to blink
+ chromium repository merge.

[jochen (gone - plz use gerrit), 2015-09-29 07:47:08]

jochen@chromium.org changed reviewers:
+ rsleevi@chromium.org

[jochen (gone - plz use gerrit), 2015-09-29 07:47:10]

some comments.

can you add tests please?

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/peer...
File content/renderer/media/peer_connection_identity_store.cc (right):

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/peer...
content/renderer/media/peer_connection_identity_store.cc:78: void
CallObserverOnSuccess(
please add a ctor

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/peer...
content/renderer/media/peer_connection_identity_store.cc:86: };
disallow copy/assign

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_certificate.h (right):

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate.h:20: explicit RTCCertificate(
no explicit

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate.h:35: };
disallow copy/assign

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_certificate_generator.cc (right):

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate_generator.cc:69: };
disallow copy/assign

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate_generator.cc:71: }  // anonymous
namespace
just // namespace

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate_generator.cc:82:
content::PeerConnectionIdentityStore* store =
inside content, you shouldn't need content::

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_certificate_generator.h (right):

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate_generator.h:12: #include
"third_party/webrtc/base/scoped_ref_ptr.h"
most of the includes appear to be unused in this header?

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate_generator.h:28: };
please add private: DISALLOW_COPY_AND_ASSIGN(RTCCertificateGenerator)

[hbos_chromium, 2015-10-02 15:43:18]

Sorry for not rebasing before leaving office (patch failure). Time zone diffs
etc.

Addressed comments and added two types of javascript tests:

RTCPeerConnection-generateCertificate.html:
  Makes sure RSA and ECDSA certificates can be generated.
RTCPeerConnection.html:
  Makes sure the RTCPeerConnection constructor does not throw an exception if
certificates are supplied.

These are pretty high level tests looking at certificates as an opaque class
(only 'expires' is visible so the tests make sure we get a certificate but can't
tell what type of certificate it is). They don't cover what the peerconnection
will end up doing with the certificates or if a call can still be made if
supplied, but this should already be covered by lower level webrtc tests. Or do
you desire more tests?

Note that there are two minor things I want to do before this lands.
1) Parse keygenAlgorithm with WebCryptoAPI. This is not possible with
publicExponent as an integer but this was a bug in the webrtc spec and it should
take a byte array [1,0,1] which is parsable by webcrypto.
2) Parameterized key type is about to land in webrtc so I might have to tweak my
key type class to match it.

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/peer...
File content/renderer/media/peer_connection_identity_store.cc (right):

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/peer...
content/renderer/media/peer_connection_identity_store.cc:78: void
CallObserverOnSuccess(
On 2015/09/29 07:47:09, jochen wrote:
> please add a ctor

Done.

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/peer...
content/renderer/media/peer_connection_identity_store.cc:86: };
On 2015/09/29 07:47:09, jochen wrote:
> disallow copy/assign

Done.

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_certificate.h (right):

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate.h:20: explicit RTCCertificate(
On 2015/09/29 07:47:09, jochen wrote:
> no explicit

Done.

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate.h:35: };
On 2015/09/29 07:47:09, jochen wrote:
> disallow copy/assign

Done.

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_certificate_generator.cc (right):

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate_generator.cc:69: };
On 2015/09/29 07:47:09, jochen wrote:
> disallow copy/assign

Done.

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate_generator.cc:71: }  // anonymous
namespace
On 2015/09/29 07:47:09, jochen wrote:
> just // namespace

Done.

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate_generator.cc:82:
content::PeerConnectionIdentityStore* store =
On 2015/09/29 07:47:09, jochen wrote:
> inside content, you shouldn't need content::

Done.

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_certificate_generator.h (right):

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate_generator.h:12: #include
"third_party/webrtc/base/scoped_ref_ptr.h"
On 2015/09/29 07:47:09, jochen wrote:
> most of the includes appear to be unused in this header?

Done.

https://codereview.chromium.org/1373023002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_certificate_generator.h:28: };
On 2015/09/29 07:47:09, jochen wrote:
> please add private: DISALLOW_COPY_AND_ASSIGN(RTCCertificateGenerator)

Done.

[jochen (gone - plz use gerrit), 2015-10-05 15:34:53]

lgtm

[jochen (gone - plz use gerrit), 2015-10-05 15:39:00]

On 2015/10/05 at 15:34:53, jochen wrote:
> lgtm

oops, ignore that, sorry

[jochen (gone - plz use gerrit), 2015-10-05 15:39:55]

jochen@chromium.org changed reviewers:
+ philipj@opera.com

[jochen (gone - plz use gerrit), 2015-10-05 15:39:55]

+philipj who knows more about media than me

sorry for being so slow with comments :-/

[philipj_slow, 2015-10-05 16:30:48]

hbos@, which bits would you like me to review? For modules/mediastream you have
tommi@ right in your office, but I'd be happy to look at any specifics that need
a second (or third) opinion.

[hbos_chromium, 2015-10-06 13:52:57]

Patchset #3 (id:40001) has been deleted

[hbos_chromium, 2015-10-06 15:56:35]

PTAL guidou, jochen.

jochen/philipj: With guidou giving a second opinion, do you want philipj to
review something specific or are we good? (I think the heart of this CL is the
RTCPeerConnection.cpp changes which use public/platform/ interfaces added in
this CL).

I now want to land this as-is if reviewers and trybots are happy.

I decided not to use AlgorithmIdentifier/WebCryptoAPI in this CL and left a
TODO. This is because 1) I found another WebRTC/WebCryptoAPI spec discrepancy
(github.com/w3c/webrtc-pc/issues/322), 2) I'm not allowed to use the WebCrypto
normalization code with current module DEPS, and 3) this CL is already big
enough (sorry, reviewers) and I want it to land soon.
I will address this in follow-up CLs.

I replaced WebRTCKeyType with a WebRTCKeyParams that resembles the parameterized
key type in webrtc that is about to land, but there is no reason to block this
CL until that webrtc CL has landed. There are a couple of added TODOs related to
this that can easily be addressed in follow up-CLs.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCCertificate.h (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificate.h:72: WebRTCCertificate&
operator=(const WebRTCCertificate&) = delete;
The include of wtf/Noncopyable caused problems when compiling with GN (file not
found), but not when compiling with gyp. I don't know if public/platform should
or should not have a dependency on wtf or, if it should, how I need to updated
the GN files. I spent some time on this already and then finally removed the
include, but if this file should use WTF_MAKE_NONCOPYABLE I need to look at this
even more. Reviewers?

[philipj_slow, 2015-10-08 08:32:31]

I've taken a closer look at the IDL files, as that's kind of my thing.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl:31: [
Can you link to http://w3c.github.io/webrtc-pc/#rtccertificate-interface above
the interface?

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl:33:
NoInterfaceObject,
The spec doesn't have [NoInterfaceObject].

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl:38: readonly
attribute Date expires;
I've filed a spec bug here: https://github.com/w3c/webrtc-pc/issues/324

The way it's implemented, I think this will return a new Date object every time.
Can you verify if this is the case?

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl
(right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl:54: //
TODO(hbos): Use AlgorithmIdentifier and the WebCryptoAPI normalization process.
Are there any difficulties in just using the existing AlgorithmIdentifier type
right now? That should remove a bunch of custom code, and actually handle the
"or DOMString" part of the type, which looks like it currently doesn't work.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl:55:
[RuntimeEnabled=RTCCertificate, RaisesException, CallWith=ScriptState] static
Promise<RTCCertificate> generateCertificate(Dictionary keygenAlgorithm);
Since this is in a partial interface in the spec, could you either implement it
as a partial interface or put it at the end of this interface with a link to
http://w3c.github.io/webrtc-pc/#sec.cert-mgmt ? (This is how the many
partial-in-spec-but-not-implementation bits of Document and other interfaces are
done.)

[Guido Urdaneta, 2015-10-08 11:13:03]

https://codereview.chromium.org/1373023002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1373023002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:96: // which
is ensured to invoke one of these. Takes ownership of |resolver|.
Would it be much more complex to handle ownership of this object without 
"delete this"? (e.g., by declaring that generateCertificate() takes ownership of
the object, and using a smartpointer in its implementation)

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
File content/renderer/media/peer_connection_identity_store.cc (right):

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:75: // Used to invokes
|observer|->OnSuccess in a PostTask.
s/invokes/invoke

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:77: : public
base::RefCountedThreadSafe<ObserverOnSuccessCaller> {
Does it have to be refcounted?

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:135: // Helper class
necessary because OnSuccess takes a scoped_ptr argument
You can probably use base::Passed() instead of the helper class.
https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers....

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
File content/renderer/media/rtc_certificate_generator.cc (right):

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.cc:30: this_ = self;
Would it be much more complex to handle ownership of this object without the
"delete this" approach?

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.cc:82: // in RequestIdentity
when rtc::KeyParams CL has landed.
Reference a crbug

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.cc:99: // of having parameter
validation code in multiple places.
Reference a crbug

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCCertificate.h (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCCertificate.h:54:
RTCCertificate(WebRTCCertificate* /*certificate*/);
Don't put |certificate| inside /**/ comments.
Leave it empty and rewrite the comment above to not use the variable name.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h:39: #include
"modules/mediastream/RTCCertificate.h"
It looks like you don't need this include here.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCCertificate.h (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificate.h:65: // Gets a raw pointer
to a member value referencing the WebRTC layer version
s/Gets/Returns

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificate.h:68: virtual const
rtc::scoped_refptr<rtc::RTCCertificate>* rtcCertificate() const = 0;
What if you return constant ref instead of constant pointer?

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCCertificateGenerator.h (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificateGenerator.h:50: const
WebURL& /*url*/,
don't put the parameter names inside /**/ comments.

[Ryan Sleevi, 2015-10-10 03:36:59]

rsleevi@chromium.org changed reviewers:
+ eroman@chromium.org
- rsleevi@chromium.org

[Ryan Sleevi, 2015-10-10 03:37:00]

Eric's a far better reviewer for this.

[Ryan Sleevi, 2015-10-10 04:04:48]

I'd say my biggest remark is to take a second pass through the
comments/documentation. There's a lot of informal colloquialisms and grammar
issues.

I'd suggest reviewing
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Comments and
looking through some samples of the Chrome code if you're not entirely sure; i
tried to avoid highlighting a bunch of individual things, because as written,
I'm not entirely sure I understand what you were trying to say or the code was
doing, but also because I didn't want you to feel like I was piling on.

I'm still a little confused as to some of the implementation bits, especially on
the Blink side. It feels... complicated and perhaps dangerous. Eric may have a
different perspective to offer..

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
File content/renderer/media/peer_connection_identity_store.cc (right):

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:15: const char
kIdentityName[] = "WebRTC";
style nit: newline before this.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:82:
rtc::scoped_refptr<webrtc::DtlsIdentityRequestObserver> observer,
Why are you passing a ref-counted object by-value? This should be const-ref to
avoid churn.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:83: rtc::SSLIdentity*
identity) {
if rtc::scoped_ptr<> supports .Pass() (as shown by line 84), why isn't this also
provided as a scoped_ptr<> to make the ownership explicit?

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:117: // Use Chromium
identity generation code for RSA.
Why split between these two?

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
File content/renderer/media/rtc_certificate.h (right):

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate.h:18: // not have direct access to the
WebRTC stuff.
comment nit: "stuff" seems a bit... colloquial

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate.h:29: const
rtc::scoped_refptr<rtc::RTCCertificate>*
Why are you returning a pointer to a scoped pointer?

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
File content/renderer/media/rtc_certificate_generator.cc (right):

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.cc:51:
der_private_key.length());
This seems quite inefficient to be converting; why is PEM involved at all?

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.cc:87: new
PeerConnectionIdentityStore(url, first_party_for_cookies);
BUG? How does this not leak store?

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
File content/renderer/media/rtc_certificate_generator.h (right):

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.h:21:
~RTCCertificateGenerator() override { }
inconsistent spacing

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:921: return NULL;
https://chromium-cpp.appspot.com/ says nullptr?

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCCertificate.h (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCCertificate.h:44: // Visible to
the JavaScript world in accordance with the .idl file:
Is this common in Blink? I'm not accustomed to seeing comments like this.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:501: }
Eric can comment, but I'm fairly certain that the implementation described here
is not aligned with the spec, because it's not aligned with WebCrypto, which was
invoked.

For example, name is not case-sensitive.  And there were also the IDL conversion
rules at play here that I don't believe are being followed.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCCertificate.h (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificate.h:44: // Interface defining
what blink needs to know about certificates (abstracting away
comment nit: Shouldn't this be Blink?

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificate.h:45: // Chromium and
WebRTC layer implementation details). You can create shallow copies of the
This doesn't seem like proper grammar, nor in line with our comment styles. It
seems colloquial, but also "abstracting way" should be "abstracting away"

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCCertificateGenerator.h (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificateGenerator.h:47: //
|observer| when the operation completes is implementation-specific.
That seems like an _extremely_ dangerous API contract.

Making code multi-threaded like this is a huge cause of crashes and abuse. You
should make the contract very clear (that it's the same thread)

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCKeyParams.h (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCKeyParams.h:44: unsigned pubExp;
See https://www.chromium.org/developers/coding-style#TOC-Types

[hbos_chromium, 2015-10-14 12:54:29]

Patchset #8 (id:160001) has been deleted

[hbos_chromium, 2015-10-14 13:00:50]

PTAL everyone.

I want this and follow-up CLs if possible to land before 48 feature freeze.

If you want me to update any more comments/documentation or think something
needs to be better explained please be more specific as to what to update.

https://codereview.chromium.org/1373023002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1373023002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:96: // which
is ensured to invoke one of these. Takes ownership of |resolver|.
On 2015/10/08 11:13:03, Guido Urdaneta wrote:
> Would it be much more complex to handle ownership of this object without 
> "delete this"? (e.g., by declaring that generateCertificate() takes ownership
of
> the object, and using a smartpointer in its implementation)

Due the different layers (Blink, Chromium) I'd probably have to pass a raw
pointer even if I wanted to give ownership of the observer to
generateCertificate implementations which could use something like scoped_ptr
internally and do the deleting for us. Would that be better than "delete this"?

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
File content/renderer/media/peer_connection_identity_store.cc (right):

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:15: const char
kIdentityName[] = "WebRTC";
On 2015/10/10 04:04:48, Ryan Sleevi (busy till 10-12) wrote:
> style nit: newline before this.

Done.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:75: // Used to invokes
|observer|->OnSuccess in a PostTask.
On 2015/10/08 11:13:03, Guido Urdaneta wrote:
> s/invokes/invoke

Done.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:77: : public
base::RefCountedThreadSafe<ObserverOnSuccessCaller> {
On 2015/10/08 11:13:03, Guido Urdaneta wrote:
> Does it have to be refcounted?

Yes, TaskRunner code uses ref counting.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:82:
rtc::scoped_refptr<webrtc::DtlsIdentityRequestObserver> observer,
On 2015/10/10 04:04:48, Ryan Sleevi (busy till 10-12) wrote:
> Why are you passing a ref-counted object by-value? This should be const-ref to
> avoid churn.

Done.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:83: rtc::SSLIdentity*
identity) {
On 2015/10/10 04:04:48, Ryan Sleevi (busy till 10-12) wrote:
> if rtc::scoped_ptr<> supports .Pass() (as shown by line 84), why isn't this
also
> provided as a scoped_ptr<> to make the ownership explicit?

This is problematic due to PostTask/base::Bind (see line 141) with template code
that that invokes by passing arguments the standard way, not using Pass().
Using scoped_ptr (without rtc namespace) and base::Passed I got around this, but
still had to use a helper function to convert from scoped_ptr to rtc::scoped_ptr
before OnSuccess call.

Class replaced by smaller helper function.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:117: // Use Chromium
identity generation code for RSA.
On 2015/10/10 04:04:48, Ryan Sleevi (busy till 10-12) wrote:
> Why split between these two?

Ideally we would only use WebRTC code for certificate generation for use in
WebRTC. For performance reasons, Chromium implemented its own generation for RSA
(I believe to allow caching, perhaps even generation, in the browser outside of
the WebRTC world).

WebRTC has now been updated to support other key types and parameters. These
other options are only supported by WebRTC code. Luckily, generating ECDSA is so
fast we can simply do it in the else-clause on the fly. But for performance
reasons we still use the Chromium code for of generating RSA.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:135: // Helper class
necessary because OnSuccess takes a scoped_ptr argument
On 2015/10/08 11:13:03, Guido Urdaneta wrote:
> You can probably use base::Passed() instead of the helper class.
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers....

Done.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
File content/renderer/media/rtc_certificate.h (right):

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate.h:18: // not have direct access to the
WebRTC stuff.
On 2015/10/10 04:04:48, Ryan Sleevi (busy till 10-12) wrote:
> comment nit: "stuff" seems a bit... colloquial

Done.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate.h:29: const
rtc::scoped_refptr<rtc::RTCCertificate>*
On 2015/10/10 04:04:48, Ryan Sleevi (busy till 10-12) wrote:
> Why are you returning a pointer to a scoped pointer?

The interface being implemented, blink::WebRTCCertificate, exists in blink which
does not "know" about webrtc code. I cannot return by value without introducing
a real dependency on webrtc from blink which is not allowed.

I resorted to returning a const ptr to the raw member data. This would otherwise
be bad practice, but I think it is an acceptable solution considering
alternatives (such as static_casting from blink interface to chromium
implementation with hidden assumptions about which implementation is used).

On second thought I'll return const reference instead of const pointer. Similar
but better style.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
File content/renderer/media/rtc_certificate_generator.cc (right):

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.cc:30: this_ = self;
On 2015/10/08 11:13:03, Guido Urdaneta wrote:
> Would it be much more complex to handle ownership of this object without the
> "delete this" approach?

I have to protect it with reference counting because it's a ref counted object
(due to webrtc interface).

Added comments and cleaned up code though. Like doing self_ref_ = this (which
you could not due in the constructor as discussed offline) and putting the
RequestIdentity logic here in one place.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.cc:82: // in RequestIdentity
when rtc::KeyParams CL has landed.
On 2015/10/08 11:13:03, Guido Urdaneta wrote:
> Reference a crbug

Bug resolved.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.cc:87: new
PeerConnectionIdentityStore(url, first_party_for_cookies);
On 2015/10/10 04:04:48, Ryan Sleevi (OOO) wrote:
> BUG? How does this not leak store?

The ownership of |store| was passed to |identity_observer|, didn't use a
scoped_ptr due to invoking store->RequestIdentity afterwards.

But nvm that, there's was no need to pass the ownership. Using scoped_ptr now
and cleaned some stuff up.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.cc:99: // of having parameter
validation code in multiple places.
On 2015/10/08 11:13:03, Guido Urdaneta wrote:
> Reference a crbug

Bug resolved.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
File content/renderer/media/rtc_certificate_generator.h (right):

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.h:21:
~RTCCertificateGenerator() override { }
On 2015/10/10 04:04:48, Ryan Sleevi (busy till 10-12) wrote:
> inconsistent spacing

Done.

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:921: return NULL;
On 2015/10/10 04:04:48, Ryan Sleevi (busy till 10-12) wrote:
> https://chromium-cpp.appspot.com/ says nullptr?

Updated all places in file as to use nullptr consistently.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCCertificate.h (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCCertificate.h:44: // Visible to
the JavaScript world in accordance with the .idl file:
On 2015/10/10 04:04:48, Ryan Sleevi (busy till 10-12) wrote:
> Is this common in Blink? I'm not accustomed to seeing comments like this.

Not sure, but the JavaScript object as defined in the specs is opaque. What
defines a certificate is obviously more than an expires field though.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCCertificate.h:54:
RTCCertificate(WebRTCCertificate* /*certificate*/);
On 2015/10/08 11:13:03, Guido Urdaneta wrote:
> Don't put |certificate| inside /**/ comments.
> Leave it empty and rewrite the comment above to not use the variable name.

Done.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl:31: [
On 2015/10/08 08:32:31, philipj wrote:
> Can you link to http://w3c.github.io/webrtc-pc/#rtccertificate-interface above
> the interface?

Done.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl:33:
NoInterfaceObject,
On 2015/10/08 08:32:31, philipj wrote:
> The spec doesn't have [NoInterfaceObject].

Done. Nice catch.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl:38: readonly
attribute Date expires;
On 2015/10/08 08:32:31, philipj wrote:
> I've filed a spec bug here: https://github.com/w3c/webrtc-pc/issues/324
> 
> The way it's implemented, I think this will return a new Date object every
time.
> Can you verify if this is the case?

You're right, that's kind of blargh. cert.expires == cert.expires is false.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:501: }
On 2015/10/10 04:04:48, Ryan Sleevi (busy till 10-12) wrote:
> Eric can comment, but I'm fairly certain that the implementation described
here
> is not aligned with the spec, because it's not aligned with WebCrypto, which
was
> invoked.
> 
> For example, name is not case-sensitive.  And there were also the IDL
conversion
> rules at play here that I don't believe are being followed.

You're right. The proper way to address this is to use WebCrypto, but the
modules are not allowed to depend on each other. I've already written code for
this that uses the WebCrypto normalization process but I found myself unable to
upload it due to the dependency it introduced. There is already a TODO(hbos) for
this in the .idl file, but I'll add another one here.

This will be addressed in a follow-up CL. In the meantime I think this is fine
considering this is a test feature and I don't think it is worth writing more
advanced argument parsing code that will be replaced by WebCrypto normalization
code soon anyway.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h:39: #include
"modules/mediastream/RTCCertificate.h"
On 2015/10/08 11:13:03, Guido Urdaneta wrote:
> It looks like you don't need this include here.

Done.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl
(right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl:54: //
TODO(hbos): Use AlgorithmIdentifier and the WebCryptoAPI normalization process.
On 2015/10/08 08:32:31, philipj wrote:
> Are there any difficulties in just using the existing AlgorithmIdentifier type
> right now? That should remove a bunch of custom code, and actually handle the
> "or DOMString" part of the type, which looks like it currently doesn't work.

Yes, I can't readily depend on the WebCrypto code that handles all cases
correctly (dependency between two modules), and until I do in a follow-up CL I
don't think it's worth the time updating the argument parser code that is only
temporary considering this is a "test" feature.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl:55:
[RuntimeEnabled=RTCCertificate, RaisesException, CallWith=ScriptState] static
Promise<RTCCertificate> generateCertificate(Dictionary keygenAlgorithm);
On 2015/10/08 08:32:31, philipj wrote:
> Since this is in a partial interface in the spec, could you either implement
it
> as a partial interface or put it at the end of this interface with a link to
> http://w3c.github.io/webrtc-pc/#sec.cert-mgmt ? (This is how the many
> partial-in-spec-but-not-implementation bits of Document and other interfaces
are
> done.)

Done.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCCertificate.h (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificate.h:44: // Interface defining
what blink needs to know about certificates (abstracting away
On 2015/10/10 04:04:48, Ryan Sleevi (busy till 10-12) wrote:
> comment nit: Shouldn't this be Blink?

Done.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificate.h:45: // Chromium and
WebRTC layer implementation details). You can create shallow copies of the
On 2015/10/10 04:04:48, Ryan Sleevi (busy till 10-12) wrote:
> This doesn't seem like proper grammar, nor in line with our comment styles. It
> seems colloquial, but also "abstracting way" should be "abstracting away"

Acknowledged.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificate.h:65: // Gets a raw pointer
to a member value referencing the WebRTC layer version
On 2015/10/08 11:13:03, Guido Urdaneta wrote:
> s/Gets/Returns

Done.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificate.h:68: virtual const
rtc::scoped_refptr<rtc::RTCCertificate>* rtcCertificate() const = 0;
On 2015/10/08 11:13:03, Guido Urdaneta wrote:
> What if you return constant ref instead of constant pointer?

Done. Yeah that feels better!

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCCertificateGenerator.h (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificateGenerator.h:47: //
|observer| when the operation completes is implementation-specific.
On 2015/10/10 04:04:48, Ryan Sleevi (OOO) wrote:
> That seems like an _extremely_ dangerous API contract.
> 
> Making code multi-threaded like this is a huge cause of crashes and abuse. You
> should make the contract very clear (that it's the same thread)

Good point, I'll say it must be the same thread.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificateGenerator.h:50: const
WebURL& /*url*/,
On 2015/10/08 11:13:03, Guido Urdaneta wrote:
> don't put the parameter names inside /**/ comments. 

Done.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCKeyParams.h (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCKeyParams.h:44: unsigned pubExp;
On 2015/10/10 04:04:48, Ryan Sleevi (busy till 10-12) wrote:
> See https://www.chromium.org/developers/coding-style#TOC-Types

Making this mirror the types of rtc::RSAParams,
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

[torbjorng, 2015-10-14 13:02:47]

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

[torbjorng, 2015-10-14 13:02:48]

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
File content/renderer/media/rtc_certificate_generator.cc (right):

https://codereview.chromium.org/1373023002/diff/100001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.cc:51:
der_private_key.length());
On 2015/10/10 04:04:48, Ryan Sleevi (busy till 10-12) wrote:
> This seems quite inefficient to be converting; why is PEM involved at all?

We don't currently provide any DER format conversions in the underlying OpenSSL
class.
However, this conversion is not slow, although DER might be a tad bit faster.

[philipj_slow, 2015-10-16 13:32:32]

The IDL files now LGTM.

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl:38: readonly
attribute Date expires;
On 2015/10/14 13:00:50, hbos_chromium wrote:
> On 2015/10/08 08:32:31, philipj wrote:
> > I've filed a spec bug here: https://github.com/w3c/webrtc-pc/issues/324
> > 
> > The way it's implemented, I think this will return a new Date object every
> time.
> > Can you verify if this is the case?
> 
> You're right, that's kind of blargh. cert.expires == cert.expires is false.

Given that this is behind a flag I think this does not need to be blocked, but
if you just want to go ahead and rename it getExpirationDate() getExpiryDate() I
will cheer you on to get the same change in the spec :)

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl
(right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl:54: //
TODO(hbos): Use AlgorithmIdentifier and the WebCryptoAPI normalization process.
On 2015/10/14 13:00:50, hbos_chromium wrote:
> On 2015/10/08 08:32:31, philipj wrote:
> > Are there any difficulties in just using the existing AlgorithmIdentifier
type
> > right now? That should remove a bunch of custom code, and actually handle
the
> > "or DOMString" part of the type, which looks like it currently doesn't work.
> 
> Yes, I can't readily depend on the WebCrypto code that handles all cases
> correctly (dependency between two modules), and until I do in a follow-up CL I
> don't think it's worth the time updating the argument parser code that is only
> temporary considering this is a "test" feature.

Acknowledged.

[hbos_chromium, 2015-10-19 09:38:35]

Bump jochen, guidou

[Guido Urdaneta, 2015-10-19 10:41:17]

lgtm % nits

https://codereview.chromium.org/1373023002/diff/180001/content/renderer/media...
File content/renderer/media/peer_connection_identity_store.cc (right):

https://codereview.chromium.org/1373023002/diff/180001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:83: }
micro-nit: put blank line before namespace closing brace

https://codereview.chromium.org/1373023002/diff/180001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:112: //
TODO(torbjorng): Update store to use rtc::KeyParams.
put crbugs  in these TODOs

https://codereview.chromium.org/1373023002/diff/180001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:130: //
DtlsIdentityStoreInterface implementations have to be async.
if you can find a reference for why this is the case, it would be good to
mention it in the comment.

https://codereview.chromium.org/1373023002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl (right):

https://codereview.chromium.org/1373023002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl:31: // The
RTCCertificate based on the WebRTC specs, see
nit: The more common style for this in blink is to just use the next line, and
use https://.

https://codereview.chromium.org/1373023002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1373023002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:481: // This
creates a dependency between the Blink and WebCrypto modules (currently not
allowed).
reference a crbug

https://codereview.chromium.org/1373023002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl
(right):

https://codereview.chromium.org/1373023002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl:136: //
TODO(hbos): Use AlgorithmIdentifier and the WebCryptoAPI normalization process.
reference crbug

[hbos_chromium, 2015-10-19 15:21:35]

Addressed nits. Bump jochen ;)

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl (right):

https://codereview.chromium.org/1373023002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl:38: readonly
attribute Date expires;
On 2015/10/16 13:32:31, philipj wrote:
> On 2015/10/14 13:00:50, hbos_chromium wrote:
> > On 2015/10/08 08:32:31, philipj wrote:
> > > I've filed a spec bug here: https://github.com/w3c/webrtc-pc/issues/324
> > > 
> > > The way it's implemented, I think this will return a new Date object every
> > time.
> > > Can you verify if this is the case?
> > 
> > You're right, that's kind of blargh. cert.expires == cert.expires is false.
> 
> Given that this is behind a flag I think this does not need to be blocked, but
> if you just want to go ahead and rename it getExpirationDate() getExpiryDate()
I
> will cheer you on to get the same change in the spec :)

The spec bug discussion has changed, right now it looks like it will become
"DOMTimeStamp expires". I added a TODO and created a bug that I'm referencing.
When that discussion has settled I'll update expires accordingly.

https://codereview.chromium.org/1373023002/diff/180001/content/renderer/media...
File content/renderer/media/peer_connection_identity_store.cc (right):

https://codereview.chromium.org/1373023002/diff/180001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:83: }
On 2015/10/19 10:41:17, Guido Urdaneta wrote:
> micro-nit: put blank line before namespace closing brace

Done.

https://codereview.chromium.org/1373023002/diff/180001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:112: //
TODO(torbjorng): Update store to use rtc::KeyParams.
On 2015/10/19 10:41:17, Guido Urdaneta wrote:
> put crbugs  in these TODOs

Done.

https://codereview.chromium.org/1373023002/diff/180001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:130: //
DtlsIdentityStoreInterface implementations have to be async.
On 2015/10/19 10:41:17, Guido Urdaneta wrote:
> if you can find a reference for why this is the case, it would be good to
> mention it in the comment.

This is an implementation requirement and should be documented in
DtlsIdentityStoreInterface which is referenced here.
Unfortunately, this is not documented. I can add a comment at
DtlsIdentityStoreInterface (dtlsidentitystore.h) but that's in the WebRTC repo
and would not be part of this CL.

https://codereview.chromium.org/1373023002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl (right):

https://codereview.chromium.org/1373023002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCCertificate.idl:31: // The
RTCCertificate based on the WebRTC specs, see
On 2015/10/19 10:41:17, Guido Urdaneta wrote:
> nit: The more common style for this in blink is to just use the next line, and
> use https://.

Done.

https://codereview.chromium.org/1373023002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1373023002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:481: // This
creates a dependency between the Blink and WebCrypto modules (currently not
allowed).
On 2015/10/19 10:41:17, Guido Urdaneta wrote:
> reference a crbug

Done.

https://codereview.chromium.org/1373023002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl
(right):

https://codereview.chromium.org/1373023002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl:136: //
TODO(hbos): Use AlgorithmIdentifier and the WebCryptoAPI normalization process.
On 2015/10/19 10:41:17, Guido Urdaneta wrote:
> reference crbug

Done.

[jochen (gone - plz use gerrit), 2015-10-20 12:27:35]

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
File content/renderer/media/peer_connection_identity_store.cc (right):

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:138:
base::Bind(&ObserverOnSuccess, observer, base::Passed(&identity)));
why not just base::Bind(&webrtc::DtlsIdentityRequestObserver::OnSuccess,
observer, base::Passed(&identityt))

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
File content/renderer/media/rtc_certificate.h (right):

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
content/renderer/media/rtc_certificate.h:25: RTCCertificate* shallowCopy() const
override;
please add a comment like // blink::WebRTCCertificate implementation.

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
File content/renderer/media/rtc_certificate_generator.cc (right):

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.cc:26: CHECK(false);
NOTREACHED()

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
File content/renderer/media/rtc_certificate_generator.h (right):

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.h:23: void generateCertificate(
same here

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
content/renderer/media/rtc_peer_connection_handler.cc:260: for (size_t i = 0; i
< blink_config.numberOfCertificates(); ++i) {
use a range for loop

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:578: *out = nullptr;
please don't change code unrelated to your CL

https://codereview.chromium.org/1373023002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1373023002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:97: static
WebRTCCertificateObserver* Create(ScriptPromiseResolver* resolver)
blink coding style would be "create"

https://codereview.chromium.org/1373023002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:252: &&
!certificates.isUndefinedOrNull()) {
no 80c line length limit in blink

https://codereview.chromium.org/1373023002/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCCertificate.h (right):

https://codereview.chromium.org/1373023002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificate.h:2: * Copyright (C) 2015
Google Inc. All rights reserved.
please use a short form header

https://codereview.chromium.org/1373023002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificate.h:36: namespace rtc {
blink shouldn't know about rtc

[hbos_chromium, 2015-10-20 15:42:23]

hbos@chromium.org changed reviewers:
+ rsleevi@chromium.org

[hbos_chromium, 2015-10-20 15:42:24]

Thanks for the reviews! Addressed jochen's comments. Sorry for the size of the
CL.

Ryan, please take a look if you want to give additional feedback. In regards to
the webcrypto stuff (RTCPeerConnection.generateCertificate with
AlgorithmIdentifier) I plan to address this in a follow-up CL of which I'll add
you as a reviewer.

PTAL eroman. Ryan did you want Eric to look at something in particular? The
Blink stuff?

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
File content/renderer/media/peer_connection_identity_store.cc (right):

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
content/renderer/media/peer_connection_identity_store.cc:138:
base::Bind(&ObserverOnSuccess, observer, base::Passed(&identity)));
On 2015/10/20 12:27:35, jochen wrote:
> why not just base::Bind(&webrtc::DtlsIdentityRequestObserver::OnSuccess,
> observer, base::Passed(&identityt))

OnSuccess takes rtc::scoped_ptr (not scoped_ptr without rtc::) which does not
work with base::Passed.

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
File content/renderer/media/rtc_certificate.h (right):

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
content/renderer/media/rtc_certificate.h:25: RTCCertificate* shallowCopy() const
override;
On 2015/10/20 12:27:35, jochen wrote:
> please add a comment like // blink::WebRTCCertificate implementation.

Done.

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
File content/renderer/media/rtc_certificate_generator.cc (right):

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.cc:26: CHECK(false);
On 2015/10/20 12:27:35, jochen wrote:
> NOTREACHED()

Done.

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
File content/renderer/media/rtc_certificate_generator.h (right):

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
content/renderer/media/rtc_certificate_generator.h:23: void generateCertificate(
On 2015/10/20 12:27:35, jochen wrote:
> same here

Done.

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/media...
content/renderer/media/rtc_peer_connection_handler.cc:260: for (size_t i = 0; i
< blink_config.numberOfCertificates(); ++i) {
On 2015/10/20 12:27:35, jochen wrote:
> use a range for loop

The certificates of WebRTCConfiguration is not a collection that you can do a
range-based for loop on, it's just a get-by-index and numberOfCertificates()
method. Iterating it the same way .iceServers() are iterated @ line 203. Do you
want me to change this?

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1373023002/diff/220001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:578: *out = nullptr;
On 2015/10/20 12:27:35, jochen wrote:
> please don't change code unrelated to your CL

Acknowledged. Changed back to old NULL usage.

https://codereview.chromium.org/1373023002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1373023002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:97: static
WebRTCCertificateObserver* Create(ScriptPromiseResolver* resolver)
On 2015/10/20 12:27:35, jochen wrote:
> blink coding style would be "create"

Done.

https://codereview.chromium.org/1373023002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:252: &&
!certificates.isUndefinedOrNull()) {
On 2015/10/20 12:27:35, jochen wrote:
> no 80c line length limit in blink

Done.

https://codereview.chromium.org/1373023002/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCCertificate.h (right):

https://codereview.chromium.org/1373023002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificate.h:2: * Copyright (C) 2015
Google Inc. All rights reserved.
On 2015/10/20 12:27:35, jochen wrote:
> please use a short form header

Done.

https://codereview.chromium.org/1373023002/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCCertificate.h:36: namespace rtc {
On 2015/10/20 12:27:35, jochen wrote:
> blink shouldn't know about rtc

Done.

[hbos_chromium, 2015-10-21 07:41:02]

hbos@chromium.org changed reviewers:
- mkwst@chromium.org, rsleevi@chromium.org

[jochen (gone - plz use gerrit), 2015-10-21 13:40:23]

lgtm once eroman approves

[eroman, 2015-10-21 22:55:51]

LGTM assuming there is follow-up to switching the AlgorithmIdentifier parsing
over to blink::normalizeAlgorithm().

https://codereview.chromium.org/1373023002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1373023002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:480: // This
may create a dependency between the Blink and WebCrypto modules?
crbug.com/544917
As discussed over email, depending on NormalizeAlgorithm.h is fine.

https://codereview.chromium.org/1373023002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:483: if
(DictionaryHelper::get(keygenAlgorithm, "name", name)) {
As-written the AlgorithmIdentifier parsing code here has two main problems:

  (1) publicExponent must be a BigInteger and not a Number
(https://github.com/w3c/webrtc-pc/issues/310)
  (2) Algorithm names must be case-insensitive (so for instance EcDsA should
also be recognized)

If your intention is to submit this changelist as-is as a temporary measure, and
then follow-up quickly with a switchover to calling WebCrypto's parsing
algorithm (which should actually involve less code for more features), that is
fine -- I am happy to review that as a separate CL.

https://codereview.chromium.org/1373023002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:508:
scriptState, DOMException::create(InvalidAccessError,
ExceptionMessages::argumentNullOrIncorrectType(1, "AlgorithmIdentifier")));
According to the WebRTC spec you should be failing a number of these with
NotSupported error.
(Or if the WebCrypto algorithm normalization failed, then return that error.)

For instance keyParams will be null when using a P-384, and subsequently result
in InvalidAcessError when it should be NotSupported.

https://codereview.chromium.org/1373023002/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCKeyParams.h (right):

https://codereview.chromium.org/1373023002/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCKeyParams.h:48: enum WebRTCECCurve {
WebRTCECCurveNistP256 };
Is it expected that this will always be fixed to P-256?

Or could this instead use WebCryptoNamedCurve:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[hbos_chromium, 2015-10-22 08:43:15]

The CQ bit was checked by hbos@chromium.org

[hbos_chromium, 2015-10-22 08:43:16]

The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org,
philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1373023002/#ps240001
(title: "Addressed jochen's comments")

[commit-bot: I haz the power, 2015-10-22 08:43:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373023002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373023002/240001

[hbos_chromium, 2015-10-22 09:40:57]

Thanks everyone! (Sorry for not splitting the CL up into more easily reviewable
pieces.)

https://codereview.chromium.org/1373023002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1373023002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:480: // This
may create a dependency between the Blink and WebCrypto modules?
crbug.com/544917
On 2015/10/21 22:55:50, eroman wrote:
> As discussed over email, depending on NormalizeAlgorithm.h is fine.

Acknowledged.

https://codereview.chromium.org/1373023002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:483: if
(DictionaryHelper::get(keygenAlgorithm, "name", name)) {
On 2015/10/21 22:55:50, eroman wrote:
> As-written the AlgorithmIdentifier parsing code here has two main problems:
> 
>   (1) publicExponent must be a BigInteger and not a Number
> (https://github.com/w3c/webrtc-pc/issues/310)
>   (2) Algorithm names must be case-insensitive (so for instance EcDsA should
> also be recognized)
> 
> If your intention is to submit this changelist as-is as a temporary measure,
and
> then follow-up quickly with a switchover to calling WebCrypto's parsing
> algorithm (which should actually involve less code for more features), that is
> fine -- I am happy to review that as a separate CL.

That's what I'll do :) Btw there is a third problem. WebCrypto requires RSA to
have a "hash" value too: https://github.com/w3c/webrtc-pc/issues/322.

https://codereview.chromium.org/1373023002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:508:
scriptState, DOMException::create(InvalidAccessError,
ExceptionMessages::argumentNullOrIncorrectType(1, "AlgorithmIdentifier")));
On 2015/10/21 22:55:51, eroman wrote:
> According to the WebRTC spec you should be failing a number of these with
> NotSupported error.
> (Or if the WebCrypto algorithm normalization failed, then return that error.)
> 
> For instance keyParams will be null when using a P-384, and subsequently
result
> in InvalidAcessError when it should be NotSupported.

Acknowledged. To be addressed in follow-up.

https://codereview.chromium.org/1373023002/diff/240001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebRTCKeyParams.h (right):

https://codereview.chromium.org/1373023002/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebRTCKeyParams.h:48: enum WebRTCECCurve {
WebRTCECCurveNistP256 };
On 2015/10/21 22:55:51, eroman wrote:
> Is it expected that this will always be fixed to P-256?
> 
> Or could this instead use WebCryptoNamedCurve:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The current standard only says we must support P-256. Whether this will change
in the future I do not know. However, the webrtc repo does not know about
webcrypto enums and I'm making this enum correspond to the webrtc enum for easy
conversion.

[commit-bot: I haz the power, 2015-10-22 10:26:29]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2015-10-22 10:27:50]

Patchset 11 (id:??) landed as
https://crrev.com/ef4ab6012fddc80992b8848cfc6700e731dc39b1
Cr-Commit-Position: refs/heads/master@{#355520}

[eroman, 2015-10-22 17:49:49]

https://codereview.chromium.org/1373023002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1373023002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:483: if
(DictionaryHelper::get(keygenAlgorithm, "name", name)) {
On 2015/10/22 09:40:56, hbos_chromium wrote:
> On 2015/10/21 22:55:50, eroman wrote:
> > As-written the AlgorithmIdentifier parsing code here has two main problems:
> > 
> >   (1) publicExponent must be a BigInteger and not a Number
> > (https://github.com/w3c/webrtc-pc/issues/310)
> >   (2) Algorithm names must be case-insensitive (so for instance EcDsA should
> > also be recognized)
> > 
> > If your intention is to submit this changelist as-is as a temporary measure,
> and
> > then follow-up quickly with a switchover to calling WebCrypto's parsing
> > algorithm (which should actually involve less code for more features), that
is
> > fine -- I am happy to review that as a separate CL.
> 
> That's what I'll do :) Btw there is a third problem. WebCrypto requires RSA to
> have a "hash" value too: https://github.com/w3c/webrtc-pc/issues/322.

Oh right, that too ... Yuck!