Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1721673004:
Create QuicTransportChannel (Closed)
|
Created:
4 years, 10 months ago by mikescarlett Modified:
4 years, 9 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. |
DescriptionCreate QuicTransportChannel
This new class allows usage of a QuicSession to establish a QUIC handshake.
BUG=
Committed: https://crrev.com/6459f84766dd6ece67e70c8743aeb2378ac267be
Cr-Commit-Position: refs/heads/master@{#11873}
Patch Set 1 #
Total comments: 74
Patch Set 2 : Respond to pthatcher's initial comments #Patch Set 3 : Minor documentation update #Patch Set 4 : Cleanup unit test #
Total comments: 10
Patch Set 5 : Modify WritePacket() and respond to comments #
Total comments: 20
Patch Set 6 : Inline and rename unit test methods #
Total comments: 16
Patch Set 7 : Respond to pthatcher and honghaiz #
Total comments: 8
Patch Set 8 : Respond to honghaiz #Patch Set 9 : Update documentation before committing #
Messages
Total messages: 35 (12 generated)
Description was changed from ========== Create QuicTransportChannel This new class allows usage of a QuicSession to establish a QUIC handshake. BUG= ========== to ========== Create QuicTransportChannel This new class allows usage of a QuicSession to establish a QUIC handshake. BUG= ==========
mikescarlett@webrtc.org changed reviewers: + pthatcher@webrtc.org
pthatcher@webrtc.org changed reviewers: + deadbeef@webrtc.org
Taylor, can you review this as well?
I'm not even 1/4 the way through so far. Still working on it. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... File webrtc/p2p/quic/quictransportchannel.h (right): https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:30: // Negotiated, and has an ecrypted connection. encrypted https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:51: // PF_SRTP_BYPASS flag after the QUIC handshake. I understand why we pass SRTP packets directly, but why would we pass packets directly before the handshake has begun? https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:70: explicit QuicTransportChannel(TransportChannelImpl* channel); Why does this need a TransportChannelImpl instead of just a TransportChannel? https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:83: // TODO(mikescarlett): Method name is DTLS-specific. Can you change the TODOs to be action to take, such as "Rename this so it's not DTLS-specific", or "Implement certification authentication."? https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:145: bool GetSrtpCryptoSuite(int* cipher) override; In voice-only calls, we tend to use SRTP_AES128_CM_SHA1_32 instead. Perhaps we need to add our own layer of negotiation on top of QUIC. That's definitely a big TODO. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:152: // use by the remote peer, for use in external identity verification. This is really just used for stats, so we should figure out which stats make sense for QUIC.
This looks very good overall. But there are a few things to fix. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... File webrtc/p2p/quic/quictransportchannel.cc (right): https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:44: const int kIdleConnectionStateLifetime = 1000; Is this in ms? If so, I don't think 1000 is long enough. With ICE, you could see 10-15 seconds of a disconnected state and then come back. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:47: const size_t kInputKeyingMaterialLength = 32; Where does this come from? https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:68: class InsecureProofSource : public net::ProofSource { Might as well call this DummyProofSource https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:83: certs->push_back("Required to establish handshake"); And say "Dummy cert" https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:84: std::string signature("Signature"); And "Dummy signature" https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:149: config_.SetBytesForConnectionIdToSend(0); What's this? https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:165: LOG_J(LS_INFO, this) << "Ignoring identical certificate"; Just use INFO (like you do ERROR) with no "LS_". https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:168: LOG_J(ERROR, this) << "Can't change local certificate in this state"; This could be a little more details, perhaps something like "Can't change the local certificate of a QUIC connection once it's active." https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:176: LOG_J(LS_INFO, this) << "NULL identity supplied. Not doing QUIC."; Should this be WARN instead of INFO? And should it return false? https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:178: return true; The whole method could be rearranged a bit to be more early-return-friendly: if (!certificate) { LOG ... return false; } if (!local_certificate_) { local_certificate_ = certificate; return true; } if (local_certificate_ == certificate) { LOG ... return true; } LOG ... return false; https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:197: return true; Same here with the early returns: if (ssl_role_ == role) { LOG ...; return true; } if (quic_state() != QUIC_TRANSPORT_CONNECTED) { ssl_role_ = role; return true; } LOG ...; return false; https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:214: !digest_alg.empty()) { Wouldn't we ignore an identical remote_fingerprint_value even if !quic_active_? And don't we have to check that the algorithms are equal? https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:227: } I think it would make sense to make this one first. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:265: return channel_->SendPacket(data, size, options); I think we can remove this. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:274: return -1; If it's SRTP_BYPASS, we can, can't we? I think this whole method could just be: if (flags & PF_SRTP_BYPASS && IsRtpPacket(data, size)) { return channel_->SendPacket(data, size, options); } LOG(LS_ERROR) << "..."; return -1; https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:312: set_writable(channel_->writable()); I think we can remove this. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:337: } Should we do this for QUIC_TRANSPORT_CONNECTED also? https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:351: if (!quic_active_ || quic_state() == QUIC_TRANSPORT_CONNECTED) { I think we can remove the !quic_active_ part of this. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:370: } I think we can remove this. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:402: } We can probably just pass RTP packet through even if the handshake isn't complete. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:405: LOG_J(ERROR, this) << "Received unexpected non-QUIC packet."; non-QUIC, non-RTP packet :). https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:458: if (!channel_->writable()) { That seems a bit drastic. Just because we're not writable now doesn't mean we won't be in the future. I don't think we need to jump to a terminal state because of that. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:463: if (!CreateQuicSession() || !StartQuicHandshake()) { This could use a LOG_J(ERROR) or LOG_J(WARN). https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:468: RTC_DCHECK(quic_->connection()->connected()); It seems like it would be a good idea to just log it and return false rather than crashing in debug builds. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:555: return net::WriteResult(net::WRITE_STATUS_BLOCKED, error); There error is only blocking if IsBlockingError(error) is true (see webrtc/base/socket.h). However, I'm not sure if we should tear down the whole thing because of a transient error of any other sort. Any idea what the QUIC library does when a normal socket gives an error other than those in IsBlockingError? https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... File webrtc/p2p/quic/quictransportchannel.h (right): https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:84: bool IsDtlsActive() const override { return quic_active_; } Can we just make this return true? Unlike DTLS, I see no reason to construct a QuicTransportChannel only to disable it (we don't have a need for SDES when using QUIC). https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:273: bool quic_active_ = false; Instead of having a separate bool, I'd suggest using simply the fact that local_certificate_ is set. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:275: bool set_remote_fingerprint_ = false; I'd suggest making a RemoteFingerprint struct that has algorithm + value (if there isn't already one) and then using Optional<RemoteFingerprint> rather than 3 separate fields. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:277: bool set_ssl_role_ = false; Same here: Optional<rtc::SSLRole> instead of two separate fields.
https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... File webrtc/p2p/quic/quictransportchannel.cc (right): https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:44: const int kIdleConnectionStateLifetime = 1000; On 2016/02/25 08:24:25, pthatcher1 wrote: > Is this in ms? If so, I don't think 1000 is long enough. With ICE, you could > see 10-15 seconds of a disconnected state and then come back. Seconds. This is roughly 16.7 minutes. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:47: const size_t kInputKeyingMaterialLength = 32; On 2016/02/25 08:24:25, pthatcher1 wrote: > Where does this come from? I added the link where HKDF input keying material is described: https://tools.ietf.org/html/rfc5869#section-2.2. I made up this value because they only specify that input keying material is for generating a pseudorandom key, but not how random it needs to be. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:68: class InsecureProofSource : public net::ProofSource { On 2016/02/25 08:24:24, pthatcher1 wrote: > Might as well call this DummyProofSource Done. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:83: certs->push_back("Required to establish handshake"); On 2016/02/25 08:24:24, pthatcher1 wrote: > And say "Dummy cert" Done. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:84: std::string signature("Signature"); On 2016/02/25 08:24:25, pthatcher1 wrote: > And "Dummy signature" Done. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:149: config_.SetBytesForConnectionIdToSend(0); On 2016/02/25 08:24:24, pthatcher1 wrote: > What's this? Added a comment. This should set the bytes reserved for the QUIC connection ID to zero, which is done since connection IDs are ignored. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:165: LOG_J(LS_INFO, this) << "Ignoring identical certificate"; On 2016/02/25 08:24:24, pthatcher1 wrote: > Just use INFO (like you do ERROR) with no "LS_". Done. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:168: LOG_J(ERROR, this) << "Can't change local certificate in this state"; On 2016/02/25 08:24:24, pthatcher1 wrote: > This could be a little more details, perhaps something like "Can't change the > local certificate of a QUIC connection once it's active." Done. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:176: LOG_J(LS_INFO, this) << "NULL identity supplied. Not doing QUIC."; On 2016/02/25 08:24:25, pthatcher1 wrote: > Should this be WARN instead of INFO? > > And should it return false? Yes it should. I think ERROR seems more appropriate assuming a local certificate is required for QUIC. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:178: return true; On 2016/02/25 08:24:25, pthatcher1 wrote: > The whole method could be rearranged a bit to be more early-return-friendly: > > if (!certificate) { > LOG ... > return false; > } > if (!local_certificate_) { > local_certificate_ = certificate; > return true; > } > if (local_certificate_ == certificate) { > LOG ... > return true; > } > LOG ... > return false; Done. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:197: return true; On 2016/02/25 08:24:24, pthatcher1 wrote: > Same here with the early returns: > > > if (ssl_role_ == role) { > LOG ...; > return true; > } > if (quic_state() != QUIC_TRANSPORT_CONNECTED) { > ssl_role_ = role; > return true; > } > LOG ...; > return false; Done. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:214: !digest_alg.empty()) { On 2016/02/25 08:24:24, pthatcher1 wrote: > Wouldn't we ignore an identical remote_fingerprint_value even if !quic_active_? > > And don't we have to check that the algorithms are equal? Yes. Fixed now. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:227: } On 2016/02/25 08:24:25, pthatcher1 wrote: > I think it would make sense to make this one first. Done. Also I think this should log an error and return false now. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:265: return channel_->SendPacket(data, size, options); On 2016/02/25 08:24:25, pthatcher1 wrote: > I think we can remove this. Done. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:274: return -1; On 2016/02/25 08:24:25, pthatcher1 wrote: > If it's SRTP_BYPASS, we can, can't we? > > I think this whole method could just be: > > if (flags & PF_SRTP_BYPASS && IsRtpPacket(data, size)) { > return channel_->SendPacket(data, size, options); > } > LOG(LS_ERROR) << "..."; > return -1; Done. That seems reasonable since SRTP is always encrypted. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:312: set_writable(channel_->writable()); On 2016/02/25 08:24:25, pthatcher1 wrote: > I think we can remove this. I removed it. So the QUIC channel is not writable until the handshake is complete. When the channel first becomes writable it will call MaybeStartQuic(). https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:337: } On 2016/02/25 08:24:25, pthatcher1 wrote: > Should we do this for QUIC_TRANSPORT_CONNECTED also? Yes. Fixed this. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:351: if (!quic_active_ || quic_state() == QUIC_TRANSPORT_CONNECTED) { On 2016/02/25 08:24:25, pthatcher1 wrote: > I think we can remove the !quic_active_ part of this. Done. !quic_active_ would essentially be false since we know we're doing QUIC. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:370: } On 2016/02/25 08:24:24, pthatcher1 wrote: > I think we can remove this. Removed. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:402: } On 2016/02/25 08:24:24, pthatcher1 wrote: > We can probably just pass RTP packet through even if the handshake isn't > complete. True. Removed the extra if statement. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:405: LOG_J(ERROR, this) << "Received unexpected non-QUIC packet."; On 2016/02/25 08:24:24, pthatcher1 wrote: > non-QUIC, non-RTP packet :). Done. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:458: if (!channel_->writable()) { On 2016/02/25 08:24:25, pthatcher1 wrote: > That seems a bit drastic. Just because we're not writable now doesn't mean we > won't be in the future. I don't think we need to jump to a terminal state > because of that. Seems fine. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:463: if (!CreateQuicSession() || !StartQuicHandshake()) { On 2016/02/25 08:24:25, pthatcher1 wrote: > This could use a LOG_J(ERROR) or LOG_J(WARN). Done. I agree. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:468: RTC_DCHECK(quic_->connection()->connected()); On 2016/02/25 08:24:25, pthatcher1 wrote: > It seems like it would be a good idea to just log it and return false rather > than crashing in debug builds. Ok I'll do that so QUIC bugs don't crash everything. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:555: return net::WriteResult(net::WRITE_STATUS_BLOCKED, error); On 2016/02/25 08:24:24, pthatcher1 wrote: > There error is only blocking if IsBlockingError(error) is true (see > webrtc/base/socket.h). > > However, I'm not sure if we should tear down the whole thing because of a > transient error of any other sort. > > Any idea what the QUIC library does when a normal socket gives an error other > than those in IsBlockingError? I believe net::WRITE_STATUS_ERROR was closing the QUIC connection. I will provide an update regarding this. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... File webrtc/p2p/quic/quictransportchannel.h (right): https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:30: // Negotiated, and has an ecrypted connection. On 2016/02/24 23:31:13, pthatcher1 wrote: > encrypted Done. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:51: // PF_SRTP_BYPASS flag after the QUIC handshake. On 2016/02/24 23:31:13, pthatcher1 wrote: > I understand why we pass SRTP packets directly, but why would we pass packets > directly before the handshake has begun? I am removing this behavior. I only did that because DtlsTransportChannelWrapper allows packets to be passed before the handshake, but now I see that's for when DTLS is turned off. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:70: explicit QuicTransportChannel(TransportChannelImpl* channel); On 2016/02/24 23:31:13, pthatcher1 wrote: > Why does this need a TransportChannelImpl instead of just a TransportChannel? QuicTransportChannel uses |channel| to implement ICE channel methods (e.g. SetIceRole(...), SetIceCredentials(...), AddRemoteCandidate(..), ...) which are part of TransportChannelImpl. Would it be better if QuicTransportChannel implemented TransportChannel instead? TransportChannelImpl seems specific to Transport and TransportController anyway, but my plan is to use QuicTransportChannel with TransportController. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:83: // TODO(mikescarlett): Method name is DTLS-specific. On 2016/02/24 23:31:13, pthatcher1 wrote: > Can you change the TODOs to be action to take, such as "Rename this so it's not > DTLS-specific", or "Implement certification authentication."? Done. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:84: bool IsDtlsActive() const override { return quic_active_; } On 2016/02/25 08:24:25, pthatcher1 wrote: > Can we just make this return true? Unlike DTLS, I see no reason to construct a > QuicTransportChannel only to disable it (we don't have a need for SDES when > using QUIC). Done. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:145: bool GetSrtpCryptoSuite(int* cipher) override; On 2016/02/24 23:31:13, pthatcher1 wrote: > In voice-only calls, we tend to use SRTP_AES128_CM_SHA1_32 instead. Perhaps we > need to add our own layer of negotiation on top of QUIC. That's definitely a > big TODO. Agreed. A hacky solution (which isn't exactly negotiation) might be adding the cipher as a key-value within the QuicCryptoServerConfig, so that the server peer can send it to the client peer within the hello. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:152: // use by the remote peer, for use in external identity verification. On 2016/02/24 23:31:13, pthatcher1 wrote: > This is really just used for stats, so we should figure out which stats make > sense for QUIC. Ok. In that case I'll ignore this method and move it to private. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:273: bool quic_active_ = false; On 2016/02/25 08:24:25, pthatcher1 wrote: > Instead of having a separate bool, I'd suggest using simply the fact that > local_certificate_ is set. Done. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:275: bool set_remote_fingerprint_ = false; On 2016/02/25 08:24:25, pthatcher1 wrote: > I'd suggest making a RemoteFingerprint struct that has algorithm + value (if > there isn't already one) and then using Optional<RemoteFingerprint> rather than > 3 separate fields. Done. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:277: bool set_ssl_role_ = false; On 2016/02/25 08:24:25, pthatcher1 wrote: > Same here: Optional<rtc::SSLRole> instead of two separate fields. Done.
https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... File webrtc/p2p/quic/quictransportchannel.cc (right): https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:555: return net::WriteResult(net::WRITE_STATUS_BLOCKED, error); On 2016/02/26 00:54:13, mikescarlett wrote: > On 2016/02/25 08:24:24, pthatcher1 wrote: > > There error is only blocking if IsBlockingError(error) is true (see > > webrtc/base/socket.h). > > > > However, I'm not sure if we should tear down the whole thing because of a > > transient error of any other sort. > > > > Any idea what the QUIC library does when a normal socket gives an error other > > than those in IsBlockingError? > > I believe net::WRITE_STATUS_ERROR was closing the QUIC connection. I will > provide an update regarding this. Strangely the QuicSession gets torn down on net::WRITE_STATUS_ERROR (by calling QuicConnection::OnError(...)). Everything would have to be reinitialized because closed connections cannot be reopened.
OK, I just need to look at the unit tests now. Everything else is looking pretty good. https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... File webrtc/p2p/quic/quictransportchannel.cc (right): https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.cc:44: const int kIdleConnectionStateLifetime = 1000; On 2016/02/26 00:54:13, mikescarlett wrote: > On 2016/02/25 08:24:25, pthatcher1 wrote: > > Is this in ms? If so, I don't think 1000 is long enough. With ICE, you could > > see 10-15 seconds of a disconnected state and then come back. > > Seconds. This is roughly 16.7 minutes. Ah... that's long enough :). But can you put a comment? ... = 1000; // seconds https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... File webrtc/p2p/quic/quictransportchannel.h (right): https://codereview.webrtc.org/1721673004/diff/1/webrtc/p2p/quic/quictransport... webrtc/p2p/quic/quictransportchannel.h:70: explicit QuicTransportChannel(TransportChannelImpl* channel); On 2016/02/26 00:54:14, mikescarlett wrote: > On 2016/02/24 23:31:13, pthatcher1 wrote: > > Why does this need a TransportChannelImpl instead of just a TransportChannel? > > QuicTransportChannel uses |channel| to implement ICE channel methods (e.g. > SetIceRole(...), SetIceCredentials(...), AddRemoteCandidate(..), ...) which are > part of TransportChannelImpl. > > Would it be better if QuicTransportChannel implemented TransportChannel instead? > TransportChannelImpl seems specific to Transport and TransportController anyway, > but my plan is to use QuicTransportChannel with TransportController. OK, that seems fine. https://codereview.webrtc.org/1721673004/diff/60001/webrtc/p2p/quic/quictrans... File webrtc/p2p/quic/quictransportchannel.cc (right): https://codereview.webrtc.org/1721673004/diff/60001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel.cc:500: return net::WriteResult(net::WRITE_STATUS_BLOCKED, error); I think we might need to do: // QUIC should never call this if IsWriteBlocked, but just in case... if (IsWriteBlocked()) { return net::WriteResult(net::WRITE_STATUS_BLOCKED, EWOULDBLOCK); } // TODO: Figure out how to tell QUIC "I dropped your packet, but don't block". int sent = channel_->SendPacket(buffer, buf_len, rtc::PacketOptions()); return net::WriteResult(net::WRITE_STATUS_OK, sent); Because the channel_ won't signal writable just because of one failed send. https://codereview.webrtc.org/1721673004/diff/60001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel.cc:519: // // be reset. // // => //
https://codereview.chromium.org/1721673004/diff/1/webrtc/p2p/quic/quictranspo... File webrtc/p2p/quic/quictransportchannel.cc (right): https://codereview.chromium.org/1721673004/diff/1/webrtc/p2p/quic/quictranspo... webrtc/p2p/quic/quictransportchannel.cc:44: const int kIdleConnectionStateLifetime = 1000; On 2016/02/27 01:14:19, pthatcher1 wrote: > On 2016/02/26 00:54:13, mikescarlett wrote: > > On 2016/02/25 08:24:25, pthatcher1 wrote: > > > Is this in ms? If so, I don't think 1000 is long enough. With ICE, you > could > > > see 10-15 seconds of a disconnected state and then come back. > > > > Seconds. This is roughly 16.7 minutes. > > Ah... that's long enough :). > > But can you put a comment? > > ... = 1000; // seconds Done. https://codereview.chromium.org/1721673004/diff/60001/webrtc/p2p/quic/quictra... File webrtc/p2p/quic/quictransportchannel.cc (right): https://codereview.chromium.org/1721673004/diff/60001/webrtc/p2p/quic/quictra... webrtc/p2p/quic/quictransportchannel.cc:500: return net::WriteResult(net::WRITE_STATUS_BLOCKED, error); On 2016/02/27 01:14:19, pthatcher1 wrote: > I think we might need to do: > > // QUIC should never call this if IsWriteBlocked, but just in case... > if (IsWriteBlocked()) { > return net::WriteResult(net::WRITE_STATUS_BLOCKED, EWOULDBLOCK); > } > // TODO: Figure out how to tell QUIC "I dropped your packet, but don't block". > int sent = channel_->SendPacket(buffer, buf_len, rtc::PacketOptions()); > return net::WriteResult(net::WRITE_STATUS_OK, sent); > > Because the channel_ won't signal writable just because of one failed send. Replaced with your suggestion. https://codereview.chromium.org/1721673004/diff/60001/webrtc/p2p/quic/quictra... webrtc/p2p/quic/quictransportchannel.cc:519: // // be reset. On 2016/02/27 01:14:19, pthatcher1 wrote: > // // => // Done.
Just style stuff now. Most of these I don't feel that strongly about, since it's mostly unit test code, so if you feel strongly the other way, feel free to push back. https://codereview.webrtc.org/1721673004/diff/60001/webrtc/p2p/quic/quictrans... File webrtc/p2p/quic/quictransportchannel_unittest.cc (right): https://codereview.webrtc.org/1721673004/diff/60001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:71: class FakeTransportChannel : public cricket::FakeTransportChannel { Can you give it a different name to avoid confustion? Perhaps FailableTransportChannel? https://codereview.webrtc.org/1721673004/diff/60001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:120: void NegotiateBeforeQuic(QuicTestPeer* peer, It might be more clear to call it "remote_peer" https://codereview.webrtc.org/1721673004/diff/60001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:121: IceRole ice_role, Might as well call it local_ice_role to be clear. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... File webrtc/p2p/quic/quictransportchannel_unittest.cc (right): https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:123: rtc::SSLRole remote_ssl_role) { Should this be called SetIceAndCryptoParameters? Something like that would be more clear. Actually, having SetIceParameters and SetCryptoParameters separately might be better. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:150: void SetIceCredentials(IceRole ice_role, Since this is only called once, I'd suggest just inlining the code into the call site. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:200: void SendPackets(size_t count, bool srtp, bool expect_success) { This method is only called once, so it seems like it would be better either just put it inline where it's called, or to have a different level of abstraction, such as: bool SendRtpPacket(bool srtp); And then the call site can handle the ASSERT and the loop. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:222: int SendInvalidSrtpPacket() { Ah, well now it looks like you should have three methods: SendSrtpPacket() SendInvalidSrtpPacket() SendRtpPacket() https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:236: bool handshake_confirmed() const { Would a more correct name be quic_connected()? https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:250: FakeTransportChannel* fake_channel() { return &fake_channel_; } I would call this ice_channel(), even though it's fake. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:304: } You have 4 methods that basically do a given operation on both peers. I think it might be worth it to just inline that code into the call sites. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:307: void TestTransfer(size_t count, bool srtp, bool expect_success) { I'd suggest to just inline this code into the tests. Duplicating code in tests doesn't have the same downsides as with non-test code, and it often makes the tests easier to read, and the failure messages easier to understand. Plus, only see one place for where it's called for each of (true, false), so at most you'd want to pull the clear/clear/send into a method like ClearAndSendRtpPackets, but even that's probably not worth it. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:330: void TestExportKeyingMaterial() { Same here: Just put this in the test case. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:430: // Test that QuicTransportChannel::WritePacket blocks when underlying channel when underlying => when the underlying
https://codereview.webrtc.org/1721673004/diff/60001/webrtc/p2p/quic/quictrans... File webrtc/p2p/quic/quictransportchannel_unittest.cc (right): https://codereview.webrtc.org/1721673004/diff/60001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:71: class FakeTransportChannel : public cricket::FakeTransportChannel { On 2016/03/01 22:46:52, pthatcher1 wrote: > Can you give it a different name to avoid confustion? Perhaps > FailableTransportChannel? Done. https://codereview.webrtc.org/1721673004/diff/60001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:120: void NegotiateBeforeQuic(QuicTestPeer* peer, On 2016/03/01 22:46:52, pthatcher1 wrote: > It might be more clear to call it "remote_peer" Done. https://codereview.webrtc.org/1721673004/diff/60001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:121: IceRole ice_role, On 2016/03/01 22:46:52, pthatcher1 wrote: > Might as well call it local_ice_role to be clear. Done. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... File webrtc/p2p/quic/quictransportchannel_unittest.cc (right): https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:123: rtc::SSLRole remote_ssl_role) { On 2016/03/01 22:46:52, pthatcher1 wrote: > Should this be called SetIceAndCryptoParameters? Something like that would be > more clear. > > Actually, having SetIceParameters and SetCryptoParameters separately might be > better. I moved the crypto-related stuff outside and put ICE-related stuff in SetIceParameters. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:150: void SetIceCredentials(IceRole ice_role, On 2016/03/01 22:46:52, pthatcher1 wrote: > Since this is only called once, I'd suggest just inlining the code into the call > site. I merged them into SetIceParameters. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:200: void SendPackets(size_t count, bool srtp, bool expect_success) { On 2016/03/01 22:46:52, pthatcher1 wrote: > This method is only called once, so it seems like it would be better either just > put it inline where it's called, or to have a different level of abstraction, > such as: > > bool SendRtpPacket(bool srtp); > > And then the call site can handle the ASSERT and the loop. Done. Split this and moved asserts like you suggested. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:222: int SendInvalidSrtpPacket() { On 2016/03/01 22:46:52, pthatcher1 wrote: > Ah, well now it looks like you should have three methods: > > SendSrtpPacket() > SendInvalidSrtpPacket() > SendRtpPacket() Done. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:236: bool handshake_confirmed() const { On 2016/03/01 22:46:52, pthatcher1 wrote: > Would a more correct name be quic_connected()? That's fine. Done. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:250: FakeTransportChannel* fake_channel() { return &fake_channel_; } On 2016/03/01 22:46:52, pthatcher1 wrote: > I would call this ice_channel(), even though it's fake. Done. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:304: } On 2016/03/01 22:46:52, pthatcher1 wrote: > You have 4 methods that basically do a given operation on both peers. I think > it might be worth it to just inline that code into the call sites. I merged this into one method and will revisit whether more should be inlined. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:307: void TestTransfer(size_t count, bool srtp, bool expect_success) { On 2016/03/01 22:46:52, pthatcher1 wrote: > I'd suggest to just inline this code into the tests. Duplicating code in tests > doesn't have the same downsides as with non-test code, and it often makes the > tests easier to read, and the failure messages easier to understand. Plus, only > see one place for where it's called for each of (true, false), so at most you'd > want to pull the clear/clear/send into a method like ClearAndSendRtpPackets, but > even that's probably not worth it. Inlined. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:330: void TestExportKeyingMaterial() { On 2016/03/01 22:46:52, pthatcher1 wrote: > Same here: Just put this in the test case. Inlined. https://codereview.webrtc.org/1721673004/diff/80001/webrtc/p2p/quic/quictrans... webrtc/p2p/quic/quictransportchannel_unittest.cc:430: // Test that QuicTransportChannel::WritePacket blocks when underlying channel On 2016/03/01 22:46:52, pthatcher1 wrote: > when underlying => when the underlying Done.
pthatcher@webrtc.org changed reviewers: + honghaiz@webrtc.org - deadbeef@webrtc.org
lgtm, with nits Honghai, can you also take a look? https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... File webrtc/p2p/quic/quictransportchannel_unittest.cc (right): https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel_unittest.cc:229: FailableTransportChannel fake_channel_; // Simulates an ICE channel. Might as well make it ice_channel_ here as well. https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel_unittest.cc:255: rtc::SSLRole peer2_ssl_role) { Can you rename this SetIceAndCryptoParameters()?
https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... File webrtc/p2p/quic/quictransportchannel.cc (right): https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.cc:524: set_quic_state(QUIC_TRANSPORT_FAILED); Is this always a failed state? Could it be just that the session ends? https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... File webrtc/p2p/quic/quictransportchannel.h (right): https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.h:85: // Sends a RTP packet is sent with the PF_SRTP_BYPASS flag. is ==> if? https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.h:91: // TransportChannel calls that we forward to the wrapped transport. TransportChannelImpl calls https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.h:203: // QuicPacketWriter override. It is strange to change the permission of a virtual method from public to private. If you don't want it to be called, you can put an assert error there. https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... File webrtc/p2p/quic/quictransportchannel_unittest.cc (right): https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel_unittest.cc:30: const int kTimeoutMs = 1000; Mark all const and helper methods as static or put them in an unanonymous namespace (the former is used more commonly in other files).
https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... File webrtc/p2p/quic/quictransportchannel.cc (right): https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.cc:442: // Performs authentication of remote peer; owned by QuicCryptoClientConfig. Performs=>Perform. Only add "s" when this is a comment for a method or class. Same for the other places.
https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... File webrtc/p2p/quic/quictransportchannel.cc (right): https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.cc:442: // Performs authentication of remote peer; owned by QuicCryptoClientConfig. On 2016/03/03 01:35:58, honghaiz3 wrote: > Performs=>Perform. Only add "s" when this is a comment for a method or class. > Same for the other places. Ok seems better. https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.cc:524: set_quic_state(QUIC_TRANSPORT_FAILED); On 2016/03/02 22:47:45, honghaiz3 wrote: > Is this always a failed state? Could it be just that the session ends? I renamed this to QUIC_TRANSPORT_CLOSED. It isn't failure if the QUIC peer tells the other peer to shutdown or reset the connection. https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... File webrtc/p2p/quic/quictransportchannel.h (right): https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.h:85: // Sends a RTP packet is sent with the PF_SRTP_BYPASS flag. On 2016/03/02 22:47:45, honghaiz3 wrote: > is ==> if? Correct. Fixed. https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.h:91: // TransportChannel calls that we forward to the wrapped transport. On 2016/03/02 22:47:45, honghaiz3 wrote: > TransportChannelImpl calls Fixed. https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.h:203: // QuicPacketWriter override. On 2016/03/02 22:47:45, honghaiz3 wrote: > It is strange to change the permission of a virtual method from public to > private. If you don't want it to be called, you can put an assert error there. I made these public since modifying the permission seems to be distracting. I'm confused whether asserting errors is correct since I don't interpret these as errors (in terms of the program failing) and think that people should not call these if they are unimplemented. https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... File webrtc/p2p/quic/quictransportchannel_unittest.cc (right): https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel_unittest.cc:30: const int kTimeoutMs = 1000; On 2016/03/02 22:47:45, honghaiz3 wrote: > Mark all const and helper methods as static or put them in an unanonymous > namespace (the former is used more commonly in other files). Done. https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel_unittest.cc:229: FailableTransportChannel fake_channel_; // Simulates an ICE channel. On 2016/03/02 19:48:13, pthatcher1 wrote: > Might as well make it ice_channel_ here as well. Done. https://codereview.webrtc.org/1721673004/diff/100001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel_unittest.cc:255: rtc::SSLRole peer2_ssl_role) { On 2016/03/02 19:48:13, pthatcher1 wrote: > Can you rename this SetIceAndCryptoParameters()? Done.
Mostly looking good. https://codereview.webrtc.org/1721673004/diff/120001/webrtc/p2p/quic/quictran... File webrtc/p2p/quic/quictransportchannel.cc (right): https://codereview.webrtc.org/1721673004/diff/120001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.cc:281: // Starts the QUIC handshake when |channel_| is writable. Starts -> Start https://codereview.webrtc.org/1721673004/diff/120001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.cc:295: // might merge the next line to this line. https://codereview.webrtc.org/1721673004/diff/120001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.cc:315: set_receiving(channel_->receiving()); If the transportchannel becomes receiving before the quic state_ changes to connected, we need to set the receiving state when quic becomes connected. https://codereview.webrtc.org/1721673004/diff/120001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.cc:430: quic_.reset(new QuicSession(std::move(connection), config_)); Is it enough just to connection->release() instead of std::move? Or maybe just the raw pointer for connection since you are taking the ownership right away?
https://codereview.webrtc.org/1721673004/diff/120001/webrtc/p2p/quic/quictran... File webrtc/p2p/quic/quictransportchannel.cc (right): https://codereview.webrtc.org/1721673004/diff/120001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.cc:281: // Starts the QUIC handshake when |channel_| is writable. On 2016/03/03 19:16:56, honghaiz3 wrote: > Starts -> Start Done. https://codereview.webrtc.org/1721673004/diff/120001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.cc:295: // might On 2016/03/03 19:16:56, honghaiz3 wrote: > merge the next line to this line. Done. https://codereview.webrtc.org/1721673004/diff/120001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.cc:315: set_receiving(channel_->receiving()); On 2016/03/03 19:16:56, honghaiz3 wrote: > If the transportchannel becomes receiving before the quic state_ changes to > connected, we need to set the receiving state when quic becomes connected. Thanks for catching this. I modified OnHandshakeComplete (which gets called when QUIC is connected) to call set_receiving(true) if |channel_| is receiving. I also added unit tests. https://codereview.webrtc.org/1721673004/diff/120001/webrtc/p2p/quic/quictran... webrtc/p2p/quic/quictransportchannel.cc:430: quic_.reset(new QuicSession(std::move(connection), config_)); On 2016/03/03 19:16:56, honghaiz3 wrote: > Is it enough just to connection->release() instead of std::move? > Or maybe just the raw pointer for connection since you are taking the ownership > right away? I made cricket::QuicSession require a scoped_ptr<QuicConnection> instead of a raw pointer to make the ownership explicit. I'm not changing it because connection->release() returns a raw pointer and doesn't compile.
lgtm
The CQ bit was checked by mikescarlett@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, honghaiz@webrtc.org Link to the patchset: https://codereview.webrtc.org/1721673004/#ps160001 (title: "Update documentation before committing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721673004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721673004/160001
The CQ bit was unchecked by mikescarlett@webrtc.org
The CQ bit was checked by mikescarlett@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721673004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721673004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL) win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by mikescarlett@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1721673004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1721673004/160001
Message was sent while issue was closed.
Description was changed from ========== Create QuicTransportChannel This new class allows usage of a QuicSession to establish a QUIC handshake. BUG= ========== to ========== Create QuicTransportChannel This new class allows usage of a QuicSession to establish a QUIC handshake. BUG= ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Create QuicTransportChannel This new class allows usage of a QuicSession to establish a QUIC handshake. BUG= ========== to ========== Create QuicTransportChannel This new class allows usage of a QuicSession to establish a QUIC handshake. BUG= Committed: https://crrev.com/6459f84766dd6ece67e70c8743aeb2378ac267be Cr-Commit-Position: refs/heads/master@{#11873} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/6459f84766dd6ece67e70c8743aeb2378ac267be Cr-Commit-Position: refs/heads/master@{#11873} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
