WebRtcBrowserTest tests with RSA and ECDSA certificates.

WebRtcBrowserTest tests with RSA and ECDSA certificates.

- peerconnection.js/WebRtcTestBase: Ability to specify which
  certificate to use (or generate and use) when creating peer connections.
- WebRtcBrowserTest: Two new tests that use non-default certificates,
  generating RSA and ECDSA.

This gives integration test coverage to RSA and ECDSA, which is good
before switching so that ECDSA becomes default (previously only default
was tested).

Also, peerconnection.js's preparePeerConnectionWithCertificate can be
used in the future to ensure cloned certificates are usable for peer
connections. (When persistance and cloning of RTCCertificate is
supported.)

BUG=chromium:581354
Committed: https://crrev.com/e007ec047dfbbf54e54a211d53b1ce9ed6d5815c
Cr-Commit-Position: refs/heads/master@{#390096}

[hbos_chromium, 2016-04-26 10:03:04]

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

[hbos_chromium, 2016-04-26 10:03:05]

Please take a look, phoglund.

+CC: kjellander, jansson: FYI, integration tests for ECDSA!

https://codereview.chromium.org/1917133002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/1917133002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_browsertest.cc:117: // Default certificate, H264
video codec.
(oops extra spaces, will remove before landing)

[kjellander_chromium, 2016-04-26 10:39:10]

Description was changed from

==========
WebRtcBrowserTest tests with RSA and ECDSA certificates.

- peerconnection.js/WebRtcTestBase: Ability to specifiy which
  certificate to use (or generate and use) when creating peer connections.
- WebRtcBrowserTest: Two new tests that use non-default certificates,
  generating RSA and ECDSA.

This gives integration test coverage to RSA and ECDSA, which is good
before switching so that ECDSA becomes default (previously only default
was tested).

Also, peerconnection.js's preparePeerConnectionWithCertificate can be
used in the future to ensure cloned certificates are usable for peer
connections. (When persistance and cloning of RTCCertificate is
supported.)

BUG=chromium:581354
==========

to

==========
WebRtcBrowserTest tests with RSA and ECDSA certificates.

- peerconnection.js/WebRtcTestBase: Ability to specify which
  certificate to use (or generate and use) when creating peer connections.
- WebRtcBrowserTest: Two new tests that use non-default certificates,
  generating RSA and ECDSA.

This gives integration test coverage to RSA and ECDSA, which is good
before switching so that ECDSA becomes default (previously only default
was tested).

Also, peerconnection.js's preparePeerConnectionWithCertificate can be
used in the future to ensure cloned certificates are usable for peer
connections. (When persistance and cloning of RTCCertificate is
supported.)

BUG=chromium:581354
==========

[phoglund_chromium, 2016-04-26 11:54:20]

https://codereview.chromium.org/1917133002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/1917133002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_browsertest_base.cc:316:
certificate_keygen_algorithm = "null";
I would prefer if you made "null" the default instead and extracted a constant
for it (e.g. std::string certificate_keygen_algorithm = kUseDefaultCertKeygen in
the function signature). Make the constant public so you can use it from the
test as well. I don't like the empty strings in there. RunAudioVideoCall(...,
kUseDefaultCertKeygen); reads a lot better than RunAudioVideoCall(..., "").

https://codereview.chromium.org/1917133002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_browsertest_base.h (right):

https://codereview.chromium.org/1917133002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_browsertest_base.h:104: std::string
certificate_keygen_algorithm = "") const;
I was sure default arguments were banned, but they appear to have been unbanned.
Alright, sure, let's go with this.

https://codereview.chromium.org/1917133002/diff/1/chrome/test/data/webrtc/pee...
File chrome/test/data/webrtc/peerconnection.js (right):

