RTCCertificates added to RTCConfiguration, used by WebRtcSession/-DescriptionFactory

RTCCertificates added to RTCConfiguration, used by WebRtcSession/-DescriptionFactory.

This CL allows you to, having generated one or more RTCCertificates, supply them to RTCConfiguration for CreatePeerConnection use. This means an SSLIdentity does not have to be generated with a DtlsIdentityStore[Interface/Impl] as part of the CreatePeerConnection steps because the certificate contains all the necessary information.

To create an RTCCertificate you have to do the identity generation yourself though. But you could reuse the same RTCCertificate for multiple connections.

BUG=webrtc:4927
R=tommi@webrtc.org, torbjorng@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/87713d0fe6fb9c86abe501bdf3d26ef4287ee617

[hbos, 2015-08-20 13:02:07]

Patchset #1 (id:1) has been deleted

[hbos, 2015-08-20 13:06:25]

Patchset #1 (id:20001) has been deleted

[hbos, 2015-08-20 13:18:35]

hbos@webrtc.org changed reviewers:
+ tommi@webrtc.org, torbjorng@webrtc.org

[hbos, 2015-08-20 13:18:36]

I will update the webrtcsession unittests to test both using a store and using
certificates before I require this to be reviewed, but feel free to take a look
already if you like.

This allows certificates to be used in RTCConfiguration but instead of passing
the certificate around, its SSLIdentity is grabbed and passed on so that no
surrounding code has to be updated. Switching over from SSLIdentity* to
scoped_refptr<RTCCertificate> should be done in a series of future CLs.

