Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(29)

Issue 1337673002: Change WebRTC SslCipher to be exposed as number only. (Closed)

Created:
3 years, 9 months ago by guoweis_webrtc
Modified:
3 years, 8 months ago
Base URL:
https://chromium.googlesource.com/external/webrtc@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Change WebRTC SslCipher to be exposed as number only. This makes the SSL exposed as uint16_t which is the IANA value. GetRfcSslCipherName is introduced to handle the conversion to names from ID. IANA value will be used for UMA reporting. Names will still be used for WebRTC stats reporting. For SRTP, currently it's still string internally but is reported as IANA number. This is used by the ongoing CL https://codereview.chromium.org/1335023002. BUG=523033 Committed: https://crrev.com/4fe3c9a77386598db9abd1f0d6983aefee9cc943 Cr-Commit-Position: refs/heads/master@{#10124}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 4

Patch Set 6 : #

Total comments: 3

Patch Set 7 : #

Total comments: 16

Patch Set 8 : #

Total comments: 20

Patch Set 9 : #

Patch Set 10 : #

Total comments: 8

Patch Set 11 : #

Total comments: 38

Patch Set 12 : #

Patch Set 13 : #

Patch Set 14 : #

Total comments: 10

Patch Set 15 : #

Patch Set 16 : #

Patch Set 17 : #

Patch Set 18 : #

Patch Set 19 : #

