RTCCertificate and RTCPeerConnection.generateCertificate added to JavaScript

--- Replaced by: https://codereview.chromium.org/1373023002/ ---

Adds a RTCCertificate JavaScript class and RTCPeerConnection.generateCertificate JavaScript method.

Interfaces are added for certificates and certificate generation (WebRTCCertificate, WebRTCCertificateGenerator) so that Chromium will be able to implement these to use WebRTC code.

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

BUG=528250, webrtc:4927

[hbos_chromium, 2015-09-02 12:36:35]

Patchset #2 (id:20001) has been deleted

[hbos_chromium, 2015-09-02 12:47:30]

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

[hbos_chromium, 2015-09-02 13:00:33]

guidou@ can you take a look?

The referenced bug gives some info about the bigger picture including a link to
the RFC, but I'd be happy to summarize it for you so that you don't have to read
a bunch of stuff.

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
File Source/modules/mediastream/RTCCertificate.h (right):

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
Source/modules/mediastream/RTCCertificate.h:45: double expires() const;
Is this the correct return type for Date in JavaScript?

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
Source/modules/mediastream/RTCCertificate.h:58: WebRTCCertificate*
m_certificate;
Equivalent of rtc::scoped_ptr?

[Guido Urdaneta, 2015-09-03 11:07:49]

This is a first round of comments.
Let's talk after you take a look at them.

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
File Source/modules/mediastream/RTCCertificate.cpp (right):

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
Source/modules/mediastream/RTCCertificate.cpp:48: delete m_certificate;
Use OwnPtr and leave this destructor empty.

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
File Source/modules/mediastream/RTCCertificate.h (right):

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
Source/modules/mediastream/RTCCertificate.h:45: double expires() const;
On 2015/09/02 13:00:33, hbos_chromium wrote:
> Is this the correct return type for Date in JavaScript?

Based on other uses of Date in blink, double is the correct type.

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
Source/modules/mediastream/RTCCertificate.h:58: WebRTCCertificate*
m_certificate;
On 2015/09/02 13:00:33, hbos_chromium wrote:
> Equivalent of rtc::scoped_ptr?

Use OwnPtr. Instructions on how to use it are here:
http://spinwhale.blogspot.se/2011/12/ownptr-and-passownptr.html

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:430:
GenerateRTCCertificateObserver(ScriptPromiseResolver* resolver)
Constructor should be private and you should have a public create method.

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:499: delete
certificateGenerator;
Is it safe to delete |certificateGenerator| here?

https://codereview.chromium.org/1311853005/diff/40001/Source/platform/Runtime...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1311853005/diff/40001/Source/platform/Runtime...
Source/platform/RuntimeEnabledFeatures.in:135: RTCCertificate
status=experimental
If this landed, will this feature work? (i.e., does the chrome part do
anything?)
If not, use status=test and change to experimental when the chrome side works.

https://codereview.chromium.org/1311853005/diff/40001/Source/platform/mediast...
File Source/platform/mediastream/RTCConfiguration.h (right):

https://codereview.chromium.org/1311853005/diff/40001/Source/platform/mediast...
Source/platform/mediastream/RTCConfiguration.h:92: ~RTCConfiguration()
Implement this in the .cpp

https://codereview.chromium.org/1311853005/diff/40001/Source/platform/mediast...
Source/platform/mediastream/RTCConfiguration.h:123:
std::vector<WebRTCCertificate*> m_certificates;
Apparently Vector is preferred to std::vector in blink.

https://codereview.chromium.org/1311853005/diff/40001/public/platform/WebRTCC...
File public/platform/WebRTCCertificate.h (right):

https://codereview.chromium.org/1311853005/diff/40001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:44: WebRTCCertificate() { }
You don't need this constructor.

https://codereview.chromium.org/1311853005/diff/40001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:45: virtual ~WebRTCCertificate() { }
= default is preferred.

https://codereview.chromium.org/1311853005/diff/40001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:47: WebRTCCertificate& operator=(const
WebRTCCertificate&) = delete;
To prevent copying you can use WTF_MAKE_NONCOPYABLE, but no class in
public/platform has that.

https://codereview.chromium.org/1311853005/diff/40001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:59: class WebRTCCertificateObserver {
What about using the generic WebCallbacks?