https://codereview.chromium.org/1917133002/diff/1/chrome/test/data/webrtc/pee...
chrome/test/data/webrtc/peerconnection.js:61: function
preparePeerConnectionWithCertificate(certificate) {
There's no need to make this "public" in the javascript: make it an internal
helper function instead. You're not calling it from the c++ right? In fact, I'd
just inline it to line 46 and remove the gPeerConnection check.

[jansson, 2016-04-27 07:07:30]

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

[jansson, 2016-04-27 07:07:30]

https://codereview.chromium.org/1917133002/diff/1/chrome/test/data/webrtc/pee...
File chrome/test/data/webrtc/peerconnection.js (right):

https://codereview.chromium.org/1917133002/diff/1/chrome/test/data/webrtc/pee...
chrome/test/data/webrtc/peerconnection.js:40: if (keygenAlgorithm == null) {
It's deemed best practice to use the strict equality operator in JS, e.g.
keygenAlgorithm === null. This also applies to inequality operator e.g. !==. The
reason is that it does not do any type conversion before comparing.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_...

[hbos_chromium, 2016-04-27 11:18:37]

Patchset #2 (id:20001) has been deleted

[hbos_chromium, 2016-04-27 11:19:03]

PTAL phoglund

https://codereview.chromium.org/1917133002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/1917133002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_browsertest_base.cc:316:
certificate_keygen_algorithm = "null";
On 2016/04/26 11:54:20, phoglund_chrome wrote:
> I would prefer if you made "null" the default instead and extracted a constant
> for it (e.g. std::string certificate_keygen_algorithm = kUseDefaultCertKeygen
in
> the function signature). Make the constant public so you can use it from the
> test as well. I don't like the empty strings in there. RunAudioVideoCall(...,
> kUseDefaultCertKeygen); reads a lot better than RunAudioVideoCall(..., "").

Done.

https://codereview.chromium.org/1917133002/diff/1/chrome/browser/media/webrtc...
File chrome/browser/media/webrtc_browsertest_base.h (right):

https://codereview.chromium.org/1917133002/diff/1/chrome/browser/media/webrtc...
chrome/browser/media/webrtc_browsertest_base.h:104: std::string
certificate_keygen_algorithm = "") const;
On 2016/04/26 11:54:20, phoglund_chrome wrote:
> I was sure default arguments were banned, but they appear to have been
unbanned.
> Alright, sure, let's go with this.

Acknowledged.

https://codereview.chromium.org/1917133002/diff/1/chrome/test/data/webrtc/pee...
File chrome/test/data/webrtc/peerconnection.js (right):