Patch Set 20 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+324 lines, -279 lines) Patch
M talk/app/webrtc/fakemetricsobserver.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +6 lines, -10 lines 0 comments Download
M talk/app/webrtc/fakemetricsobserver.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +11 lines, -26 lines 0 comments Download
M talk/app/webrtc/peerconnection_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +64 lines, -60 lines 0 comments Download
M talk/app/webrtc/peerconnectioninterface.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +8 lines, -3 lines 0 comments Download
M talk/app/webrtc/statscollector.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +6 lines, -4 lines 0 comments Download
M talk/app/webrtc/statscollector_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +9 lines, -2 lines 0 comments Download
M talk/app/webrtc/umametrics.h View 1 2 3 4 5 6 7 8 2 chunks +7 lines, -6 lines 0 comments Download
M talk/app/webrtc/webrtcsession.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +14 lines, -13 lines 0 comments Download
M talk/session/media/channel.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +5 lines, -4 lines 0 comments Download
M talk/session/media/channel.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 5 chunks +9 lines, -6 lines 0 comments Download
M talk/session/media/channel_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +7 lines, -7 lines 0 comments Download
M talk/session/media/mediasession.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -1 line 0 comments Download
M talk/session/media/mediasession.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +9 lines, -9 lines 0 comments Download
M talk/session/media/mediasession_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -2 lines 0 comments Download
M talk/session/media/srtpfilter.h View 1 2 3 4 5 6 7 8 2 chunks +1 line, -7 lines 0 comments Download
M talk/session/media/srtpfilter.cc View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -4 lines 0 comments Download
M talk/session/media/srtpfilter_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -2 lines 0 comments Download
M webrtc/base/opensslstreamadapter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +7 lines, -7 lines 0 comments Download
M webrtc/base/opensslstreamadapter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 7 chunks +49 lines, -42 lines 0 comments Download
M webrtc/base/sslstreamadapter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +28 lines, -6 lines 0 comments Download
M webrtc/base/sslstreamadapter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 4 chunks +26 lines, -7 lines 0 comments Download
M webrtc/base/sslstreamadapter_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +27 lines, -27 lines 0 comments Download
M webrtc/p2p/base/dtlstransportchannel.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -2 lines 0 comments Download
M webrtc/p2p/base/dtlstransportchannel.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +3 lines, -3 lines 0 comments Download
M webrtc/p2p/base/dtlstransportchannel_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 3 chunks +9 lines, -9 lines 0 comments Download
M webrtc/p2p/base/faketransportcontroller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +2 lines, -2 lines 0 comments Download
M webrtc/p2p/base/p2ptransportchannel.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +2 lines, -2 lines 0 comments Download
M webrtc/p2p/base/transport.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +2 lines, -2 lines 0 comments Download
M webrtc/p2p/base/transport.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +2 lines, -2 lines 0 comments Download
M webrtc/p2p/base/transportchannel.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 74 (36 generated)
guoweis_webrtc
PTAL.
3 years, 9 months ago (2015-09-10 23:57:56 UTC) #2
guoweis_webrtc
On 2015/09/10 23:57:56, guoweis wrote: > PTAL. Seems that we at least use TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA as ...
3 years, 9 months ago (2015-09-11 17:16:47 UTC) #3
guoweis_webrtc
On 2015/09/11 17:16:47, guoweis wrote: > On 2015/09/10 23:57:56, guoweis wrote: > > PTAL. > ...
3 years, 9 months ago (2015-09-11 17:58:06 UTC) #4
guoweis_webrtc
On 2015/09/11 17:58:06, guoweis wrote: > On 2015/09/11 17:16:47, guoweis wrote: > > On 2015/09/10 ...
3 years, 9 months ago (2015-09-11 18:01:02 UTC) #5
joachim
On 2015/09/11 17:58:06, guoweis wrote: > On 2015/09/11 17:16:47, guoweis wrote: > > On 2015/09/10 ...
3 years, 9 months ago (2015-09-14 09:59:02 UTC) #6
pthatcher1
I agree with jbauch. We should at least have the ones in there. However, with ...
3 years, 9 months ago (2015-09-16 03:52:30 UTC) #7
pthatcher1
I agree with jbauch. We should at least have the ones in there. However, with ...
3 years, 9 months ago (2015-09-16 03:52:31 UTC) #8
joachim
https://codereview.webrtc.org/1337673002/diff/80001/talk/app/webrtc/peerconnection_unittest.cc File talk/app/webrtc/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1337673002/diff/80001/talk/app/webrtc/peerconnection_unittest.cc#newcode1354 talk/app/webrtc/peerconnection_unittest.cc:1354: 1); Shouldn't this be EXPECT_EQ(1, ...) here and below?
3 years, 9 months ago (2015-09-18 22:24:25 UTC) #9
guoweis_webrtc
PTAL. https://codereview.webrtc.org/1337673002/diff/80001/talk/app/webrtc/peerconnection_unittest.cc File talk/app/webrtc/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1337673002/diff/80001/talk/app/webrtc/peerconnection_unittest.cc#newcode1354 talk/app/webrtc/peerconnection_unittest.cc:1354: 1); On 2015/09/18 22:24:25, joachim wrote: > Shouldn't ...
3 years, 9 months ago (2015-09-22 19:59:39 UTC) #13
Ryan Sleevi
https://codereview.webrtc.org/1337673002/diff/160001/talk/app/webrtc/webrtcsession.cc File talk/app/webrtc/webrtcsession.cc (right): https://codereview.webrtc.org/1337673002/diff/160001/talk/app/webrtc/webrtcsession.cc#newcode95 talk/app/webrtc/webrtcsession.cc:95: return SrtpCipherType_AES_CM_128_HMAC_SHA1_80; For what it's worth, this registration is ...
3 years, 9 months ago (2015-09-22 21:42:15 UTC) #15
guoweis_webrtc
One more question. At https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc/base/opensslstreamadapter.cc&l=144, there are many "default" ciphers for TLS1.0, 1.2, etc. Can ...
3 years, 9 months ago (2015-09-23 06:46:16 UTC) #16
guoweis_webrtc
On 2015/09/23 06:46:16, guoweis wrote: > One more question. > > At > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc/base/opensslstreamadapter.cc&l=144, > ...
3 years, 9 months ago (2015-09-23 06:47:57 UTC) #17
juberti
https://codereview.webrtc.org/1337673002/diff/220001/talk/app/webrtc/fakemetricsobserver.h File talk/app/webrtc/fakemetricsobserver.h (left): https://codereview.webrtc.org/1337673002/diff/220001/talk/app/webrtc/fakemetricsobserver.h#oldcode63 talk/app/webrtc/fakemetricsobserver.h:63: // This is a 2 dimension array. The first ...
3 years, 8 months ago (2015-09-24 13:41:15 UTC) #21
charlieloveeric3
<groupId>org.slf4j</groupId> <artifactId>slf4j-api</artifactId> </dependency> <dependency> <groupId>ch.qos.logback</groupId> <artifactId>logback-core</artifactId> </dependency> <dependency> <groupId>ch.qos.logback</groupId> <artifactId>logback-classic</artifactId> </dependency> <dependency> <groupId>org.slf4j</groupId> <artifactId>jcl-over-slf4j</artifactId> </dependency>
3 years, 8 months ago (2015-09-24 13:44:27 UTC) #23
guoweis_webrtc
PTAL. https://codereview.webrtc.org/1337673002/diff/220001/talk/app/webrtc/fakemetricsobserver.h File talk/app/webrtc/fakemetricsobserver.h (left): https://codereview.webrtc.org/1337673002/diff/220001/talk/app/webrtc/fakemetricsobserver.h#oldcode63 talk/app/webrtc/fakemetricsobserver.h:63: // This is a 2 dimension array. The ...
3 years, 8 months ago (2015-09-24 18:27:13 UTC) #28
Ryan Sleevi
I don't believe this does what you want, and it also suffers from necessitating static ...
3 years, 8 months ago (2015-09-24 21:09:58 UTC) #30
juberti
https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/opensslstreamadapter.cc File webrtc/base/opensslstreamadapter.cc (right): https://codereview.webrtc.org/1337673002/diff/220001/webrtc/base/opensslstreamadapter.cc#newcode144 webrtc/base/opensslstreamadapter.cc:144: 0xC014, On 2015/09/24 18:27:13, guoweis wrote: > On 2015/09/24 ...
3 years, 8 months ago (2015-09-24 21:37:32 UTC) #32
guoweis_webrtc
PTAL. https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/fakemetricsobserver.cc File talk/app/webrtc/fakemetricsobserver.cc (right): https://codereview.webrtc.org/1337673002/diff/320001/talk/app/webrtc/fakemetricsobserver.cc#newcode67 talk/app/webrtc/fakemetricsobserver.cc:67: if (it != counters_[type].end()) { On 2015/09/24 21:37:32, ...
3 years, 8 months ago (2015-09-25 18:30:32 UTC) #39
davidben_webrtc
lgtm with comments, though presumably you want to wait for a WebRTC review too. https://codereview.webrtc.org/1337673002/diff/450001/talk/app/webrtc/fakemetricsobserver.cc ...
3 years, 8 months ago (2015-09-25 19:23:13 UTC) #41
guoweis_webrtc
Thanks. Issues addressed. juberti/pthatcher, PTAL.
3 years, 8 months ago (2015-09-25 21:20:26 UTC) #42
guoweis_webrtc
On 2015/09/25 21:20:26, guoweis wrote: > Thanks. Issues addressed. > > juberti/pthatcher, PTAL. juberti/pthatcher, Ping.
3 years, 8 months ago (2015-09-28 17:13:42 UTC) #43
Ryan Sleevi
At this point, there's a lot going on in this CL, so I don't want ...
3 years, 8 months ago (2015-09-28 17:24:17 UTC) #44
pthatcher1
https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/fakemetricsobserver.h File talk/app/webrtc/fakemetricsobserver.h (right): https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/fakemetricsobserver.h#newcode61 talk/app/webrtc/fakemetricsobserver.h:61: // comes to sparse enum type, like SSL cipher ...
3 years, 8 months ago (2015-09-29 22:25:17 UTC) #45
guoweis_webrtc
https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/fakemetricsobserver.h File talk/app/webrtc/fakemetricsobserver.h (right): https://codereview.webrtc.org/1337673002/diff/470001/talk/app/webrtc/fakemetricsobserver.h#newcode61 talk/app/webrtc/fakemetricsobserver.h:61: // comes to sparse enum type, like SSL cipher ...
3 years, 8 months ago (2015-09-30 04:09:47 UTC) #47
pthatcher1
https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstreamadapter.cc File webrtc/base/opensslstreamadapter.cc (right): https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstreamadapter.cc#newcode147 webrtc/base/opensslstreamadapter.cc:147: 0xC00A; // TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA On 2015/09/30 04:09:46, guoweis wrote: > ...
3 years, 8 months ago (2015-09-30 05:46:22 UTC) #48
guoweis_webrtc
PTAL. https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstreamadapter.cc File webrtc/base/opensslstreamadapter.cc (right): https://codereview.webrtc.org/1337673002/diff/470001/webrtc/base/opensslstreamadapter.cc#newcode147 webrtc/base/opensslstreamadapter.cc:147: 0xC00A; // TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA On 2015/09/30 05:46:22, pthatcher1 wrote: ...
3 years, 8 months ago (2015-09-30 17:41:55 UTC) #50
pthatcher1
https://codereview.webrtc.org/1337673002/diff/570001/talk/app/webrtc/peerconnection_unittest.cc File talk/app/webrtc/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1337673002/diff/570001/talk/app/webrtc/peerconnection_unittest.cc#newcode1345 talk/app/webrtc/peerconnection_unittest.cc:1345: EXPECT_EQ_WAIT(rtc::SSLStreamAdapter::GetSslCipherRfcNameById( I think a good pair of names for ...
3 years, 8 months ago (2015-09-30 18:39:35 UTC) #51
guoweis_webrtc
PTAL. https://codereview.webrtc.org/1337673002/diff/570001/talk/app/webrtc/peerconnection_unittest.cc File talk/app/webrtc/peerconnection_unittest.cc (right): https://codereview.webrtc.org/1337673002/diff/570001/talk/app/webrtc/peerconnection_unittest.cc#newcode1345 talk/app/webrtc/peerconnection_unittest.cc:1345: EXPECT_EQ_WAIT(rtc::SSLStreamAdapter::GetSslCipherRfcNameById( On 2015/09/30 18:39:35, pthatcher1 wrote: > I ...
3 years, 8 months ago (2015-09-30 20:14:21 UTC) #55
pthatcher1
lgtm
3 years, 8 months ago (2015-09-30 20:35:50 UTC) #56
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1337673002/650001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1337673002/650001
3 years, 8 months ago (2015-09-30 20:37:52 UTC) #59
commit-bot: I haz the power
Try jobs failed on following builders: android_clang_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_clang_dbg/builds/7935) android_gn_dbg on tryserver.webrtc (JOB_FAILED, ...
3 years, 8 months ago (2015-09-30 20:39:00 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1337673002/670001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1337673002/670001
3 years, 8 months ago (2015-09-30 20:46:42 UTC) #64
commit-bot: I haz the power
Try jobs failed on following builders: win_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/9722)
3 years, 8 months ago (2015-09-30 20:49:29 UTC) #66
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1337673002/750001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1337673002/750001
3 years, 8 months ago (2015-10-01 01:47:35 UTC) #70
commit-bot: I haz the power
Committed patchset #20 (id:750001)
3 years, 8 months ago (2015-10-01 01:49:14 UTC) #71
commit-bot: I haz the power
Patchset 20 (id:??) landed as https://crrev.com/4fe3c9a77386598db9abd1f0d6983aefee9cc943 Cr-Commit-Position: refs/heads/master@{#10124}
3 years, 8 months ago (2015-10-01 01:49:29 UTC) #72
guoweis_webrtc
A revert of this CL (patchset #20 id:750001) has been created in https://codereview.webrtc.org/1380603005/ by guoweis@webrtc.org. ...
3 years, 8 months ago (2015-10-01 02:22:49 UTC) #73
juberti
3 years, 8 months ago (2015-10-01 04:57:51 UTC) #74
Message was sent while issue was closed.
let's talk about this CL tomorrow. 

An API like this:
bool SetSrtpCiphers(const std::vector<std::string>& ciphers) override;
bool GetSrtpCryptoSuite(std::string* cipher) override;
bool GetSslCipherSuite(uint16_t* cipher) override;

feels entirely inconsistent.

Powered by Google App Engine
This is Rietveld 408576698