Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(574)

Issue 2013523002: Replacing DtlsIdentityStoreInterface with RTCCertificateGeneratorInterface. (Closed)

Created:
4 years, 7 months ago by hbos
Modified:
4 years, 6 months ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Replacing DtlsIdentityStoreInterface with RTCCertificateGeneratorInterface. The store was used in WebRtcSessionDescriptionFactory to generate certificates, now a generator is used instead (new API). PeerConnection[Factory][Interface], and WebRtcSession are updated to pass generators all the way down to the WebRtcSessionDescriptionFactory instead of stores. The webrtc implementation of a generator, RTCCertificateGenerator, is used as the default generator (peerconnectionfactory.cc:189) instead of the webrtc implementation of a store, DtlsIdentityStoreImpl. The generator is fully parameterized and does not generate RSA-1024 unless you ask for it (which makes sense not to do beforehand since ECDSA is now default). The store was not fully parameterized (known filed bug). The "top" layer, PeerConnectionFactoryInterface::CreatePeerConnection, is updated to take a generator instead of a store. But as to not break Chromium, the old function signature taking a store is kept. It is implemented to invoke the generator version by wrapping the store in an RTCCertificateGeneratorStoreWrapper. As soon as Chromium is updated to use the new function signature we can remove the old CreatePeerConnection. Due to having multiple CreatePeerConnection signatures, some calling places are updated to resolve the ambiguity introduced. BUG=webrtc:5707, webrtc:5708 R=phoglund@webrtc.org, tommi@webrtc.org TBR=tkchin@webrc.org Committed: https://chromium.googlesource.com/external/webrtc/+/400781a2091d09a725b32c6953247036b22478e8

Patch Set 1 : The meat: Changes to PeerConnection/Factory, WebRtcSession and WebRtcSessionDescriptionFactory chain #

Patch Set 2 : The side dish: Update ambigous calls #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+207 lines, -182 lines) Patch
M talk/app/webrtc/objc/RTCPeerConnection.mm View 1 2 chunks +11 lines, -2 lines 0 comments Download
M webrtc/api/java/jni/peerconnection_jni.cc View 1 1 chunk +5 lines, -1 line 0 comments Download
M webrtc/api/peerconnection.h View 2 chunks +1 line, -2 lines 0 comments Download
M webrtc/api/peerconnection.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M webrtc/api/peerconnectionfactory.h View 4 chunks +6 lines, -6 lines 0 comments Download
M webrtc/api/peerconnectionfactory.cc View 6 chunks +26 lines, -24 lines 0 comments Download
M webrtc/api/peerconnectionfactoryproxy.h View 4 chunks +6 lines, -6 lines 0 comments Download
M webrtc/api/peerconnectioninterface.h View 2 chunks +37 lines, -2 lines 0 comments Download
M webrtc/api/peerconnectioninterface_unittest.cc View 1 1 chunk +6 lines, -2 lines 0 comments Download
M webrtc/api/webrtcsession.h View 1 chunk +1 line, -1 line 0 comments Download
M webrtc/api/webrtcsession.cc View 3 chunks +7 lines, -8 lines 0 comments Download
M webrtc/api/webrtcsession_unittest.cc View 1 1 chunk +4 lines, -1 line 0 comments Download
M webrtc/api/webrtcsessiondescriptionfactory.h View 6 chunks +28 lines, -37 lines 2 comments Download
M webrtc/api/webrtcsessiondescriptionfactory.cc View 6 chunks +56 lines, -82 lines 0 comments Download
M webrtc/examples/peerconnection/client/conductor.cc View 1 1 chunk +5 lines, -1 line 0 comments Download
M webrtc/sdk/objc/Framework/Classes/RTCPeerConnection.mm View 1 1 chunk +6 lines, -5 lines 0 comments Download

Messages

Total messages: 19 (7 generated)
hbos
Please take a look tommi and tkchin. tkchin mainly for touching .mm files.
4 years, 7 months ago (2016-05-24 19:32:42 UTC) #2
tommi
lgtm https://codereview.webrtc.org/2013523002/diff/20001/webrtc/api/webrtcsessiondescriptionfactory.h File webrtc/api/webrtcsessiondescriptionfactory.h (right): https://codereview.webrtc.org/2013523002/diff/20001/webrtc/api/webrtcsessiondescriptionfactory.h#newcode46 webrtc/api/webrtcsessiondescriptionfactory.h:46: sigslot::signal0<> SignalRequestFailed; as a note on future changes, ...
4 years, 7 months ago (2016-05-24 21:06:59 UTC) #3
hbos
Please take a look tkchin, or at least stamp the .mm files. https://codereview.webrtc.org/2013523002/diff/20001/webrtc/api/webrtcsessiondescriptionfactory.h File webrtc/api/webrtcsessiondescriptionfactory.h ...
4 years, 7 months ago (2016-05-25 07:52:31 UTC) #4
tommi
On 2016/05/25 07:52:31, hbos wrote: > Please take a look tkchin, or at least stamp ...
4 years, 7 months ago (2016-05-25 08:27:47 UTC) #5
hbos
PTAL phoglund for .mm files
4 years, 6 months ago (2016-05-27 11:33:41 UTC) #7
phoglund
lgtm for .mm
4 years, 6 months ago (2016-05-27 11:38:07 UTC) #8
hbos
OK, sorry for another TBR-land, tkchin, but you're OOO now and we're both OOO for ...
4 years, 6 months ago (2016-05-27 12:06:43 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2013523002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2013523002/20001
4 years, 6 months ago (2016-05-27 12:08:15 UTC) #12
commit-bot: I haz the power
Try jobs failed on following builders: linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds/2893)
4 years, 6 months ago (2016-05-27 12:18:47 UTC) #14
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/400781a2091d09a725b32c6953247036b22478e8 Cr-Commit-Position: refs/heads/master@{#12947}
4 years, 6 months ago (2016-05-27 12:52:10 UTC) #16
hbos
Committed patchset #2 (id:20001) manually as 400781a2091d09a725b32c6953247036b22478e8 (presubmit successful).
4 years, 6 months ago (2016-05-27 12:52:13 UTC) #18
hbos
4 years, 6 months ago (2016-05-27 13:08:35 UTC) #19
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.webrtc.org/2020633002/ by hbos@webrtc.org.

The reason for reverting is: There are more CreatePeerConnection calls than I
anticipated/had found in Chromium, like remoting/protocol/webrtc_transport.cc.
Reverting due to broken Chromium FYI bots..

Powered by Google App Engine
This is Rietveld 408576698