Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2001103002:
RTCCertificateGeneratorInterface and RTCCertificateGeneratorStoreWrapper added. (Closed)
DescriptionRTCCertificateGeneratorInterface and RTCCertificateGeneratorStoreWrapper added.
This CL adds these classes but does not change any functonality or interface
yet. This is in preparation for future CLs. To be used for this:
https://codereview.webrtc.org/2000163002/
RTCCertificateGenerator is meant to replace DtlsIdentityStoreInterface and
implementations. In order to continue to support mocking and to help with the
transition, RTCCertificateGenerator gets an interface that it implements (just
like the store has both interface and impl).
PeerConnectionFactoryInterface::CreatePeerConnection will take an
RTCCertificateGeneratorInterface instead of DtlsIdentityStoreInterface. As to
not break Chromium, both versions of CreatePeerConnection need to exist for a
transition period. This will be done by wrapping a store into a generator
wrapper - RTCCertificateGeneratorStoreWrapper.
BUG=webrtc:5707, webrtc:5708
R=hta@webrtc.org, tommi@chromium.org, tommi@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/6c96314b42e872abbba3884904a4fdc3dfa8955d
Patch Set 1 #
Total comments: 4
Patch Set 2 : Removed self-reference (this_), added LOG #
Total comments: 6
Patch Set 3 : Addressed nits #
Messages
Total messages: 24 (12 generated)
Description was changed from ========== RTCCertificateGeneratorInterface and RTCCertificateGeneratorStoreWrapper added. This CL adds these classes but does not change any functonality or interface yet. This is in preparation for future CLs. RTCCertificateGenerator is meant to replace DtlsIdentityStoreInterface and implementations. In order to continue to support mocking and to help with the transition, RTCCertificateGenerator gets an interface that it implements (just like the store has both interface and impl). PeerConnectionFactoryInterface::CreatePeerConnection will take an RTCCertificateGeneratorInterface instead of DtlsIdentityStoreInterface. As to not break Chromium, both versions of CreatePeerConnection need to exist for a transition period. This will be done by wrapping a store into a generator wrapper - RTCCertificateGeneratorStoreWrapper. BUG=webrtc:5707, webrtc:5708 ========== to ========== RTCCertificateGeneratorInterface and RTCCertificateGeneratorStoreWrapper added. This CL adds these classes but does not change any functonality or interface yet. This is in preparation for future CLs. To be used for this: https://codereview.webrtc.org/2000163002/ RTCCertificateGenerator is meant to replace DtlsIdentityStoreInterface and implementations. In order to continue to support mocking and to help with the transition, RTCCertificateGenerator gets an interface that it implements (just like the store has both interface and impl). PeerConnectionFactoryInterface::CreatePeerConnection will take an RTCCertificateGeneratorInterface instead of DtlsIdentityStoreInterface. As to not break Chromium, both versions of CreatePeerConnection need to exist for a transition period. This will be done by wrapping a store into a generator wrapper - RTCCertificateGeneratorStoreWrapper. BUG=webrtc:5707, webrtc:5708 ==========
hbos@webrtc.org changed reviewers: + hta@webrtc.org, tommi@webrtc.org
Please take a look hta and tommi.
The concept of a self-owning entity in a world where everyone's moving from refcounting to garbage collection doesn't make me incredibly happy. But will LGTM it after Tommi's had his say - I don't have a better solution. https://codereview.webrtc.org/2001103002/diff/1/webrtc/api/dtlsidentitystore.cc File webrtc/api/dtlsidentitystore.cc (right): https://codereview.webrtc.org/2001103002/diff/1/webrtc/api/dtlsidentitystore.... webrtc/api/dtlsidentitystore.cc:52: Callback(nullptr); The error is lost to the world here. Should you at least DLOG it? https://codereview.webrtc.org/2001103002/diff/1/webrtc/api/dtlsidentitystore.... webrtc/api/dtlsidentitystore.cc:87: rtc::scoped_refptr<DtlsIdentityRequestObserver> this_; can you call it something other than "this_"? "prevent_garbage_collection_" would be my suggestion, "self_reference_ might be another, but Tommi probably has a stronger opinion.
Over to tommi then, please take a look.
Patchset #2 (id:20001) has been deleted
Removed self-reference. Please take a look tommi. https://codereview.webrtc.org/2001103002/diff/1/webrtc/api/dtlsidentitystore.cc File webrtc/api/dtlsidentitystore.cc (right): https://codereview.webrtc.org/2001103002/diff/1/webrtc/api/dtlsidentitystore.... webrtc/api/dtlsidentitystore.cc:52: Callback(nullptr); On 2016/05/23 13:50:38, hta-webrtc wrote: > The error is lost to the world here. Should you at least DLOG it? Done, LOG(LS_WARNING). https://codereview.webrtc.org/2001103002/diff/1/webrtc/api/dtlsidentitystore.... webrtc/api/dtlsidentitystore.cc:87: rtc::scoped_refptr<DtlsIdentityRequestObserver> this_; On 2016/05/23 13:50:38, hta-webrtc wrote: > can you call it something other than "this_"? > "prevent_garbage_collection_" would be my suggestion, "self_reference_ might be > another, but Tommi probably has a stronger opinion. > On second thought this self-referencing thing is completely unnecessary. It is already referenced by the thread message handling. "this_" was a bug; if the posting was aborted "this_ = nullptr;" would never happen and the circular self-reference lead to a memory leak. Removed this_, problem solved.
Since I got the OWNER lgtm I need and this is just a preparation CL for a follow-up that tommi will have to review anyway and this doesn't touch any active code I'll go ahead land this so I can start the review on the follow-up. I can address any comments on this change in there, hope that's OK.
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org Link to the patchset: https://codereview.webrtc.org/2001103002/#ps40001 (title: "Removed self-reference (this_), added LOG")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001103002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001103002/40001
On 2016/05/24 17:53:35, hbos wrote: > Since I got the OWNER lgtm I need and this is just a preparation CL for a > follow-up that tommi will have to review anyway and this doesn't touch any > active code I'll go ahead land this so I can start the review on the follow-up. > I can address any comments on this change in there, hope that's OK. Oh, no I don't. I'll wait for tommi. :)
The CQ bit was unchecked by hbos@webrtc.org
tommi@chromium.org changed reviewers: + tommi@chromium.org
lgtm with nits addressed https://codereview.webrtc.org/2001103002/diff/40001/webrtc/api/dtlsidentityst... File webrtc/api/dtlsidentitystore.cc (right): https://codereview.webrtc.org/2001103002/diff/40001/webrtc/api/dtlsidentityst... webrtc/api/dtlsidentitystore.cc:45: void OnFailure(int error) override { nit: make implementation of OnFailure and OnSuccess, private? https://codereview.webrtc.org/2001103002/diff/40001/webrtc/api/dtlsidentityst... File webrtc/api/dtlsidentitystore.h (right): https://codereview.webrtc.org/2001103002/diff/40001/webrtc/api/dtlsidentityst... webrtc/api/dtlsidentitystore.h:136: // generator API, |DtlsIdentityStoreInterface|. This will used while This will be used https://codereview.webrtc.org/2001103002/diff/40001/webrtc/api/dtlsidentityst... webrtc/api/dtlsidentitystore.h:138: // bugs.webrtc.org/5708. Then it will be removed. nit: Once those bugs have been fixed, this will be removed.
https://codereview.webrtc.org/2001103002/diff/40001/webrtc/api/dtlsidentityst... File webrtc/api/dtlsidentitystore.cc (right): https://codereview.webrtc.org/2001103002/diff/40001/webrtc/api/dtlsidentityst... webrtc/api/dtlsidentitystore.cc:45: void OnFailure(int error) override { On 2016/05/24 18:03:40, tommi-chrömium wrote: > nit: make implementation of OnFailure and OnSuccess, private? Done. Callback can also be private. https://codereview.webrtc.org/2001103002/diff/40001/webrtc/api/dtlsidentityst... File webrtc/api/dtlsidentitystore.h (right): https://codereview.webrtc.org/2001103002/diff/40001/webrtc/api/dtlsidentityst... webrtc/api/dtlsidentitystore.h:136: // generator API, |DtlsIdentityStoreInterface|. This will used while On 2016/05/24 18:03:40, tommi-chrömium wrote: > This will be used haow ai gramma https://codereview.webrtc.org/2001103002/diff/40001/webrtc/api/dtlsidentityst... webrtc/api/dtlsidentitystore.h:138: // bugs.webrtc.org/5708. Then it will be removed. On 2016/05/24 18:03:40, tommi-chrömium wrote: > nit: Once those bugs have been fixed, this will be removed. Done.
lgtm
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org, tommi@chromium.org Link to the patchset: https://codereview.webrtc.org/2001103002/#ps60001 (title: "Addressed nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2001103002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2001103002/60001
The CQ bit was unchecked by hbos@webrtc.org
Description was changed from ========== RTCCertificateGeneratorInterface and RTCCertificateGeneratorStoreWrapper added. This CL adds these classes but does not change any functonality or interface yet. This is in preparation for future CLs. To be used for this: https://codereview.webrtc.org/2000163002/ RTCCertificateGenerator is meant to replace DtlsIdentityStoreInterface and implementations. In order to continue to support mocking and to help with the transition, RTCCertificateGenerator gets an interface that it implements (just like the store has both interface and impl). PeerConnectionFactoryInterface::CreatePeerConnection will take an RTCCertificateGeneratorInterface instead of DtlsIdentityStoreInterface. As to not break Chromium, both versions of CreatePeerConnection need to exist for a transition period. This will be done by wrapping a store into a generator wrapper - RTCCertificateGeneratorStoreWrapper. BUG=webrtc:5707, webrtc:5708 ========== to ========== RTCCertificateGeneratorInterface and RTCCertificateGeneratorStoreWrapper added. This CL adds these classes but does not change any functonality or interface yet. This is in preparation for future CLs. To be used for this: https://codereview.webrtc.org/2000163002/ RTCCertificateGenerator is meant to replace DtlsIdentityStoreInterface and implementations. In order to continue to support mocking and to help with the transition, RTCCertificateGenerator gets an interface that it implements (just like the store has both interface and impl). PeerConnectionFactoryInterface::CreatePeerConnection will take an RTCCertificateGeneratorInterface instead of DtlsIdentityStoreInterface. As to not break Chromium, both versions of CreatePeerConnection need to exist for a transition period. This will be done by wrapping a store into a generator wrapper - RTCCertificateGeneratorStoreWrapper. BUG=webrtc:5707, webrtc:5708 R=hta@webrtc.org, tommi@chromium.org, tommi@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/6c96314b42e872abbba388490... ==========
Message was sent while issue was closed.
Description was changed from ========== RTCCertificateGeneratorInterface and RTCCertificateGeneratorStoreWrapper added. This CL adds these classes but does not change any functonality or interface yet. This is in preparation for future CLs. To be used for this: https://codereview.webrtc.org/2000163002/ RTCCertificateGenerator is meant to replace DtlsIdentityStoreInterface and implementations. In order to continue to support mocking and to help with the transition, RTCCertificateGenerator gets an interface that it implements (just like the store has both interface and impl). PeerConnectionFactoryInterface::CreatePeerConnection will take an RTCCertificateGeneratorInterface instead of DtlsIdentityStoreInterface. As to not break Chromium, both versions of CreatePeerConnection need to exist for a transition period. This will be done by wrapping a store into a generator wrapper - RTCCertificateGeneratorStoreWrapper. BUG=webrtc:5707, webrtc:5708 R=hta@webrtc.org, tommi@chromium.org, tommi@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/6c96314b42e872abbba388490... ========== to ========== RTCCertificateGeneratorInterface and RTCCertificateGeneratorStoreWrapper added. This CL adds these classes but does not change any functonality or interface yet. This is in preparation for future CLs. To be used for this: https://codereview.webrtc.org/2000163002/ RTCCertificateGenerator is meant to replace DtlsIdentityStoreInterface and implementations. In order to continue to support mocking and to help with the transition, RTCCertificateGenerator gets an interface that it implements (just like the store has both interface and impl). PeerConnectionFactoryInterface::CreatePeerConnection will take an RTCCertificateGeneratorInterface instead of DtlsIdentityStoreInterface. As to not break Chromium, both versions of CreatePeerConnection need to exist for a transition period. This will be done by wrapping a store into a generator wrapper - RTCCertificateGeneratorStoreWrapper. BUG=webrtc:5707, webrtc:5708 R=hta@webrtc.org, tommi@chromium.org, tommi@webrtc.org Committed: https://crrev.com/6c96314b42e872abbba3884904a4fdc3dfa8955d Cr-Commit-Position: refs/heads/master@{#12879} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) manually as 6c96314b42e872abbba3884904a4fdc3dfa8955d (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