[hbos_chromium, 2015-09-03 16:01:36]

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

[hbos_chromium, 2015-09-03 16:01:37]

PTAL mkwst

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
File Source/modules/mediastream/RTCCertificate.cpp (right):

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
Source/modules/mediastream/RTCCertificate.cpp:48: delete m_certificate;
On 2015/09/03 11:07:49, Guido Urdaneta wrote:
> Use OwnPtr and leave this destructor empty.

Done.

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
File Source/modules/mediastream/RTCCertificate.h (right):

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
Source/modules/mediastream/RTCCertificate.h:45: double expires() const;
On 2015/09/03 11:07:49, Guido Urdaneta wrote:
> On 2015/09/02 13:00:33, hbos_chromium wrote:
> > Is this the correct return type for Date in JavaScript?
> 
> Based on other uses of Date in blink, double is the correct type.

Acknowledged.

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
Source/modules/mediastream/RTCCertificate.h:58: WebRTCCertificate*
m_certificate;
On 2015/09/03 11:07:49, Guido Urdaneta wrote:
> On 2015/09/02 13:00:33, hbos_chromium wrote:
> > Equivalent of rtc::scoped_ptr?
> 
> Use OwnPtr. Instructions on how to use it are here:
> http://spinwhale.blogspot.se/2011/12/ownptr-and-passownptr.html

Done. Thanks for the link, I was confused about OwnPtr usage.

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:430:
GenerateRTCCertificateObserver(ScriptPromiseResolver* resolver)
On 2015/09/03 11:07:49, Guido Urdaneta wrote:
> Constructor should be private and you should have a public create method.

Done.

https://codereview.chromium.org/1311853005/diff/40001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:499: delete
certificateGenerator;
On 2015/09/03 11:07:49, Guido Urdaneta wrote:
> Is it safe to delete |certificateGenerator| here?

Yes the object need only live for the generateCertificate call. Perhaps I should
use OwnPtr here as well.

https://codereview.chromium.org/1311853005/diff/40001/Source/platform/Runtime...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1311853005/diff/40001/Source/platform/Runtime...
Source/platform/RuntimeEnabledFeatures.in:135: RTCCertificate
status=experimental
On 2015/09/03 11:07:49, Guido Urdaneta wrote:
> If this landed, will this feature work? (i.e., does the chrome part do
> anything?)
> If not, use status=test and change to experimental when the chrome side works.

Done.

https://codereview.chromium.org/1311853005/diff/40001/Source/platform/mediast...
File Source/platform/mediastream/RTCConfiguration.h (right):

https://codereview.chromium.org/1311853005/diff/40001/Source/platform/mediast...
Source/platform/mediastream/RTCConfiguration.h:92: ~RTCConfiguration()
On 2015/09/03 11:07:49, Guido Urdaneta wrote:
> Implement this in the .cpp

I'm having trouble compiling after adding a .cpp file (cannot link
libmodules.so)... I'll address this later before landing this CL.

https://codereview.chromium.org/1311853005/diff/40001/Source/platform/mediast...
Source/platform/mediastream/RTCConfiguration.h:123:
std::vector<WebRTCCertificate*> m_certificates;
On 2015/09/03 11:07:49, Guido Urdaneta wrote:
> Apparently Vector is preferred to std::vector in blink.

Done.

https://codereview.chromium.org/1311853005/diff/40001/public/platform/WebRTCC...
File public/platform/WebRTCCertificate.h (right):

https://codereview.chromium.org/1311853005/diff/40001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:44: WebRTCCertificate() { }
On 2015/09/03 11:07:49, Guido Urdaneta wrote:
> You don't need this constructor.

Actually because I've defined other constructors (as deleted) I need to
explicitly add the default constructor or else I get this error message:
"constructor for 'content::RTCCertificate' must explicitly initialize the base
class 'blink::WebRTCCertificate' which does not have a default constructor"

Updated to "= default" with constructor and destructor.

https://codereview.chromium.org/1311853005/diff/40001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:45: virtual ~WebRTCCertificate() { }
On 2015/09/03 11:07:49, Guido Urdaneta wrote:
> = default is preferred.

Done.