https://codereview.chromium.org/1917133002/diff/1/chrome/test/data/webrtc/pee...
chrome/test/data/webrtc/peerconnection.js:40: if (keygenAlgorithm == null) {
On 2016/04/27 07:07:30, jansson wrote:
> It's deemed best practice to use the strict equality operator in JS, e.g.
> keygenAlgorithm === null. This also applies to inequality operator e.g. !==.
The
> reason is that it does not do any type conversion before comparing.
> 
>
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_...

Done. Also updated gPeerConnection comparison.

https://codereview.chromium.org/1917133002/diff/1/chrome/test/data/webrtc/pee...
chrome/test/data/webrtc/peerconnection.js:61: function
preparePeerConnectionWithCertificate(certificate) {
On 2016/04/26 11:54:20, phoglund_chrome wrote:
> There's no need to make this "public" in the javascript: make it an internal
> helper function instead. You're not calling it from the c++ right? In fact,
I'd
> just inline it to line 46 and remove the gPeerConnection check.

It's not used but I was planning on probably using it in a different CL, but
I'll just move it back into its own function then.

https://codereview.chromium.org/1917133002/diff/40001/chrome/browser/media/we...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/1917133002/diff/40001/chrome/browser/media/we...
chrome/browser/media/webrtc_browsertest_base.cc:59: const char
WebRtcTestBase::kUseDefaultVideoCodec[] = "";
I could default this to "null" too if you want before landing, but then we can't
distinguish between the value null and the string "null" (ofc that's a pointless
thing to be able to do) - see CreateLocalOffer.

[phoglund_chromium, 2016-04-27 13:23:13]

lgtm with if statement removed

https://codereview.chromium.org/1917133002/diff/1/chrome/test/data/webrtc/pee...
File chrome/test/data/webrtc/peerconnection.js (right):

https://codereview.chromium.org/1917133002/diff/1/chrome/test/data/webrtc/pee...
chrome/test/data/webrtc/peerconnection.js:61: function
preparePeerConnectionWithCertificate(certificate) {
On 2016/04/27 11:19:03, hbos_chromium wrote:
> On 2016/04/26 11:54:20, phoglund_chrome wrote:
> > There's no need to make this "public" in the javascript: make it an internal
> > helper function instead. You're not calling it from the c++ right? In fact,
> I'd
> > just inline it to line 46 and remove the gPeerConnection check.
> 
> It's not used but I was planning on probably using it in a different CL, but
> I'll just move it back into its own function then.

Yes, do it in the later CL in that case.

https://codereview.chromium.org/1917133002/diff/40001/chrome/browser/media/we...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/1917133002/diff/40001/chrome/browser/media/we...
chrome/browser/media/webrtc_browsertest_base.cc:59: const char
WebRtcTestBase::kUseDefaultVideoCodec[] = "";
On 2016/04/27 11:19:03, hbos_chromium wrote:
> I could default this to "null" too if you want before landing, but then we
can't
> distinguish between the value null and the string "null" (ofc that's a
pointless
> thing to be able to do) - see CreateLocalOffer.

Hm, I think the video codec is used to construct perf bucket names, so don't do
that.

https://codereview.chromium.org/1917133002/diff/40001/chrome/browser/media/we...
chrome/browser/media/webrtc_browsertest_base.cc:317: if
(certificate_keygen_algorithm.empty())
Remove this if statement now that null is the default.

https://codereview.chromium.org/1917133002/diff/40001/chrome/browser/media/we...
File chrome/browser/media/webrtc_browsertest_base.h (right):

https://codereview.chromium.org/1917133002/diff/40001/chrome/browser/media/we...
chrome/browser/media/webrtc_browsertest_base.h:125: const std::string&
video_codec = kUseDefaultVideoCodec) const;
Nice!

[hbos_chromium, 2016-04-27 15:09:08]

https://codereview.chromium.org/1917133002/diff/40001/chrome/browser/media/we...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/1917133002/diff/40001/chrome/browser/media/we...
chrome/browser/media/webrtc_browsertest_base.cc:317: if
(certificate_keygen_algorithm.empty())
On 2016/04/27 13:23:12, phoglund_chrome wrote:
> Remove this if statement now that null is the default.

Done.

[hbos_chromium, 2016-04-27 15:09:29]

The CQ bit was checked by hbos@chromium.org

[hbos_chromium, 2016-04-27 15:09:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
phoglund@chromium.org
Link to the patchset: https://codereview.chromium.org/1917133002/#ps60001
(title: "Removed if-statement")

[commit-bot: I haz the power, 2016-04-27 15:09:58]

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

[commit-bot: I haz the power, 2016-04-27 16:24:24]

Description was changed from

==========
WebRtcBrowserTest tests with RSA and ECDSA certificates.

- peerconnection.js/WebRtcTestBase: Ability to specify which
  certificate to use (or generate and use) when creating peer connections.
- WebRtcBrowserTest: Two new tests that use non-default certificates,
  generating RSA and ECDSA.

This gives integration test coverage to RSA and ECDSA, which is good
before switching so that ECDSA becomes default (previously only default
was tested).

Also, peerconnection.js's preparePeerConnectionWithCertificate can be
used in the future to ensure cloned certificates are usable for peer
connections. (When persistance and cloning of RTCCertificate is
supported.)

BUG=chromium:581354
==========

to

==========
WebRtcBrowserTest tests with RSA and ECDSA certificates.

- peerconnection.js/WebRtcTestBase: Ability to specify which
  certificate to use (or generate and use) when creating peer connections.
- WebRtcBrowserTest: Two new tests that use non-default certificates,
  generating RSA and ECDSA.

This gives integration test coverage to RSA and ECDSA, which is good
before switching so that ECDSA becomes default (previously only default
was tested).

Also, peerconnection.js's preparePeerConnectionWithCertificate can be
used in the future to ensure cloned certificates are usable for peer
connections. (When persistance and cloning of RTCCertificate is
supported.)

BUG=chromium:581354
==========

[commit-bot: I haz the power, 2016-04-27 16:24:25]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-04-30 17:10:25]

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

[commit-bot: I haz the power, 2016-04-30 17:50:19]

Description was changed from

==========
WebRtcBrowserTest tests with RSA and ECDSA certificates.

- peerconnection.js/WebRtcTestBase: Ability to specify which
  certificate to use (or generate and use) when creating peer connections.
- WebRtcBrowserTest: Two new tests that use non-default certificates,
  generating RSA and ECDSA.

This gives integration test coverage to RSA and ECDSA, which is good
before switching so that ECDSA becomes default (previously only default
was tested).

Also, peerconnection.js's preparePeerConnectionWithCertificate can be
used in the future to ensure cloned certificates are usable for peer
connections. (When persistance and cloning of RTCCertificate is
supported.)

BUG=chromium:581354
==========

to

==========
WebRtcBrowserTest tests with RSA and ECDSA certificates.

- peerconnection.js/WebRtcTestBase: Ability to specify which
  certificate to use (or generate and use) when creating peer connections.
- WebRtcBrowserTest: Two new tests that use non-default certificates,
  generating RSA and ECDSA.

This gives integration test coverage to RSA and ECDSA, which is good
before switching so that ECDSA becomes default (previously only default
was tested).

Also, peerconnection.js's preparePeerConnectionWithCertificate can be
used in the future to ensure cloned certificates are usable for peer
connections. (When persistance and cloning of RTCCertificate is
supported.)

BUG=chromium:581354

Committed: https://crrev.com/e007ec047dfbbf54e54a211d53b1ce9ed6d5815c
Cr-Commit-Position: refs/heads/master@{#390096}
==========