Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2166873002:
Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. (Closed)
DescriptionModified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage.
To allow end-to-end QuicDataChannel usage with a
PeerConnection, RTCConfiguration has been modified to
include a boolean for whether to do QUIC, since negotiation of
QUIC is not implemented. If one peer does QUIC, then it will be
assumed that the other peer must do QUIC or the connection
will fail.
PeerConnection has been modified to create data channels of type
QuicDataChannel when the peer wants to do QUIC.
WebRtcSession has ben modified to use a QuicDataTransport
instead of a DtlsTransportChannelWrapper/DataChannel
when QUIC should be used
QuicDataTransport implements the generic functions of
BaseChannel to manage the QuicTransportChannel.
Committed: https://crrev.com/34b54c36a533dadb6ceb70795119194e6f530ef5
Committed: https://crrev.com/9763d56464b9f660e21a6737a73155009d5b3e00
Cr-Original-Commit-Position: refs/heads/master@{#13645}
Cr-Commit-Position: refs/heads/master@{#13657}
Patch Set 1 #Patch Set 2 : Minor fix. #
Total comments: 32
Patch Set 3 : cr comments #Patch Set 4 : Minor fix in unit tests. #Patch Set 5 : Minor changes. #
Total comments: 15
Patch Set 6 : Modified the quicdatatransport and faketransportcontroller. #
Total comments: 6
Patch Set 7 : Expose the ice transport channel from QuicTransportChannel. #
Total comments: 1
Patch Set 8 : Minor fix. #Patch Set 9 : Fix the broken test in Chromium. #
Total comments: 1
Patch Set 10 : Change the comments and minor fix. #
Messages
Total messages: 43 (20 generated)
Description was changed from ========== Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. To allow end-to-end QuicDataChannel usage with PeerConnection. ========== to ========== Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicDataTransport instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used. QuicDataTransport implements the generic functions of BaseChannel to manage the QuicTransportChannel. ==========
Description was changed from ========== Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicDataTransport instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used. QuicDataTransport implements the generic functions of BaseChannel to manage the QuicTransportChannel. ========== to ========== Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicDataTransport instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used QuicDataTransport implements the generic functions of BaseChannel to manage the QuicTransportChannel. ==========
zhihuang@webrtc.org changed reviewers: + deadbeef@webrtc.org, pthatcher@webrtc.org
My main question is: can we really support changing the transport of a QuicDataChannel? My understanding was that the state of a QuicDataChannel is tied to the transport, so by switching the transport some packets could be dropped (even though the channel is supposed to be reliable). If this is correct, then I'd expect: * Setting a description that bundles on "audio" with max-bundle is OK. * Setting a description with no bundling at all is OK. * Setting a description without max-bundle, then one that bundles on "audio" should fail, because the QuicDataTransport would be changing its transport from "data" to "audio". https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... File webrtc/api/quicdatachannel.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:242: // Set new transport channel. Is it possible to switch transports in the middle of a session? Couldn't that cause packets to be lost, since we're losing the state of the streams in the old transport? https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... File webrtc/api/quicdatachannel_unittest.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel_unittest.cc:617: EXPECT_TRUE(data_channel->SetTransportChannel(other_transport_channel)); The old expectation seems correct to me, I don't think we want to allow a QuicDataChannel to switch transports. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... File webrtc/api/quicdatatransport.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:41: void QuicDataTransport::DestroyTransportChannels_n() { nit: I think the location of the method in the .cc file should match where it is in the .h file. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:90: bool QuicDataTransport::SetTransport_n(const std::string& transport_name) { I think there needs to be a way to un-set the transport (for example, when PeerConnection::Close is called, or a description is set with "m=application 0"). This can be done by defining a new method or using an empty string to signify "no transport" in this method. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.cc File webrtc/api/webrtcsession.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1704: #endif // HAVE_QUIC Ok, so when using QUIC, the QuicTransportChannel always exists, but its content name isn't populated until SetXDescription. This seems fine. But why don't you use the SetTransport method here, which will handle creating the transport channel? If you do that, set_transport_name and SetTransportChannel can also be private. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:2062: } // namespace webrtc We still need to clean up the QuicDataTransport at the correct times. Wherever the cricket::DataChannel would be destroyed, we should call SetTransport("") or something which will make the QuicDataTransport tear down its transport channel. There's also the fact that bundling isn't handled. If you try to bundle data on the audio transport, nothing will happen. I guess this is ok for now as long as we document it, but we had talked about returning an error. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession_... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession_... webrtc/api/webrtcsession_unittest.cc:4184: session_->quic_data_transport()->SetTransport(transport_name); Again, is this something we can really support?
On 2016/07/21 23:39:57, Taylor Brandstetter wrote: > My main question is: can we really support changing the transport of a > QuicDataChannel? My understanding was that the state of a QuicDataChannel is > tied to the transport, so by switching the transport some packets could be > dropped (even though the channel is supposed to be reliable). I was originally thinking that a QuicDataChannel cannot move to a different QuicTransportChannel, but now that I think about, it *could* happen if we were willing to drop all the pending messages being sent. That probably wouldn't be worth it, but it does mean that if we've never been open yet and haven't started sending anything yet, then there's nothing to drop, so we could switch. In other words, if the switch happens before the QuicDataChannel ever goes to the open state, we could easily switch it. > > If this is correct, then I'd expect: > > * Setting a description that bundles on "audio" with max-bundle is OK. > * Setting a description with no bundling at all is OK. > * Setting a description without max-bundle, then one that bundles on "audio" > should fail, because the QuicDataTransport would be changing its transport from > "data" to "audio". > As long as the we haven't sent anything than we could do the last without failing. Whether that's worth it or not, I'm not sure. > https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... > File webrtc/api/quicdatachannel.cc (right): > > https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... > webrtc/api/quicdatachannel.cc:242: // Set new transport channel. > Is it possible to switch transports in the middle of a session? Couldn't that > cause packets to be lost, since we're losing the state of the streams in the old > transport? You're correct: we'd have to drop all the messages that we've started sending but haven't finished sending. So it's probably not worth supporting. > > https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... > File webrtc/api/quicdatachannel_unittest.cc (right): > > https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... > webrtc/api/quicdatachannel_unittest.cc:617: > EXPECT_TRUE(data_channel->SetTransportChannel(other_transport_channel)); > The old expectation seems correct to me, I don't think we want to allow a > QuicDataChannel to switch transports. > > https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... > File webrtc/api/quicdatatransport.cc (right): > > https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... > webrtc/api/quicdatatransport.cc:41: void > QuicDataTransport::DestroyTransportChannels_n() { > nit: I think the location of the method in the .cc file should match where it is > in the .h file. > > https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... > webrtc/api/quicdatatransport.cc:90: bool QuicDataTransport::SetTransport_n(const > std::string& transport_name) { > I think there needs to be a way to un-set the transport (for example, when > PeerConnection::Close is called, or a description is set with "m=application > 0"). This can be done by defining a new method or using an empty string to > signify "no transport" in this method. > > https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.cc > File webrtc/api/webrtcsession.cc (right): > > https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.... > webrtc/api/webrtcsession.cc:1704: #endif // HAVE_QUIC > Ok, so when using QUIC, the QuicTransportChannel always exists, but its content > name isn't populated until SetXDescription. This seems fine. But why don't you > use the SetTransport method here, which will handle creating the transport > channel? If you do that, set_transport_name and SetTransportChannel can also be > private. > > https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.... > webrtc/api/webrtcsession.cc:2062: } // namespace webrtc > We still need to clean up the QuicDataTransport at the correct times. Wherever > the cricket::DataChannel would be destroyed, we should call SetTransport("") or > something which will make the QuicDataTransport tear down its transport channel. > > There's also the fact that bundling isn't handled. If you try to bundle data on > the audio transport, nothing will happen. I guess this is ok for now as long as > we document it, but we had talked about returning an error. > > https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession_... > File webrtc/api/webrtcsession_unittest.cc (right): > > https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession_... > webrtc/api/webrtcsession_unittest.cc:4184: > session_->quic_data_transport()->SetTransport(transport_name); > Again, is this something we can really support?
https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... File webrtc/api/quicdatatransport.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:90: bool QuicDataTransport::SetTransport_n(const std::string& transport_name) { On 2016/07/21 23:39:57, Taylor Brandstetter wrote: > I think there needs to be a way to un-set the transport (for example, when > PeerConnection::Close is called, or a description is set with "m=application > 0"). This can be done by defining a new method or using an empty string to > signify "no transport" in this method. I don't think we need to unset the transport. Just close the transport underneath and close the data channels. Why do you need more?
https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... File webrtc/api/quicdatachannel.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:242: // Set new transport channel. On 2016/07/21 23:39:57, Taylor Brandstetter wrote: > Is it possible to switch transports in the middle of a session? Couldn't that > cause packets to be lost, since we're losing the state of the streams in the old > transport? We should at least do this: if (state_ == kOpen) { return false; } Otherwise, I think changing the transport channel will work. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... File webrtc/api/quicdatachannel_unittest.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel_unittest.cc:617: EXPECT_TRUE(data_channel->SetTransportChannel(other_transport_channel)); On 2016/07/21 23:39:57, Taylor Brandstetter wrote: > The old expectation seems correct to me, I don't think we want to allow a > QuicDataChannel to switch transports. As long as it hasn't been open yet, it might be OK. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... File webrtc/api/quicdatatransport.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:44: transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTP); Can you add a new component constant, perhaps ICE_CANDIDATE_COMPONENT_DEFAULT (with the same value), so we don't need "RTP" in here? https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:65: transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTP); Can you put this in a common method, perhaps DestroyQuicTransportChannel() and call it from the two placs? https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:100: transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP))); Can you put this in a CreateQuicTransportChannel method next to the DestroyQuicTransportChannel method and use it here? https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... File webrtc/api/quicdatatransport.h (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.h:76: } Can you put this together with SetTransport and put in a comment that says that the QuicDataTransport is acting like a BaseChannel with those methods? That would make things more clear. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.cc File webrtc/api/webrtcsession.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1702: return true; I don't understand why we create the quic_transport_channel_ here, and why we store it in WebrtcSession at all. Just calling quic_data_transport_->SetTransport(content->name) here instead of SetTransportChnnel and set_transport_name would accomplish the same in more simple way, wouldn't it? https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1911: cricket::BaseChannel* channel = GetChannel(content.name); I think we should add a method called GetTransportName(content.name) and use that. Then we could put in the #ifdef HAVE_QUIC version to return that transport name and only call transport_controller_->ReadyForRemoteCandidates once. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1918: quic_transport_channel_->transport_name()); Why get it from the quic_transport_channel_ instead of getting it from the quic_data_transport_? https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:2062: } // namespace webrtc On 2016/07/21 23:39:57, Taylor Brandstetter wrote: > We still need to clean up the QuicDataTransport at the correct times. Wherever > the cricket::DataChannel would be destroyed, we should call SetTransport("") or > something which will make the QuicDataTransport tear down its transport channel. > Agreed, but I would say we should just destroy the QuicDataTransport, not SetTransport(""). > There's also the fact that bundling isn't handled. If you try to bundle data on > the audio transport, nothing will happen. I guess this is ok for now as long as > we document it, but we had talked about returning an error. I think we should go all the way and call SetTransportName at the right time (in the code that enables bundle). That was the whole point of making QuicDataTransport look like a BaseChannel, wasn't it? https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession_... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession_... webrtc/api/webrtcsession_unittest.cc:564: session_options->data_channel_type = cricket::DCT_QUIC; Why not just do session_options->data_channel_type = session_->data_channel_type() ?
On 2016/07/22 09:44:40, pthatcher1 wrote: > On 2016/07/21 23:39:57, Taylor Brandstetter wrote: > > My main question is: can we really support changing the transport of a > > QuicDataChannel? My understanding was that the state of a QuicDataChannel is > > tied to the transport, so by switching the transport some packets could be > > dropped (even though the channel is supposed to be reliable). > > I was originally thinking that a QuicDataChannel cannot move to a different > QuicTransportChannel, but now that I think about, it *could* happen if we were > willing to drop all the pending messages being sent. That probably wouldn't be > worth it, but it does mean that if we've never been open yet and haven't started > sending anything yet, then there's nothing to drop, so we could switch. I think this violates the expectation of the API. "reliable" is supposed to mean 100% reliable, not "reliable unless you start bundling on a different transport". Allowing this could lead to bugs that are hard to root-cause. So, I'm still of the opinion that we shouldn't allow switching the QUIC transport until we can do so reliably.
https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... File webrtc/api/quicdatachannel.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:242: // Set new transport channel. On 2016/07/22 17:57:57, pthatcher1 wrote: > On 2016/07/21 23:39:57, Taylor Brandstetter wrote: > > Is it possible to switch transports in the middle of a session? Couldn't that > > cause packets to be lost, since we're losing the state of the streams in the > old > > transport? > > We should at least do this: > > if (state_ == kOpen) { > return false; > } > > > Otherwise, I think changing the transport channel will work. If we base the success of "Set___Description" on the open state of the data channel, that seems like it's asking for trouble. Sometimes it will succeed and sometimes it won't depending on timing. I'd prefer to just always disallow it, and tell people to use max-bundle or force bundling on the data transport. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... File webrtc/api/quicdatatransport.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:90: bool QuicDataTransport::SetTransport_n(const std::string& transport_name) { On 2016/07/22 10:46:03, pthatcher1 wrote: > On 2016/07/21 23:39:57, Taylor Brandstetter wrote: > > I think there needs to be a way to un-set the transport (for example, when > > PeerConnection::Close is called, or a description is set with "m=application > > 0"). This can be done by defining a new method or using an empty string to > > signify "no transport" in this method. > > I don't think we need to unset the transport. Just close the transport > underneath and close the data channels. Why do you need more? That's what unsetting the transport would do. Are you suggesting WebRtcSession does this directly, as opposed to going through QuicDataTransport? I don't like that. It means the ownership of the transport is fuzzy, and we'd still need a way to tell QuicDataTransport "your transport is closed, stop using it".
https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... File webrtc/api/quicdatachannel.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatachanne... webrtc/api/quicdatachannel.cc:242: // Set new transport channel. On 2016/07/22 19:04:56, Taylor Brandstetter wrote: > On 2016/07/22 17:57:57, pthatcher1 wrote: > > On 2016/07/21 23:39:57, Taylor Brandstetter wrote: > > > Is it possible to switch transports in the middle of a session? Couldn't > that > > > cause packets to be lost, since we're losing the state of the streams in the > > old > > > transport? > > > > We should at least do this: > > > > if (state_ == kOpen) { > > return false; > > } > > > > > > Otherwise, I think changing the transport channel will work. > > If we base the success of "Set___Description" on the open state of the data > channel, that seems like it's asking for trouble. Sometimes it will succeed and > sometimes it won't depending on timing. I'd prefer to just always disallow it, > and tell people to use max-bundle or force bundling on the data transport. I will revert this change for now. I think we might need this when negotiation. But that comes later. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... File webrtc/api/quicdatatransport.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:41: void QuicDataTransport::DestroyTransportChannels_n() { On 2016/07/21 23:39:57, Taylor Brandstetter wrote: > nit: I think the location of the method in the .cc file should match where it is > in the .h file. Done. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:44: transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTP); On 2016/07/22 17:57:57, pthatcher1 wrote: > Can you add a new component constant, perhaps ICE_CANDIDATE_COMPONENT_DEFAULT > (with the same value), so we don't need "RTP" in here? We already have "const int ICE_CANDIDATE_COMPONENT_DEFAULT = 1;" in p2pconstants.cc. I will use that one. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:65: transport_name_, cricket::ICE_CANDIDATE_COMPONENT_RTP); On 2016/07/22 17:57:57, pthatcher1 wrote: > Can you put this in a common method, perhaps DestroyQuicTransportChannel() and > call it from the two placs? Since we are not supporting switching the transportchannel in QuicDataChannel for now, I will disable the support of switching the transport channel for QuicDataTransport as well. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:90: bool QuicDataTransport::SetTransport_n(const std::string& transport_name) { On 2016/07/22 19:04:56, Taylor Brandstetter wrote: > On 2016/07/22 10:46:03, pthatcher1 wrote: > > On 2016/07/21 23:39:57, Taylor Brandstetter wrote: > > > I think there needs to be a way to un-set the transport (for example, when > > > PeerConnection::Close is called, or a description is set with "m=application > > > 0"). This can be done by defining a new method or using an empty string to > > > signify "no transport" in this method. > > > > I don't think we need to unset the transport. Just close the transport > > underneath and close the data channels. Why do you need more? > > That's what unsetting the transport would do. Are you suggesting WebRtcSession > does this directly, as opposed to going through QuicDataTransport? I don't like > that. It means the ownership of the transport is fuzzy, and we'd still need a > way to tell QuicDataTransport "your transport is closed, stop using it". I think what Peter suggested is that we can directly delete the QuicDataTransport instead of unsetting the transport for it. The transport and channels will be destroyed in the destructor of QuicDataTransport. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:100: transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP))); On 2016/07/22 17:57:57, pthatcher1 wrote: > Can you put this in a CreateQuicTransportChannel method next to the > DestroyQuicTransportChannel method and use it here? Done. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... File webrtc/api/quicdatatransport.h (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.h:76: } On 2016/07/22 17:57:57, pthatcher1 wrote: > Can you put this together with SetTransport and put in a comment that says that > the QuicDataTransport is acting like a BaseChannel with those methods? That > would make things more clear. Done. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.cc File webrtc/api/webrtcsession.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1702: return true; On 2016/07/22 17:57:57, pthatcher1 wrote: > I don't understand why we create the quic_transport_channel_ here, and why we > store it in WebrtcSession at all. Just calling > quic_data_transport_->SetTransport(content->name) here instead of > SetTransportChnnel and set_transport_name would accomplish the same in more > simple way, wouldn't it? Yes, we don't need quic_transport_channel_ here anymore. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1911: cricket::BaseChannel* channel = GetChannel(content.name); On 2016/07/22 17:57:57, pthatcher1 wrote: > I think we should add a method called GetTransportName(content.name) and use > that. Then we could put in the #ifdef HAVE_QUIC version to return that > transport name and only call transport_controller_->ReadyForRemoteCandidates > once. Done. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1918: quic_transport_channel_->transport_name()); On 2016/07/22 17:57:57, pthatcher1 wrote: > Why get it from the quic_transport_channel_ instead of getting it from the > quic_data_transport_? Done. https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession_... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/2166873002/diff/20001/webrtc/api/webrtcsession_... webrtc/api/webrtcsession_unittest.cc:564: session_options->data_channel_type = cricket::DCT_QUIC; On 2016/07/22 17:57:57, pthatcher1 wrote: > Why not just do > > session_options->data_channel_type = session_->data_channel_type() > > ? Oh, I should have noticed this. https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/webrtcsession.cc File webrtc/api/webrtcsession.cc (right): https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1071: If we do not support changing the underneath transport channels, then we may have to figure out some way to negotiate the bundle. https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1709: quic_data_transport_->SetTransport(transport_name); Is it better to make it "quic_data_transport_->SetTransport(bundle_transport ? *bundle_transport : content->name); " ?
Pretty close. https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:903: // TODO(mikescarlett): Handle case when config is NULL. Maybe we should make these TODO(zhihuang) :). https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/quicdatatransp... File webrtc/api/quicdatatransport.cc (right): https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:35: DestroyTransportChannels(); DestroyTransportChannel(quic_transport_channel_); https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:44: tc = CreateTransportChannel(transport_name); if (!SetTransportChannel(tc)) { DestroyTransportChannel(tc); return false; } https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:49: } You don't need this if you have the "return false" above. https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:183: } You don't need this https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:187: void QuicDataTransport::DestroyTransportChannels() { DestroyTransportChannel(TransportChannel* tc) https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:188: if (quic_transport_channel_) { if (!tc) { return; } network_thread_->Invoke<void>(...); https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/webrtcsession.cc File webrtc/api/webrtcsession.cc (right): https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1709: quic_data_transport_->SetTransport(transport_name); Why don't we do this: quic_data_transport_.reset(new QuicDataTransport(..., content->name, transport_name)); ? Then you can simplify the QuicDataTransport code and ignore half my comments there :)
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #6 (id:140001) has been deleted
https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/peerconnection.cc File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/peerconnection... webrtc/api/peerconnection.cc:903: // TODO(mikescarlett): Handle case when config is NULL. On 2016/07/27 23:21:49, pthatcher1 wrote: > Maybe we should make these TODO(zhihuang) :). Done. https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/quicdatatransp... File webrtc/api/quicdatatransport.cc (right): https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:35: DestroyTransportChannels(); On 2016/07/27 23:21:49, pthatcher1 wrote: > DestroyTransportChannel(quic_transport_channel_); Done. https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:49: } On 2016/07/27 23:21:49, pthatcher1 wrote: > You don't need this if you have the "return false" above. Done. https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/quicdatatransp... webrtc/api/quicdatatransport.cc:183: } On 2016/07/27 23:21:49, pthatcher1 wrote: > You don't need this Done. https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/webrtcsession.cc File webrtc/api/webrtcsession.cc (right): https://codereview.webrtc.org/2166873002/diff/80001/webrtc/api/webrtcsession.... webrtc/api/webrtcsession.cc:1709: quic_data_transport_->SetTransport(transport_name); On 2016/07/27 23:21:49, pthatcher1 wrote: > Why don't we do this: > > quic_data_transport_.reset(new QuicDataTransport(..., content->name, > transport_name)); > > ? > > > Then you can simplify the QuicDataTransport code and ignore half my comments > there :) The original design is that the quic_data_transport_ will be created when initializing the webrtcsession and the quic_transport_channel will be set when calling CreateDataChannel. Doing this means that we can not create QuicDataChannel(the api one) from peerconnection before calling CreateDataChannel(the pc one). Are we trying to do this? Also it seems that we can not create the quic_data_transport earlier if we change the constructor because we don't know the transport_name and content name. https://codereview.webrtc.org/2166873002/diff/160001/webrtc/p2p/base/transpor... File webrtc/p2p/base/transportcontroller.cc (left): https://codereview.webrtc.org/2166873002/diff/160001/webrtc/p2p/base/transpor... webrtc/p2p/base/transportcontroller.cc:204: I will fix these.
Looks good, I just have a couple more minor concerns about the tests. SIDENOTE: This CL has made me realize that having BaseChannel (and by extension QuicDataTransport) depend on TransportController was a poor design choice. If nothing else, it makes BaseChannel hard to unit test. It also means multiple classes are responsible for doing proper reference counting (with Create/DeleteTransportChannel). I think later we should do some refactoring, and move this dependency on TransportController up to WebRtcSession/PeerConnection. https://codereview.webrtc.org/2166873002/diff/160001/webrtc/api/quicdatatrans... File webrtc/api/quicdatatransport.cc (right): https://codereview.webrtc.org/2166873002/diff/160001/webrtc/api/quicdatatrans... webrtc/api/quicdatatransport.cc:47: if (!SetTransportChannel(transport_channel)) { I like the way you did this. If we later support changing the transport while a data channel is open, we'll only have to change the SetTransportChannel method. https://codereview.webrtc.org/2166873002/diff/160001/webrtc/api/quicdatatrans... File webrtc/api/quicdatatransport_unittest.cc (left): https://codereview.webrtc.org/2166873002/diff/160001/webrtc/api/quicdatatrans... webrtc/api/quicdatatransport_unittest.cc:355: Do we not need this test any more? I think it's still relevant. Only now, it's that calling 'SetTransport("a")' followed by 'SetTransport("b")' will fail. https://codereview.webrtc.org/2166873002/diff/160001/webrtc/p2p/base/faketran... File webrtc/p2p/base/faketransportcontroller.h (right): https://codereview.webrtc.org/2166873002/diff/160001/webrtc/p2p/base/faketran... webrtc/p2p/base/faketransportcontroller.h:619: FakeTransportChannel* fake_ice_transport_channel_; I don't think having the FakeTransportController own a single fake ICE transport channel will work as we add more tests. If you create two QuicTransportChannels, it will be double-freed. And if you create zero, it will leak. How about this: In FakeQuicTransport::CreateTransportChannel, you create a new FakeTransportChannel each time. Then you access it by calling an "ice_transport_channel()" method on the QuicTransportChannel, and casting it to a FakeTransportChannel in the test (since the test knows that's what it will be). I think it's appropriate for the QuicTransportChannel to have a method exposing its ICE transport channel, since that's already something exposed in the API: https://www.w3.org/TR/webrtc/#dom-rtcdtlstransport-transport Or another alternative: FakeQuicTransport creates a FakeQuicTransportChannel, which is a subclass of QuicTransportChannel that has a "fake_ice_transport_channel()" method. So something like this: class FakeQuicTransportChannel : QuicTransportChannel { public: FakeQuicTransportChannel(FakeTransportChannel* ch) : QuicTransportChannel(ch), fake_transport_channel_(ch) { } FakeTransportChannel* fake_transport_channel() { return fake_transport_channel_; } private: FakeTransportChannel* fake_transport_channel_; }; Then in FakeQuicTransport::CreateTransportChannel you'd do: return new FakeQuicTransportChannel(new FakeTransportChannel(name(), component()));
Patchset #7 (id:180001) has been deleted
Patchset #7 (id:200001) has been deleted
Patchset #7 (id:220001) has been deleted
Patchset #7 (id:240001) has been deleted
https://codereview.webrtc.org/2166873002/diff/160001/webrtc/api/quicdatatrans... File webrtc/api/quicdatatransport_unittest.cc (left): https://codereview.webrtc.org/2166873002/diff/160001/webrtc/api/quicdatatrans... webrtc/api/quicdatatransport_unittest.cc:355: On 2016/08/01 18:05:03, Taylor Brandstetter wrote: > Do we not need this test any more? I think it's still relevant. Only now, it's > that calling 'SetTransport("a")' followed by 'SetTransport("b")' will fail. Agreed. I will add it back. https://codereview.webrtc.org/2166873002/diff/160001/webrtc/p2p/base/faketran... File webrtc/p2p/base/faketransportcontroller.h (right): https://codereview.webrtc.org/2166873002/diff/160001/webrtc/p2p/base/faketran... webrtc/p2p/base/faketransportcontroller.h:619: FakeTransportChannel* fake_ice_transport_channel_; On 2016/08/01 18:05:03, Taylor Brandstetter wrote: > I don't think having the FakeTransportController own a single fake ICE transport > channel will work as we add more tests. If you create two QuicTransportChannels, > it will be double-freed. And if you create zero, it will leak. > > How about this: In FakeQuicTransport::CreateTransportChannel, you create a new > FakeTransportChannel each time. Then you access it by calling an > "ice_transport_channel()" method on the QuicTransportChannel, and casting it to > a FakeTransportChannel in the test (since the test knows that's what it will > be). > > I think it's appropriate for the QuicTransportChannel to have a method exposing > its ICE transport channel, since that's already something exposed in the API: > https://www.w3.org/TR/webrtc/#dom-rtcdtlstransport-transport > > Or another alternative: FakeQuicTransport creates a FakeQuicTransportChannel, > which is a subclass of QuicTransportChannel that has a > "fake_ice_transport_channel()" method. So something like this: > > class FakeQuicTransportChannel : QuicTransportChannel { > public: > FakeQuicTransportChannel(FakeTransportChannel* ch) : > QuicTransportChannel(ch), > fake_transport_channel_(ch) { > } > > FakeTransportChannel* fake_transport_channel() { > return fake_transport_channel_; > } > > private: > FakeTransportChannel* fake_transport_channel_; > }; > > Then in FakeQuicTransport::CreateTransportChannel you'd do: > > return new FakeQuicTransportChannel(new FakeTransportChannel(name(), > component())); This is a good point! I like the idea to expose the ice_tranport_channel.
lgtm https://codereview.webrtc.org/2166873002/diff/260001/webrtc/p2p/base/faketran... File webrtc/p2p/base/faketransportcontroller.h (right): https://codereview.webrtc.org/2166873002/diff/260001/webrtc/p2p/base/faketran... webrtc/p2p/base/faketransportcontroller.h:468: FakeTransportChannel* fake_ice_transport_channel_ = nit: Since this is a local variable it shouldn't have an underscore at the end of its name.
On 2016/08/01 18:05:03, Taylor Brandstetter wrote: > Looks good, I just have a couple more minor concerns about the tests. > > SIDENOTE: This CL has made me realize that having BaseChannel (and by extension > QuicDataTransport) depend on TransportController was a poor design choice. If > nothing else, it makes BaseChannel hard to unit test. It also means multiple > classes are responsible for doing proper reference counting (with > Create/DeleteTransportChannel). I think later we should do some refactoring, and > move this dependency on TransportController up to WebRtcSession/PeerConnection. I agree. More generally for BaseChannel, we should move away from having a BaseChannel at all. More generally for TransportController, we should be able to simplify things a lot once we don't have BaseChannel. The only reason we have any of that complexity is because of BUNDLE and RTCP-mux. RTCP-mux is going away and BUNDLE could be controlled completely by PeerConnection (once BaseChannel is gone). More generally for ref-counting, we need a good solution for how to hand out ORTC-style IceTransport/DtlsTransport/QuicTransport/SctpTransport that doesn't have a PeerConnection or BaseChannel as an owner, but has references floating around in Javascript, Java, or Objc. > > https://codereview.webrtc.org/2166873002/diff/160001/webrtc/api/quicdatatrans... > File webrtc/api/quicdatatransport.cc (right): > > https://codereview.webrtc.org/2166873002/diff/160001/webrtc/api/quicdatatrans... > webrtc/api/quicdatatransport.cc:47: if (!SetTransportChannel(transport_channel)) > { > I like the way you did this. If we later support changing the transport while a > data channel is open, we'll only have to change the SetTransportChannel method. > > https://codereview.webrtc.org/2166873002/diff/160001/webrtc/api/quicdatatrans... > File webrtc/api/quicdatatransport_unittest.cc (left): > > https://codereview.webrtc.org/2166873002/diff/160001/webrtc/api/quicdatatrans... > webrtc/api/quicdatatransport_unittest.cc:355: > Do we not need this test any more? I think it's still relevant. Only now, it's > that calling 'SetTransport("a")' followed by 'SetTransport("b")' will fail. > > https://codereview.webrtc.org/2166873002/diff/160001/webrtc/p2p/base/faketran... > File webrtc/p2p/base/faketransportcontroller.h (right): > > https://codereview.webrtc.org/2166873002/diff/160001/webrtc/p2p/base/faketran... > webrtc/p2p/base/faketransportcontroller.h:619: FakeTransportChannel* > fake_ice_transport_channel_; > I don't think having the FakeTransportController own a single fake ICE transport > channel will work as we add more tests. If you create two QuicTransportChannels, > it will be double-freed. And if you create zero, it will leak. > > How about this: In FakeQuicTransport::CreateTransportChannel, you create a new > FakeTransportChannel each time. Then you access it by calling an > "ice_transport_channel()" method on the QuicTransportChannel, and casting it to > a FakeTransportChannel in the test (since the test knows that's what it will > be). > > I think it's appropriate for the QuicTransportChannel to have a method exposing > its ICE transport channel, since that's already something exposed in the API: > https://www.w3.org/TR/webrtc/#dom-rtcdtlstransport-transport > > Or another alternative: FakeQuicTransport creates a FakeQuicTransportChannel, > which is a subclass of QuicTransportChannel that has a > "fake_ice_transport_channel()" method. So something like this: > > class FakeQuicTransportChannel : QuicTransportChannel { > public: > FakeQuicTransportChannel(FakeTransportChannel* ch) : > QuicTransportChannel(ch), > fake_transport_channel_(ch) { > } > > FakeTransportChannel* fake_transport_channel() { > return fake_transport_channel_; > } > > private: > FakeTransportChannel* fake_transport_channel_; > }; > > Then in FakeQuicTransport::CreateTransportChannel you'd do: > > return new FakeQuicTransportChannel(new FakeTransportChannel(name(), > component()));
lgtm
The CQ bit was checked by zhihuang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2166873002/#ps280001 (title: "Minor fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicDataTransport instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used QuicDataTransport implements the generic functions of BaseChannel to manage the QuicTransportChannel. ========== to ========== Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicDataTransport instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used QuicDataTransport implements the generic functions of BaseChannel to manage the QuicTransportChannel. ==========
Message was sent while issue was closed.
Committed patchset #8 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicDataTransport instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used QuicDataTransport implements the generic functions of BaseChannel to manage the QuicTransportChannel. ========== to ========== Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicDataTransport instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used QuicDataTransport implements the generic functions of BaseChannel to manage the QuicTransportChannel. Committed: https://crrev.com/34b54c36a533dadb6ceb70795119194e6f530ef5 Cr-Commit-Position: refs/heads/master@{#13645} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/34b54c36a533dadb6ceb70795119194e6f530ef5 Cr-Commit-Position: refs/heads/master@{#13645}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:280001) has been created in https://codereview.webrtc.org/2206793007/ by deadbeef@webrtc.org. The reason for reverting is: Reverting because it broke an RTP data channel test on the FYI bots..
Message was sent while issue was closed.
Description was changed from ========== Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicDataTransport instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used QuicDataTransport implements the generic functions of BaseChannel to manage the QuicTransportChannel. Committed: https://crrev.com/34b54c36a533dadb6ceb70795119194e6f530ef5 Cr-Commit-Position: refs/heads/master@{#13645} ========== to ========== Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicDataTransport instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used QuicDataTransport implements the generic functions of BaseChannel to manage the QuicTransportChannel. Committed: https://crrev.com/34b54c36a533dadb6ceb70795119194e6f530ef5 Cr-Commit-Position: refs/heads/master@{#13645} ==========
https://codereview.webrtc.org/2166873002/diff/300001/webrtc/api/peerconnectio... File webrtc/api/peerconnection.cc (right): https://codereview.webrtc.org/2166873002/diff/300001/webrtc/api/peerconnectio... webrtc/api/peerconnection.cc:1639: // condition. Otherwise the unit tests in WebRtcDataBrowserTest will fail when Maybe also add "and RTP data channels would be successfully negotiated by default" to this comment and the one below. I think more important than the unit test failing is that we want to leave RTP data channels broken, so people won't try to use them.
Patchset #10 (id:320001) has been deleted
The CQ bit was checked by zhihuang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2166873002/#ps340001 (title: "Change the comments and minor fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicDataTransport instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used QuicDataTransport implements the generic functions of BaseChannel to manage the QuicTransportChannel. Committed: https://crrev.com/34b54c36a533dadb6ceb70795119194e6f530ef5 Cr-Commit-Position: refs/heads/master@{#13645} ========== to ========== Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicDataTransport instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used QuicDataTransport implements the generic functions of BaseChannel to manage the QuicTransportChannel. Committed: https://crrev.com/34b54c36a533dadb6ceb70795119194e6f530ef5 Cr-Commit-Position: refs/heads/master@{#13645} ==========
Message was sent while issue was closed.
Committed patchset #10 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicDataTransport instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used QuicDataTransport implements the generic functions of BaseChannel to manage the QuicTransportChannel. Committed: https://crrev.com/34b54c36a533dadb6ceb70795119194e6f530ef5 Cr-Commit-Position: refs/heads/master@{#13645} ========== to ========== Modified PeerConnection and WebRtcSession for end-to-end QuicDataChannel usage. To allow end-to-end QuicDataChannel usage with a PeerConnection, RTCConfiguration has been modified to include a boolean for whether to do QUIC, since negotiation of QUIC is not implemented. If one peer does QUIC, then it will be assumed that the other peer must do QUIC or the connection will fail. PeerConnection has been modified to create data channels of type QuicDataChannel when the peer wants to do QUIC. WebRtcSession has ben modified to use a QuicDataTransport instead of a DtlsTransportChannelWrapper/DataChannel when QUIC should be used QuicDataTransport implements the generic functions of BaseChannel to manage the QuicTransportChannel. Committed: https://crrev.com/34b54c36a533dadb6ceb70795119194e6f530ef5 Committed: https://crrev.com/9763d56464b9f660e21a6737a73155009d5b3e00 Cr-Original-Commit-Position: refs/heads/master@{#13645} Cr-Commit-Position: refs/heads/master@{#13657} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/9763d56464b9f660e21a6737a73155009d5b3e00 Cr-Commit-Position: refs/heads/master@{#13657} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