https://codereview.chromium.org/1311853005/diff/40001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:47: WebRTCCertificate& operator=(const
WebRTCCertificate&) = delete;
On 2015/09/03 11:07:49, Guido Urdaneta wrote:
> To prevent copying you can use WTF_MAKE_NONCOPYABLE, but no class in
> public/platform has that.

Ok. I'll use it and see if anyone complains.

https://codereview.chromium.org/1311853005/diff/40001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:59: class WebRTCCertificateObserver {
On 2015/09/03 11:07:49, Guido Urdaneta wrote:
> What about using the generic WebCallbacks?

Done.

[hbos_chromium, 2015-09-03 17:11:47]

"Mike West (buried)" - Oh sorry, buried as in buried in work? Should I find
someone else to review this? Need an owner is all :)

[Mike West, 2015-09-04 08:03:14]

On 2015/09/03 at 17:11:47, hbos wrote:
> "Mike West (buried)" - Oh sorry, buried as in buried in work? Should I find
someone else to review this? Need an owner is all :)

I can stamp it once someone familiar with the WebRTC code is happy with it. :)

Have you sent an intent to implement to blink-dev@?

[hbos_chromium, 2015-09-07 11:58:46]

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

[hbos_chromium, 2015-09-07 11:58:46]

jochen, can you take a look?
It would be good to have someone who knows blink reviewing this CL (first time
blink-ing for me). tommi said you've reviewed some of our (WebRTC) stuff before.

re: mike: I just sent out an intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3Pps95Q6zRQ

[jochen (gone - plz use gerrit), 2015-09-09 14:44:01]

hum, I wonder whether this API can share infrastructure with webcrypto code?

I added rsleevi@ explicitly to the intent to implement thread to spark some
discussion

[hbos_chromium, 2015-09-14 09:58:06]

PTAL jochen
- About a week has passed since by blink-dev intent to implement post (with some
activity, see
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/3Pps95Q6zRQ),
will you take a look at the CL now?

[jochen (gone - plz use gerrit), 2015-09-15 08:04:19]

some comments

https://codereview.chromium.org/1311853005/diff/60001/Source/modules/mediastr...
File Source/modules/mediastream/RTCCertificate.idl (right):

https://codereview.chromium.org/1311853005/diff/60001/Source/modules/mediastr...
Source/modules/mediastream/RTCCertificate.idl:36: readonly attribute Date
expires;
uh, I see this poor name comes from the spec :-/

and what does it mean that it may not be reflected in the cert's notAfter value?

https://codereview.chromium.org/1311853005/diff/60001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1311853005/diff/60001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:257:
rtcConfiguration->appendCertificate(certificate->certificateShallowCopy());
why do you append a copy instead of just keeping the original?

https://codereview.chromium.org/1311853005/diff/60001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.idl (right):

https://codereview.chromium.org/1311853005/diff/60001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.idl:54:
[RuntimeEnabled=RTCCertificate, RaisesException, CallWith=ScriptState] static
Promise<RTCCertificate> generateCertificate(Dictionary keygenAlgorithm);
this should be AlgorithmIdentifier, not Dictionary, no?

https://codereview.chromium.org/1311853005/diff/60001/Source/platform/mediast...
File Source/platform/mediastream/RTCConfiguration.cpp (right):

https://codereview.chromium.org/1311853005/diff/60001/Source/platform/mediast...
Source/platform/mediastream/RTCConfiguration.cpp:36: // TODO(hbos):
RTCConfiguration::~RTCConfiguration() here.
yes please

https://codereview.chromium.org/1311853005/diff/60001/Source/platform/mediast...
File Source/platform/mediastream/RTCConfiguration.h (right):