[tommi, 2015-08-20 14:58:12]

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:724: if (!certificate) {
I don't think you need this variable actually.
Here you could just do

if (rtc_configuration.certificates.empty()) {

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:735: // Use the already generated certificate.
is the intent to support multiple certificates eventually?

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:740: certificate,
...and here you could pass rtc_configuration.certificates[0]

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:187: new
rtc::RefCountedObject<WebRtcIdentityRequestObserver>();
initialize in the initializer list?

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsessiondescriptionfactory.h (right):

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:99: // Construct with DTLS
enabled using the specified |dtls_identity_store| to
nit: add an empty line above this comment. same below. (for readability)

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:138: bool
waiting_for_certificate() const {
can we call this:

waiting_for_certificate_for_testing()

[hbos, 2015-08-20 15:51:36]

Patchset #2 (id:60001) has been deleted

[hbos, 2015-08-20 16:06:11]

PTAL.
webrtcsession_unittest has been parameterized, testing the usage of both store
and provided certificate. I have also addressed tommi's comments.

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:724: if (!certificate) {
On 2015/08/20 14:58:11, tommi (webrtc) wrote:
> I don't think you need this variable actually.
> Here you could just do
> 
> if (rtc_configuration.certificates.empty()) {

You're right. In latest PS though certificate != nullptr became relevant in two
places and the variable more necessary so I still have it but it's moved.

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:735: // Use the already generated certificate.
On 2015/08/20 14:58:11, tommi (webrtc) wrote:
> is the intent to support multiple certificates eventually?

The specs say if multiple are provided it is up to us to choose which one to use
but it doesn't say how to pick one. Unless there are compatibility issues(?)
this is one way to do it ;)

But yeah, me and torbjorng talked about this briefly. If which one to use
matters compatibility/security/performance-wise we should have a function that
decides which one to pick... (although performance-wise: at this point the
certificates have already been generated).

I'll add a TODO, forgot to do that.

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:187: new
rtc::RefCountedObject<WebRtcIdentityRequestObserver>();
On 2015/08/20 14:58:11, tommi (webrtc) wrote:
> initialize in the initializer list?

Done. This required moving it to helper constructor's initializer list and
initializing it there, but it allowed it to be made const.

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsessiondescriptionfactory.h (right):

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:99: // Construct with DTLS
enabled using the specified |dtls_identity_store| to
On 2015/08/20 14:58:11, tommi (webrtc) wrote:
> nit: add an empty line above this comment. same below. (for readability)

Done.

https://codereview.webrtc.org/1288033009/diff/40001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:138: bool
waiting_for_certificate() const {
On 2015/08/20 14:58:11, tommi (webrtc) wrote:
> can we call this:
> 
> waiting_for_certificate_for_testing()

Done.

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession_unittest.cc (left):

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:3882: 
Two things wrong with this, which is why I removed it:
- A unittest should not do things that will have consequences outside of the
test being teared down. Quitting the thread has the lasting consequence that the
thread has stopped. If other tests run after it and for example try to do Invoke
they will not succeed (and the fact that Invoke did nothing is not immediately
clear). Adding TEST_P's changed the order of some tests and exposed this issue.
(Previously this was always the last test being run).
- Quit() does not look like it is actually process pending messages(?), it just
marks it stopped and wakes the thread up (but it's already awake?).

With messages not being processed anyway I did not replace this by a message
processing loop.

[torbjorng (webrtc), 2015-08-21 12:39:19]

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:582: // TODO(hbos,torbjorng): How to decide
which certificate to use?
My understanding is that multiple certificates make sense for DTLS negotiations.
 Quoting from the RFC at 4.2.1.1:

"Although any given DTLS connection will use only one certificate, this
attribute allows the caller to provide multiple certificates that support
different algorithms. The final certificate will be selected based on the DTLS
handshake, which establishes which certificates are allowed. The
RTCPeerConnection implementation selects which of the certificates is used for a
given connection; how certificates are selected is outside the scope of this
specification."

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:728: if (!certificate) {
Is dtls_identity_store.Pass() and certificate type compatible?
Then, consider combining these calls via a conditionally assigned parameter.

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:410: void
InitWithDtls(RTCCertificateGenerationMethod cert_gen_method) {
Argument splits method into disjoints chunks. Consider splitting into two
methods, or else if possible parameterize and share code between then/else.

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:1211:
InitWithDtls(DTLS_IDENTITY_STORE);
Should we use both cert sources here (as provided by TEST_P)?

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:391: // SSLIdentity* (then
there will be no need to do GetReference here).
Perhaps we ought to rename GetReference to make things less confusing?

[tommi, 2015-08-21 14:24:39]

can you ping back after addressing Torbjorn's comments?

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface.h:246:
std::vector<rtc::scoped_refptr<rtc::RTCCertificate>> certificates;
This "struct" is outgrowing it's "structness".  Can you add a TODO for you, me
or whomever to change it to be a class and make the member variables private? 
Since we're now adding a vector, pointers to refcounted objects etc, it is
becoming more and more important to check correct usage of this type. (thread
checks etc)

[hbos, 2015-08-24 10:16:44]

PTAL, comments addressed.

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/peerconne...
File talk/app/webrtc/peerconnectioninterface.h (right):

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/peerconne...
talk/app/webrtc/peerconnectioninterface.h:246:
std::vector<rtc::scoped_refptr<rtc::RTCCertificate>> certificates;
On 2015/08/21 14:24:39, tommi (webrtc) wrote:
> This "struct" is outgrowing it's "structness".  Can you add a TODO for you, me
> or whomever to change it to be a class and make the member variables private? 
> Since we're now adding a vector, pointers to refcounted objects etc, it is
> becoming more and more important to check correct usage of this type. (thread
> checks etc)

Done.

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:582: // TODO(hbos,torbjorng): How to decide
which certificate to use?
On 2015/08/21 12:39:19, torbjorng1 wrote:
> My understanding is that multiple certificates make sense for DTLS
negotiations.
>  Quoting from the RFC at 4.2.1.1:
> 
> "Although any given DTLS connection will use only one certificate, this
> attribute allows the caller to provide multiple certificates that support
> different algorithms. The final certificate will be selected based on the DTLS
> handshake, which establishes which certificates are allowed. The
> RTCPeerConnection implementation selects which of the certificates is used for
a
> given connection; how certificates are selected is outside the scope of this
> specification."

Acknowledged. We must look into this more, major code changes might be necessary
including passing around a collection of certificates instead of a single one.
But leaving as-is for the current CL. Let's walk before we can run; let's
support ECDSA before we can do proper DTLS negotiations.

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession.cc:728: if (!certificate) {
On 2015/08/21 12:39:19, torbjorng1 wrote:
> Is dtls_identity_store.Pass() and certificate type compatible?
> Then, consider combining these calls via a conditionally assigned parameter.

You either use store and not certificate, or certificate and not store. If it
took both then it would have to ignore one of the parameters if the other is not
null (ignore store if cert != null). I originally made a constructor that took
both parameters but was asked in the previous version of these CLs to split it
up into different constructor calls.

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:410: void
InitWithDtls(RTCCertificateGenerationMethod cert_gen_method) {
On 2015/08/21 12:39:19, torbjorng1 wrote:
> Argument splits method into disjoints chunks. Consider splitting into two
> methods, or else if possible parameterize and share code between then/else.

Done.

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsession_unittest.cc:1211:
InitWithDtls(DTLS_IDENTITY_STORE);
On 2015/08/21 12:39:19, torbjorng1 wrote:
> Should we use both cert sources here (as provided by TEST_P)?

Yeah why not! Done.

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1288033009/diff/80001/talk/app/webrtc/webrtcses...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:391: // SSLIdentity* (then
there will be no need to do GetReference here).
On 2015/08/21 12:39:19, torbjorng1 wrote:
> Perhaps we ought to rename GetReference to make things less confusing?

Sure but let's do that in a separate CL, added a TODO.

[tommi, 2015-08-24 11:26:33]

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:729: // Use the |dtls_identity_store| to
generate a certificate.
can you add a DCHECK(dtls_identity_store)

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession_unittest.cc (left):

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession_unittest.cc:3881: rtc::Thread::Current()->Quit();
why removing this?

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:245: if (msg.message_id !=
MSG_USE_CONSTRUCTOR_CERTIFICATE)
{

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsessiondescriptionfactory.h (right):

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:102:
WebRtcSessionDescriptionFactory(
will this ctor eventually be deleted?

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:117:
rtc::scoped_refptr<rtc::RTCCertificate> certificate,
const &

[hbos, 2015-08-24 12:01:34]

PTAL, addressed comments again.

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:729: // Use the |dtls_identity_store| to
generate a certificate.
On 2015/08/24 11:26:32, tommi (webrtc) wrote:
> can you add a DCHECK(dtls_identity_store)

Done.

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession_unittest.cc (left):

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession_unittest.cc:3881: rtc::Thread::Current()->Quit();
On 2015/08/24 11:26:33, tommi (webrtc) wrote:
> why removing this?

I commented on this earlier:
"Two things wrong with this, which is why I removed it:
- A unittest should not do things that will have consequences outside of the
test being teared down. Quitting the thread has the lasting consequence that the
thread has stopped. If other tests run after it and for example try to do Invoke
they will not succeed (and the fact that Invoke did nothing is not immediately
clear). Adding TEST_P's changed the order of some tests and exposed this issue.
(Previously this was always the last test being run).
- Quit() does not look like it is actually processing pending messages(?), it
just marks it stopped and wakes the thread up (but it's already awake?).

With messages not being processed anyway I did not replace this by a message
processing loop."

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsessiondescriptionfactory.cc (right):

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.cc:245: if (msg.message_id !=
MSG_USE_CONSTRUCTOR_CERTIFICATE)
On 2015/08/24 11:26:33, tommi (webrtc) wrote:
> {

Done.

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsessiondescriptionfactory.h (right):

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:102:
WebRtcSessionDescriptionFactory(
On 2015/08/24 11:26:33, tommi (webrtc) wrote:
> will this ctor eventually be deleted?

Remember that if no certificates are provided we still have to generate them for
the caller, with the same functionality as before the latest RFC.

|dtls_identity_store| is required for as long as we want to allow projects using
WebRTC to be able to supply own identity generation code in this event. Such as
Chrome having its own RSA generation code today. It would be nice to remove it
but that will likely have performance implications.

In fact, for as long as this holds true, our GenerateCertificate function (not
yet added) will need to (optionally) take a |dtls_identity_store| argument as
well.

When ECDSA is the default and RSA is "almost abandoned" we might not care about
this and always use the webrtc implementation of the store, in which case they
can be removed (along with the possibility of Chrome generating keys before
WebRTC comes into the picture).

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsessiondescriptionfactory.h:117:
rtc::scoped_refptr<rtc::RTCCertificate> certificate,
On 2015/08/24 11:26:33, tommi (webrtc) wrote:
> const &

Done.

[tommi, 2015-08-24 15:15:12]

lgtm

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession_unittest.cc (left):

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession_unittest.cc:3881: rtc::Thread::Current()->Quit();
On 2015/08/24 12:01:34, hbos wrote:
> On 2015/08/24 11:26:33, tommi (webrtc) wrote:
> > why removing this?
> 
> I commented on this earlier:
> "Two things wrong with this, which is why I removed it:
> - A unittest should not do things that will have consequences outside of the
> test being teared down. Quitting the thread has the lasting consequence that
the
> thread has stopped. If other tests run after it and for example try to do
Invoke
> they will not succeed (and the fact that Invoke did nothing is not immediately
> clear). Adding TEST_P's changed the order of some tests and exposed this
issue.
> (Previously this was always the last test being run).

This thread is created by and for the test.  If other tests are relying on it,
aren't they broken or the harness itself?

> - Quit() does not look like it is actually processing pending messages(?), it
> just marks it stopped and wakes the thread up (but it's already awake?).
> 
> With messages not being processed anyway I did not replace this by a message
> processing loop."

As discussed offline, I'm going to look into fixing the issue whereby the
ThreadManager implicitly wraps the current thread and causes the main thread
object (and queue) to be shared by multiple different tests.  I think this sort
of thing could be causing flakiness.

[torbjorng (webrtc), 2015-08-24 15:34:34]

LGTM.

https://codereview.webrtc.org/1288033009/diff/120001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1288033009/diff/120001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:582: // TODO(hbos,torbjorng): How to decide
which certificate to use?
Please spell out the [0] uglyness in the comment.

[hbos, 2015-08-24 15:48:00]

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession_unittest.cc (left):

https://codereview.webrtc.org/1288033009/diff/100001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession_unittest.cc:3881: rtc::Thread::Current()->Quit();
On 2015/08/24 15:15:12, tommi (webrtc) wrote:
> On 2015/08/24 12:01:34, hbos wrote:
> > On 2015/08/24 11:26:33, tommi (webrtc) wrote:
> > > why removing this?
> > 
> > I commented on this earlier:
> > "Two things wrong with this, which is why I removed it:
> > - A unittest should not do things that will have consequences outside of the
> > test being teared down. Quitting the thread has the lasting consequence that
> the
> > thread has stopped. If other tests run after it and for example try to do
> Invoke
> > they will not succeed (and the fact that Invoke did nothing is not
immediately
> > clear). Adding TEST_P's changed the order of some tests and exposed this
> issue.
> > (Previously this was always the last test being run).
> 
> This thread is created by and for the test.  If other tests are relying on it,
> aren't they broken or the harness itself?
> 
> > - Quit() does not look like it is actually processing pending messages(?),
it
> > just marks it stopped and wakes the thread up (but it's already awake?).
> > 
> > With messages not being processed anyway I did not replace this by a message
> > processing loop."
> 
> As discussed offline, I'm going to look into fixing the issue whereby the
> ThreadManager implicitly wraps the current thread and causes the main thread
> object (and queue) to be shared by multiple different tests.  I think this
sort
> of thing could be causing flakiness.

Acknowledged.

https://codereview.webrtc.org/1288033009/diff/120001/talk/app/webrtc/webrtcse...
File talk/app/webrtc/webrtcsession.cc (right):

https://codereview.webrtc.org/1288033009/diff/120001/talk/app/webrtc/webrtcse...
talk/app/webrtc/webrtcsession.cc:582: // TODO(hbos,torbjorng): How to decide
which certificate to use?
On 2015/08/24 15:34:34, torbjorng1 wrote:
> Please spell out the [0] uglyness in the comment.

Done.

[hbos, 2015-08-24 15:48:31]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2015-08-24 15:48:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
torbjorng@webrtc.org, tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1288033009/#ps140001 (title:
"Addressed torbjorng's comment and merged with master")

[commit-bot: I haz the power, 2015-08-24 15:48:44]

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

[commit-bot: I haz the power, 2015-08-24 17:49:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-24 17:49:03]

Try jobs failed on following builders:
  android on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[hbos, 2015-08-25 07:53:41]

Committed patchset #5 (id:140001) manually as
87713d0fe6fb9c86abe501bdf3d26ef4287ee617 (presubmit successful).