https://codereview.chromium.org/1311853005/diff/60001/Source/platform/mediast...
Source/platform/mediastream/RTCConfiguration.h:91: {
non-trivial dtors shouldn't go into the header

https://codereview.chromium.org/1311853005/diff/60001/public/platform/WebRTCC...
File public/platform/WebRTCCertificate.h (right):

https://codereview.chromium.org/1311853005/diff/60001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:43: // be freed.
why is this restriction required?

https://codereview.chromium.org/1311853005/diff/60001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:44: class WebRTCCertificate {
should there be a method for getting the keyType?

https://codereview.chromium.org/1311853005/diff/60001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:53: virtual WebRTCCertificate* shallowCopy()
const = 0;
does it matter that it's a shallow copy?

https://codereview.chromium.org/1311853005/diff/60001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:55: virtual double expires() const = 0;
it's unclear what this should return. please add a comment, and rename the
method to something like

notValidAfterTimestampInMS()

or whatever this means.

shouldn't this also have a notValidBeforeTimestampInMS()?

https://codereview.chromium.org/1311853005/diff/60001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:59: class WebRTCCertificateGenerator {
please move this class to a separate header

https://codereview.chromium.org/1311853005/diff/60001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:66: int keyType,
should be an enum? (which in turn should live in its own header)

[hbos_chromium, 2015-09-17 16:19:39]

I'm having build/gyp trouble moving the destructor of RTCConfiguration.h into
RTCConfiguration.cpp (new file).

Having added RTCConfiguration.cpp to blink_platform.gypi I'm getting undefined
reference link error to the destructor.
(This seemed odd because it's part of 'blink_platform' and I can build this
target separately just fine. It fails on 'SOLINK lib/libmodules.so'. Building
'modules' fails with the same error.)

I see that 'modules' dependency on 'blink_platform' is conditional!
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Building with 'component=="shared_library" and link_core_modules_separately==1'
works fine.
Building with neither also works fine.
But building with 'component=="shared_library"' alone, which is what I was
doing, gives the link error.

I'm a little confused. I'm tempted to simply make the dependencies of interest
non-conditional but maybe I'm not supposed to do that.
- Do you happen to know anything about this/link_core_modules_separately?

[jochen (gone - plz use gerrit), 2015-09-18 08:27:04]

On 2015/09/17 at 16:19:39, hbos wrote:
> I'm having build/gyp trouble moving the destructor of RTCConfiguration.h into
RTCConfiguration.cpp (new file).
> 
> Having added RTCConfiguration.cpp to blink_platform.gypi I'm getting undefined
reference link error to the destructor.
> (This seemed odd because it's part of 'blink_platform' and I can build this
target separately just fine. It fails on 'SOLINK lib/libmodules.so'. Building
'modules' fails with the same error.)
> 
> I see that 'modules' dependency on 'blink_platform' is conditional!
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Building with 'component=="shared_library" and
link_core_modules_separately==1' works fine.
> Building with neither also works fine.
> But building with 'component=="shared_library"' alone, which is what I was
doing, gives the link error.
> 
> I'm a little confused. I'm tempted to simply make the dependencies of interest
non-conditional but maybe I'm not supposed to do that.
> - Do you happen to know anything about this/link_core_modules_separately?

i guess you have to put some MODULES_EXPORT macros on the classes

[hbos_chromium, 2015-09-21 16:12:22]

PTAL jochen, I've addressed your comments.

(With patch failures, I will merge and re-run the trybots tomorrow, but now I'm
heading home. Time zone differences, best to get the review out before then.)

https://codereview.chromium.org/1311853005/diff/60001/Source/modules/mediastr...
File Source/modules/mediastream/RTCCertificate.idl (right):

https://codereview.chromium.org/1311853005/diff/60001/Source/modules/mediastr...
Source/modules/mediastream/RTCCertificate.idl:36: readonly attribute Date
expires;
On 2015/09/15 08:04:18, jochen wrote:
> uh, I see this poor name comes from the spec :-/
> 
> and what does it mean that it may not be reflected in the cert's notAfter
value?

Indeed it does. Uhm, I'm not sure... notAfter is only mentioned once in the
spec, as a keyword, it's never defined... I could only guess what it might mean.

I added a comment here and in .h file clarifying the purpose of expires.

https://codereview.chromium.org/1311853005/diff/60001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1311853005/diff/60001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.cpp:257:
rtcConfiguration->appendCertificate(certificate->certificateShallowCopy());
On 2015/09/15 08:04:19, jochen wrote:
> why do you append a copy instead of just keeping the original?

The WebRTCCertificate is owned by the JavaScript object RTCCertificate and will
be deleted when RTCCertificate is deleted in a garbage collection event. If we
hand out raw pointers there may be a risk that we end up with dangling pointers.
It looks to me like |rtcConfiguration| has a life cycle independent of the
RTCCertificate object and |rtcConfiguration| has a list of certs.

Reference counting could protect the object against being destroyed, but this
has to work across blink and chrome layers.

By creating a shallow copy, I can leave it up to the implementation details to
ensure that the internal data (webrtc layer certificate) is protected by
reference counting. Any shallow copy will have a reference to the webrtc layer
certificate and as long as there is at least one shallow copy alive this data
will not be destroyed.
(The chromium implementation of WebRTCCertificate will have a webrtc type
rtc::scoped_refptr<rtc::RTCCertificate> member).

Alternatively, is there a way to make WebRTCCertificate reference counted so
that it can be referenced and used across both blink and chrome layers? That
would work just as well as shallow copying and be less confusing.

https://codereview.chromium.org/1311853005/diff/60001/Source/modules/mediastr...
File Source/modules/mediastream/RTCPeerConnection.idl (right):

https://codereview.chromium.org/1311853005/diff/60001/Source/modules/mediastr...
Source/modules/mediastream/RTCPeerConnection.idl:54:
[RuntimeEnabled=RTCCertificate, RaisesException, CallWith=ScriptState] static
Promise<RTCCertificate> generateCertificate(Dictionary keygenAlgorithm);
On 2015/09/15 08:04:19, jochen wrote:
> this should be AlgorithmIdentifier, not Dictionary, no?

You're right. It should be a WebCryptoAPI AlgorithmIdentifier, which is an
object (Dictionary) or DOMString that is parsed. I took a look at this after
your comment.

Currently I'm doing the parsing myself. What I should be using is the
WebCryptoAPI normalization process which parses it for us. I tried to do this,
but I believe I've found a WebCryptoAPI bug. If I try to parse what the WebRTC
spec says must be supported, normalizeAlgorithm() complains that the
publicExponent parameter is not an Uint8Array. I have filed a bug
(crbug.com/534416) with rsleevi@ CC'd.

In the meantime I've added a TODO.

https://codereview.chromium.org/1311853005/diff/60001/Source/platform/mediast...
File Source/platform/mediastream/RTCConfiguration.cpp (right):

https://codereview.chromium.org/1311853005/diff/60001/Source/platform/mediast...
Source/platform/mediastream/RTCConfiguration.cpp:36: // TODO(hbos):
RTCConfiguration::~RTCConfiguration() here.
On 2015/09/15 08:04:19, jochen wrote:
> yes please

Done.

https://codereview.chromium.org/1311853005/diff/60001/Source/platform/mediast...
File Source/platform/mediastream/RTCConfiguration.h (right):

https://codereview.chromium.org/1311853005/diff/60001/Source/platform/mediast...
Source/platform/mediastream/RTCConfiguration.h:91: {
On 2015/09/15 08:04:19, jochen wrote:
> non-trivial dtors shouldn't go into the header

Done. I saw that nearby classes uses PLATFORM_EXPORT so that's what I used too
and it worked. (Should it be MODULES_EXPORT instead?)

https://codereview.chromium.org/1311853005/diff/60001/public/platform/WebRTCC...
File public/platform/WebRTCCertificate.h (right):

https://codereview.chromium.org/1311853005/diff/60001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:43: // be freed.
On 2015/09/15 08:04:19, jochen wrote:
> why is this restriction required?

See previous message about why the shallow copying. Either WebRTCCertificate
itself should be reference counted or its internal data should (hence shallow
copying).
Updated comment.

https://codereview.chromium.org/1311853005/diff/60001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:44: class WebRTCCertificate {
On 2015/09/15 08:04:19, jochen wrote:
> should there be a method for getting the keyType?

Done.

https://codereview.chromium.org/1311853005/diff/60001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:53: virtual WebRTCCertificate* shallowCopy()
const = 0;
On 2015/09/15 08:04:19, jochen wrote:
> does it matter that it's a shallow copy?

(Originals and shallow copies are "equals". Besides being an abstraction layer
they're basically just a way to have reference counting to the internal data.
See previous message addressing why the shallow copying.)

https://codereview.chromium.org/1311853005/diff/60001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:55: virtual double expires() const = 0;
On 2015/09/15 08:04:19, jochen wrote:
> it's unclear what this should return. please add a comment, and rename the
> method to something like
> 
> notValidAfterTimestampInMS()
> 
> or whatever this means.
> 
> shouldn't this also have a notValidBeforeTimestampInMS()?

Keeping the name due to the specs so that everything calls it the same thing,
not just RTCCertificate. (Is that OK?)
Added a comment that should clear up any confusion.

https://codereview.chromium.org/1311853005/diff/60001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:59: class WebRTCCertificateGenerator {
On 2015/09/15 08:04:19, jochen wrote:
> please move this class to a separate header

Done.

https://codereview.chromium.org/1311853005/diff/60001/public/platform/WebRTCC...
public/platform/WebRTCCertificate.h:66: int keyType,
On 2015/09/15 08:04:19, jochen wrote:
> should be an enum? (which in turn should live in its own header)

Moved to new file and changed to parameterizable class (WebRTCKeyType) to
reflect how the KeyType will look in webrtc in the future.

https://codereview.chromium.org/1311853005/diff/80001/public/platform/WebRTCK...
File public/platform/WebRTCKeyType.h (right):

https://codereview.chromium.org/1311853005/diff/80001/public/platform/WebRTCK...
public/platform/WebRTCKeyType.h:37: enum WebRTCKeyFamily { KeyFamilyRsa,
KeyFamilyEcdsa, KeyFamilyLast };
Should even this enum be in its own header?

[jochen (gone - plz use gerrit), 2015-09-22 15:48:53]

some more comments.

https://codereview.chromium.org/1311853005/diff/100001/Source/modules/mediast...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1311853005/diff/100001/Source/modules/mediast...
Source/modules/mediastream/RTCPeerConnection.cpp:120: void onError()
should this also be marked as override?

https://codereview.chromium.org/1311853005/diff/100001/Source/modules/mediast...
Source/modules/mediastream/RTCPeerConnection.cpp:122:
m_resolver->resolve(static_cast<RTCCertificate*>(nullptr));
why not reject?

https://codereview.chromium.org/1311853005/diff/100001/Source/platform/medias...
File Source/platform/mediastream/RTCConfiguration.h (right):

https://codereview.chromium.org/1311853005/diff/100001/Source/platform/medias...
Source/platform/mediastream/RTCConfiguration.h:135: Vector<WebRTCCertificate*>
m_certificates;
why not Vector<OwnPtr<WebRTCCertificate>> ?

https://codereview.chromium.org/1311853005/diff/100001/public/platform/WebRTC...
File public/platform/WebRTCKeyType.h (right):

https://codereview.chromium.org/1311853005/diff/100001/public/platform/WebRTC...
public/platform/WebRTCKeyType.h:37: enum WebRTCKeyFamily { KeyFamilyRsa,
KeyFamilyEcdsa, KeyFamilyLast };
enum values should start with the enum's name, i.e. WebRTCKeyFamilyRsa, etc..

https://codereview.chromium.org/1311853005/diff/100001/public/platform/WebRTC...
public/platform/WebRTCKeyType.h:42: static WebRTCKeyType createRSA(int
modulusLength = 1024)
what's a negative modules? also, do we really need a default value here? and why
such a low length as default?

https://codereview.chromium.org/1311853005/diff/100001/public/platform/WebRTC...
public/platform/WebRTCKeyType.h:51: explicit WebRTCKeyType(WebRTCKeyFamily
family = KeyFamilyRsa)
if you have factory methods, the ctor should be private, no?

https://codereview.chromium.org/1311853005/diff/100001/public/platform/WebRTC...
public/platform/WebRTCKeyType.h:63: int m_parameter;
why do you store this, there's no getter for this

[hbos_chromium, 2015-09-24 12:02:35]

Patchset #6 (id:120001) has been deleted

[hbos_chromium, 2015-09-25 08:53:30]

Patchset #7 (id:160001) has been deleted

[hbos_chromium, 2015-09-25 09:39:31]

Patchset #7 (id:180001) has been deleted

[hbos_chromium, 2015-09-28 08:10:16]

PTAL jochen, guidou.

With Chrome and Blink merged I re-uploaded the Blink patch set. I then added
Chromium implementations of the Blink interfaces and addressed jochen's
comments.

https://codereview.chromium.org/1311853005/diff/100001/Source/modules/mediast...
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1311853005/diff/100001/Source/modules/mediast...
Source/modules/mediastream/RTCPeerConnection.cpp:120: void onError()
On 2015/09/22 15:48:53, jochen wrote:
> should this also be marked as override?

Done.

https://codereview.chromium.org/1311853005/diff/100001/Source/modules/mediast...
Source/modules/mediastream/RTCPeerConnection.cpp:122:
m_resolver->resolve(static_cast<RTCCertificate*>(nullptr));
On 2015/09/22 15:48:53, jochen wrote:
> why not reject?

Spec does not say what happens if we fail to generate (this should not happen
during normal circumstances). I agree reject makes more sense than returning
null. Changed to a parameterless reject().

I tried to specify an exception with an error message first but it crashes on
me, SEGV_MAPERR... Maybe the isolate is not valid anymore, I'm not sure.
// This and variations do not work
m_resolver->reject(v8::Exception::TypeError(v8String(
  m_resolver->scriptState()->isolate(),
  String("A rare error occurred during certificate generation."))));

https://codereview.chromium.org/1311853005/diff/100001/Source/platform/medias...
File Source/platform/mediastream/RTCConfiguration.h (right):

https://codereview.chromium.org/1311853005/diff/100001/Source/platform/medias...
Source/platform/mediastream/RTCConfiguration.h:135: Vector<WebRTCCertificate*>
m_certificates;
On 2015/09/22 15:48:53, jochen wrote:
> why not Vector<OwnPtr<WebRTCCertificate>> ?

Done.

https://codereview.chromium.org/1311853005/diff/100001/public/platform/WebRTC...
File public/platform/WebRTCKeyType.h (right):

https://codereview.chromium.org/1311853005/diff/100001/public/platform/WebRTC...
public/platform/WebRTCKeyType.h:37: enum WebRTCKeyFamily { KeyFamilyRsa,
KeyFamilyEcdsa, KeyFamilyLast };
On 2015/09/22 15:48:53, jochen wrote:
> enum values should start with the enum's name, i.e. WebRTCKeyFamilyRsa, etc..

Done.

https://codereview.chromium.org/1311853005/diff/100001/public/platform/WebRTC...
public/platform/WebRTCKeyType.h:42: static WebRTCKeyType createRSA(int
modulusLength = 1024)
On 2015/09/22 15:48:53, jochen wrote:
> what's a negative modules? also, do we really need a default value here? and
why
> such a low length as default?

Added a BLINK_ASSERT sanity check. Removed default value (previously used 1024
because that is what generation code inside of webrtc has been using).

https://codereview.chromium.org/1311853005/diff/100001/public/platform/WebRTC...
public/platform/WebRTCKeyType.h:51: explicit WebRTCKeyType(WebRTCKeyFamily
family = KeyFamilyRsa)
On 2015/09/22 15:48:53, jochen wrote:
> if you have factory methods, the ctor should be private, no?

Need a public default constructor if I want to use Nullable<WebRTCKeyType>
without setting a value. I introduced WebRTCKeyFamilyNull instead and made the
default constructor construct that.

I use Nullable<WebRTCKeyType> in RTCPeerConnection::generateCertificate. If you
don't like this I can get around that.

https://codereview.chromium.org/1311853005/diff/100001/public/platform/WebRTC...
public/platform/WebRTCKeyType.h:63: int m_parameter;
On 2015/09/22 15:48:53, jochen wrote:
> why do you store this, there's no getter for this

Added getter. It will be used to convert from blink::WebRTCKeyType to
rtc::KeyType when KeyType has become parameterized.

[jochen (gone - plz use gerrit), 2015-09-28 11:39:28]

the base URL of this CL is the now no longer existing blink.git repository.

as mentioned in the merge FAQ, you'll have to create a new CL :-/

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

On 2015/09/28 11:39:28, jochen wrote:
> the base URL of this CL is the now no longer existing blink.git repository.
> 
> as mentioned in the merge FAQ, you'll have to create a new CL :-/

OK. Reuploaded: https://codereview.chromium.org/1373023002/. Closing this one.