Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2416023002:
Introduce rtc::PacketTransportInterface and let cricket::TransportChannel inherit. (Closed)
DescriptionIntroduce rtc::PacketTransportInterface and let cricket::TransportChannel inherit.
Introduce rtc::PacketTransportInterface. Refactor cricket::TransportChannel.
Fix signal slots parameter types in all related code.
BUG=webrtc:6531
Committed: https://crrev.com/d89ab145cd07c052664ed24873878335652a6e36
Cr-Commit-Position: refs/heads/master@{#14778}
Patch Set 1 #Patch Set 2 : Resolve unused variable issue in release build. #
Total comments: 22
Patch Set 3 : Rename rtc::PacketTransport to rtc::PacketTransportInterface, #Patch Set 4 : Let BaseChannel::PacketIsRtcp() use PacketTransportInterface. #
Total comments: 6
Patch Set 5 : Fix nits (names and todo text). #Patch Set 6 : Remove static_casts for rtc::PacketTransportInterface or cricket::TransportChannel. #Patch Set 7 : Rebase. #
Total comments: 20
Patch Set 8 : Style and comments. #Patch Set 9 : #include and forward declaration for chromium integration. #Patch Set 10 : Rebase. #
Messages
Total messages: 51 (33 generated)
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/17753) linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...) mac_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/15181)
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
johan@webrtc.org changed reviewers: + deadbeef@webrtc.org, pthatcher@webrtc.org
Creating a PacketTransportInterface as discussed in https://codereview.webrtc.org/2377883003/ . Only creating the PacketTransportInterface and resolving follow up issues in webrtc so far. Would be good to check if this CL works towards the right direction.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9188)
This is the right direction. Just needs a few modifications. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.cc:442: TransportChannel* channel = static_cast<TransportChannel*>(pt); The only thing this method needs from pt is ->writable(), which is part of the interface. So why do we need the type cast? I guess there's the RTC_DCHECK, but that's just a pointer compare, and we could move the cast to just that line if the compiler requires it. But it might be better to change the member variable "TransportChannel* channel_" into "PacketTransportInterface* transport_". Then the check would be transport_ == transport, which wouldn't need the type cast. I only see one place where we need anything from channel_ that we wouldn't get from transport_, and that's the RTC_DCHECK in OnReceivingState, which is the same as this RTC_DCHECK except in reverse. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel.h (right): https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.h:218: void OnReadyToSend(rtc::PacketTransport* pt); Can you call these "transport" instead of "pt"? "pt" sounds like "payload type" to me. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/packettra... File webrtc/p2p/base/packettransport.h (right): https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/packettra... webrtc/p2p/base/packettransport.h:22: } This shouldn't be in PacketTransport because it's ICE-specific. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/packettra... webrtc/p2p/base/packettransport.h:28: struct PacketTransport : public sigslot::has_slots<> { Can you change the name to PacketTransportInterface? And change the name of this file to packettransportinterface.h? https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/packettra... webrtc/p2p/base/packettransport.h:30: virtual const std::string& transport_name() const = 0; I'm not sure this is needed in PacketTransport. I'm guessing it isn't. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/packettra... webrtc/p2p/base/packettransport.h:31: virtual int component() const = 0; This shouldn't be in PacketTransport because it's ICE-specific. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/packettra... webrtc/p2p/base/packettransport.h:48: sigslot::signal2<PacketTransport*, const rtc::SentPacket&> SignalSentPacket; Can you put all the signals after all the methods? https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/packettra... webrtc/p2p/base/packettransport.h:69: virtual bool GetStats(cricket::ConnectionInfos* infos) = 0; This shouldn't be in PacketTransport because it's ICE-specific. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.cc (right): https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.cc:572: << pt->writable() << "."; We probably want to make this less ICE-specific, with something like: LOG(LS_INFO) << "Transport " << transport->debug_name() << " writability changed to " << transport->writable(); Then a TransportChannel can make debug_name() return a transport_name() + component(). https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.h (right): https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.h:208: void OnChannelWritableState_n(rtc::PacketTransport* pt); This could probably still be a TransportChannel*. All the others are, and the method is "OnChannelX". https://codereview.webrtc.org/2416023002/diff/20001/webrtc/pc/channel.h File webrtc/pc/channel.h (right): https://codereview.webrtc.org/2416023002/diff/20001/webrtc/pc/channel.h#newco... webrtc/pc/channel.h:245: int flags); OnChannelRead should probably be renamed to OnPacketRead.
Description was changed from ========== Introduce rtc::PacketTransport interface and let cricket::TransportChannel inherit. Introduce rtc::PacketTransport. Refactor cricket::TransportChannel. Fix signal slots parameter types in all related code. BUG= ========== to ========== Introduce rtc::PacketTransport interface and let cricket::TransportChannel inherit. Introduce rtc::PacketTransport. Refactor cricket::TransportChannel. Fix signal slots parameter types in all related code. BUG=webrtc:6531 ==========
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel.cc (right): https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.cc:442: TransportChannel* channel = static_cast<TransportChannel*>(pt); On 2016/10/13 20:26:37, pthatcher1 wrote: > The only thing this method needs from pt is ->writable(), which is part of the > interface. So why do we need the type cast? I guess there's the RTC_DCHECK, > but that's just a pointer compare, and we could move the cast to just that line > if the compiler requires it. Done. > > But it might be better to change the member variable "TransportChannel* > channel_" into "PacketTransportInterface* transport_". Then the check would be > transport_ == transport, which wouldn't need the type cast. > > I only see one place where we need anything from channel_ that we wouldn't get > from transport_, and that's the RTC_DCHECK in OnReceivingState, which is the > same as this RTC_DCHECK except in reverse. In the methods above are several lines of forwarding TransportChannel's signals to subscribers of DtlsTransportChannel. That code won't compile, if we replace the member variable channel_ by transport_. What are the plans are for this signal-fowarding code, anyway? https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel.h (right): https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.h:218: void OnReadyToSend(rtc::PacketTransport* pt); On 2016/10/13 20:26:37, pthatcher1 wrote: > Can you call these "transport" instead of "pt"? "pt" sounds like "payload type" > to me. Done. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/packettra... File webrtc/p2p/base/packettransport.h (right): https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/packettra... webrtc/p2p/base/packettransport.h:22: } On 2016/10/13 20:26:37, pthatcher1 wrote: > This shouldn't be in PacketTransport because it's ICE-specific. Done. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/packettra... webrtc/p2p/base/packettransport.h:28: struct PacketTransport : public sigslot::has_slots<> { On 2016/10/13 20:26:37, pthatcher1 wrote: > Can you change the name to PacketTransportInterface? And change the name of > this file to packettransportinterface.h? Done. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/packettra... webrtc/p2p/base/packettransport.h:30: virtual const std::string& transport_name() const = 0; On 2016/10/13 20:26:37, pthatcher1 wrote: > I'm not sure this is needed in PacketTransport. I'm guessing it isn't. Probably not. Removing it for now. It is used in channel.cc. But that use case is TransportController interaction and looks ICE specific to me. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/packettra... webrtc/p2p/base/packettransport.h:31: virtual int component() const = 0; On 2016/10/13 20:26:37, pthatcher1 wrote: > This shouldn't be in PacketTransport because it's ICE-specific. Acknowledged. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/packettra... webrtc/p2p/base/packettransport.h:48: sigslot::signal2<PacketTransport*, const rtc::SentPacket&> SignalSentPacket; On 2016/10/13 20:26:37, pthatcher1 wrote: > Can you put all the signals after all the methods? Done. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/packettra... webrtc/p2p/base/packettransport.h:69: virtual bool GetStats(cricket::ConnectionInfos* infos) = 0; On 2016/10/13 20:26:37, pthatcher1 wrote: > This shouldn't be in PacketTransport because it's ICE-specific. Done. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.cc (right): https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.cc:572: << pt->writable() << "."; On 2016/10/13 20:26:37, pthatcher1 wrote: > We probably want to make this less ICE-specific, with something like: > > LOG(LS_INFO) << "Transport " << transport->debug_name() << " writability changed > to " << transport->writable(); > > Then a TransportChannel can make debug_name() return a transport_name() + > component(). Done. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.h (right): https://codereview.webrtc.org/2416023002/diff/20001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.h:208: void OnChannelWritableState_n(rtc::PacketTransport* pt); On 2016/10/13 20:26:38, pthatcher1 wrote: > This could probably still be a TransportChannel*. All the others are, and the > method is "OnChannelX". Acknowledged. https://codereview.webrtc.org/2416023002/diff/20001/webrtc/pc/channel.h File webrtc/pc/channel.h (right): https://codereview.webrtc.org/2416023002/diff/20001/webrtc/pc/channel.h#newco... webrtc/pc/channel.h:245: int flags); On 2016/10/13 20:26:38, pthatcher1 wrote: > OnChannelRead should probably be renamed to OnPacketRead. Acknowledged.
Just a few nits. I'd like to have Taylor take a look also. He might have a good idea of how to avoid some of the static casts, which is the only thing from giving this a very enthusiastic thumbs up. For example, one possibility is to make the PacketTransportInterface into a PacketTransportInterface<T> where the SignalX<PacketTransportInterace*> SignalX would be a SignalX<T*>, and a TransportChannel would be a PacketTransportInterrface<TransportChannel>. It's a little more tricky in two or three places, but it would be nice to avoid the static casts in many places. https://codereview.webrtc.org/2416023002/diff/60001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel.h (right): https://codereview.webrtc.org/2416023002/diff/60001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.h:26: struct PacketTransportChannel; Did you mean PacketTransportInterface? https://codereview.webrtc.org/2416023002/diff/60001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.h (right): https://codereview.webrtc.org/2416023002/diff/60001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.h:208: void OnChannelWritableState_n(rtc::PacketTransportInterface* pt); pt => transport https://codereview.webrtc.org/2416023002/diff/60001/webrtc/pc/channel.h File webrtc/pc/channel.h (right): https://codereview.webrtc.org/2416023002/diff/60001/webrtc/pc/channel.h#newco... webrtc/pc/channel.h:384: // TODO(johan): replace TransportChannel* by rtc::PacketTransportInterface*. replace => Replace by => with
Description was changed from ========== Introduce rtc::PacketTransport interface and let cricket::TransportChannel inherit. Introduce rtc::PacketTransport. Refactor cricket::TransportChannel. Fix signal slots parameter types in all related code. BUG=webrtc:6531 ========== to ========== Introduce rtc::PacketTransportInterface and let cricket::TransportChannel inherit. Introduce rtc::PacketTransportInterface. Refactor cricket::TransportChannel. Fix signal slots parameter types in all related code. BUG=webrtc:6531 ==========
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios32_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/11550) ios64_gyp_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gyp_dbg/builds/1451) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/11572) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/13897) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/13790)
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
PTAL I managed to remove the explicit static_casts. The remaining casts should be type safe implicit upcasts from cricket::TransportChannel to rtc::PacketTransportInterface. https://codereview.webrtc.org/2416023002/diff/60001/webrtc/p2p/base/dtlstrans... File webrtc/p2p/base/dtlstransportchannel.h (right): https://codereview.webrtc.org/2416023002/diff/60001/webrtc/p2p/base/dtlstrans... webrtc/p2p/base/dtlstransportchannel.h:26: struct PacketTransportChannel; On 2016/10/14 17:59:17, pthatcher1 wrote: > Did you mean PacketTransportInterface? Oups. Yes. https://codereview.webrtc.org/2416023002/diff/60001/webrtc/p2p/base/transport... File webrtc/p2p/base/transportcontroller.h (right): https://codereview.webrtc.org/2416023002/diff/60001/webrtc/p2p/base/transport... webrtc/p2p/base/transportcontroller.h:208: void OnChannelWritableState_n(rtc::PacketTransportInterface* pt); On 2016/10/14 17:59:17, pthatcher1 wrote: > pt => transport Done. https://codereview.webrtc.org/2416023002/diff/60001/webrtc/pc/channel.h File webrtc/pc/channel.h (right): https://codereview.webrtc.org/2416023002/diff/60001/webrtc/pc/channel.h#newco... webrtc/pc/channel.h:384: // TODO(johan): replace TransportChannel* by rtc::PacketTransportInterface*. On 2016/10/14 17:59:17, pthatcher1 wrote: > replace => Replace > by => with Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Before I hit the commit button and probably break Chromium's build tree: - Chromium's cricket::ChannelSocketAdapter connects to signals that would now be part of rtc::PacketTransportInterface. https://cs.chromium.org/chromium/src/remoting/protocol/channel_socket_adapter... https://cs.chromium.org/chromium/src/remoting/protocol/channel_socket_adapter... - A few methods in cricket::ChannelSocketAdapter would need a modified parameter list: rtc::PacketTransportInterface* instead of cricket::TransportChannel*. - Otherwise template matching for sigslot would fail. Currently, I assume, that this CL and a CL for chromium's cricket::ChannelSocketAdapter have to be applied well timed.
On 2016/10/17 21:11:15, johan wrote: > Before I hit the commit button and probably break Chromium's build tree: > - Chromium's cricket::ChannelSocketAdapter connects to signals that would now be > part of rtc::PacketTransportInterface. > > https://cs.chromium.org/chromium/src/remoting/protocol/channel_socket_adapter... > > https://cs.chromium.org/chromium/src/remoting/protocol/channel_socket_adapter... > - A few methods in cricket::ChannelSocketAdapter would need a modified parameter > list: rtc::PacketTransportInterface* instead of cricket::TransportChannel*. > - Otherwise template matching for sigslot would fail. > > Currently, I assume, that this CL and a CL for chromium's > cricket::ChannelSocketAdapter have to be applied well timed. Thanks for remembering not to break Chromium :). It's not as simple as being well-timed. I think we need to do something like the following: 1. Land a CL that adds PacketTransportInterface to WebRTC as a typedef for TransportChannel. 2. Wait for WebRTC to roll to Chromium (it's supposed to happen every day or so). 3. Change the code in Chromium to use PacketTransportInterface in the places where it's needed. 4. Land this CL changing the typedef to an actual interface. Also, it would be nice if Taylor looked at this CL before submitting. He might also have an idea of a better way to roll this into Chromium without breaking it.
Aside from style nits, the only real thing I'd ask for is more comments documenting PacketTransportInterface. I realize it's not your fault that TransportChannel was missing these comments in the first place, but as long as we're starting fresh, it would be nice if the interface was clearly specified. Also, I'm left with one question: Should receiving() go on PacketTransportInterface? It seems like that would make sense. Peter? https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/dtlstran... File webrtc/p2p/base/dtlstransportchannel.h (right): https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.h:26: struct PacketTransportInterface; If you change "struct" to "class", remember to change here as well. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:284: bool HasChannel(TransportChannel const* const ch) { nit: It seems we could just get rid of this method. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:287: bool HasTransport(rtc::PacketTransportInterface const* const transport) { nit: Our style is "const Foo*" rather than "Foo const*". And the pointer itself doesn't really need to be const. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/packettr... File webrtc/p2p/base/packettransportinterface.h (right): https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/packettr... webrtc/p2p/base/packettransportinterface.h:23: struct PacketTransportInterface : public sigslot::has_slots<> { Any reason to use struct? As a matter of style, we generally only use structs for passive, data-holding objects. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/packettr... webrtc/p2p/base/packettransportinterface.h:25: virtual const std::string debug_name() const = 0; nit: Could be just "name()", with a comment explaining that it's used to identify the object for debugging purposes. Either way though, a comment would be nice. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/packettr... webrtc/p2p/base/packettransportinterface.h:26: virtual bool writable() const = 0; Should have a comment explaining what "writable" means. Which I think would generally be, "The underlying transport has been established." Maybe Peter has a less vague suggestion, but anything would be better than nothing. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/packettr... webrtc/p2p/base/packettransportinterface.h:29: // TODO(johan): Remove the default argument once channel code is updated. Could the comments document where the return codes are defined? Or if they're defined by the subclasses, at least state that here. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/packettr... webrtc/p2p/base/packettransportinterface.h:46: sigslot::signal1<PacketTransportInterface*> SignalWritableState; Would be nice to have a comment here too ("Emitted when the writable state, represented by |writable()|, changes.") https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/packettr... webrtc/p2p/base/packettransportinterface.h:48: // Emitted when the PacketTransportInterface's ability to send has changed. Again, not your fault that this comment was vague/wrong. But could you change it to: "Emitted when the PacketTransportInterface is ready to send packets. "Ready to send" is more sensitive than the writable state; a transport may be writable, but temporarily not able to send packets. For example, the underlying transport's socket buffer may be full, as indicated by SendPacket's return code and/or GetError." https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/transpor... File webrtc/p2p/base/transportchannel.h (left): https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/transpor... webrtc/p2p/base/transportchannel.h:75: bool receiving() const { return receiving_; } Does it make sense for receiving() to go on PacketTransportInterface?
https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/dtlstran... File webrtc/p2p/base/dtlstransportchannel.h (right): https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/dtlstran... webrtc/p2p/base/dtlstransportchannel.h:26: struct PacketTransportInterface; On 2016/10/17 23:24:50, Taylor Brandstetter wrote: > If you change "struct" to "class", remember to change here as well. Done. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:284: bool HasChannel(TransportChannel const* const ch) { On 2016/10/17 23:24:50, Taylor Brandstetter wrote: > nit: It seems we could just get rid of this method. Acknowledged. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:287: bool HasTransport(rtc::PacketTransportInterface const* const transport) { On 2016/10/17 23:24:50, Taylor Brandstetter wrote: > nit: Our style is "const Foo*" rather than "Foo const*". And the pointer itself > doesn't really need to be const. Acknowledged. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/packettr... File webrtc/p2p/base/packettransportinterface.h (right): https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/packettr... webrtc/p2p/base/packettransportinterface.h:23: struct PacketTransportInterface : public sigslot::has_slots<> { On 2016/10/17 23:24:51, Taylor Brandstetter wrote: > Any reason to use struct? As a matter of style, we generally only use structs > for passive, data-holding objects. "struct" usually helps to let people consider twice before adding private/protected members / implementation details to abstract interfaces. Anyway, changing this to class with public-only members. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/packettr... webrtc/p2p/base/packettransportinterface.h:25: virtual const std::string debug_name() const = 0; On 2016/10/17 23:24:51, Taylor Brandstetter wrote: > nit: Could be just "name()", with a comment explaining that it's used to > identify the object for debugging purposes. Either way though, a comment would > be nice. Ack on adding a comment. Please discuss with Peter whether you prefer name() or debug_name(). I'm fine with any of them. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/packettr... webrtc/p2p/base/packettransportinterface.h:26: virtual bool writable() const = 0; On 2016/10/17 23:24:51, Taylor Brandstetter wrote: > Should have a comment explaining what "writable" means. Which I think would > generally be, "The underlying transport has been established." Maybe Peter has a > less vague suggestion, but anything would be better than nothing. Acknowledged. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/packettr... webrtc/p2p/base/packettransportinterface.h:29: // TODO(johan): Remove the default argument once channel code is updated. On 2016/10/17 23:24:50, Taylor Brandstetter wrote: > Could the comments document where the return codes are defined? Or if they're > defined by the subclasses, at least state that here. Documenting current behaviour. You won't like it :-) But fixing that should be done in a dedicated CL. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/packettr... webrtc/p2p/base/packettransportinterface.h:46: sigslot::signal1<PacketTransportInterface*> SignalWritableState; On 2016/10/17 23:24:51, Taylor Brandstetter wrote: > Would be nice to have a comment here too ("Emitted when the writable state, > represented by |writable()|, changes.") Done. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/packettr... webrtc/p2p/base/packettransportinterface.h:48: // Emitted when the PacketTransportInterface's ability to send has changed. On 2016/10/17 23:24:51, Taylor Brandstetter wrote: > Again, not your fault that this comment was vague/wrong. But could you change it > to: > > "Emitted when the PacketTransportInterface is ready to send packets. "Ready to > send" is more sensitive than the writable state; a transport may be writable, > but temporarily not able to send packets. For example, the underlying > transport's socket buffer may be full, as indicated by SendPacket's return code > and/or GetError." Done. https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/transpor... File webrtc/p2p/base/transportchannel.h (left): https://codereview.webrtc.org/2416023002/diff/120001/webrtc/p2p/base/transpor... webrtc/p2p/base/transportchannel.h:75: bool receiving() const { return receiving_; } On 2016/10/17 23:24:51, Taylor Brandstetter wrote: > Does it make sense for receiving() to go on PacketTransportInterface? Sense of code symmetry aesthetics says receiving() should be in the same place as writable(). Imho, yes it makes sense.
lgtm; thanks for responding to all my nits and adding TODOs.
The CQ bit was checked by johan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Peter, what is your opinion on adding receiving() and SignalReceivingState to rtc::PacketTransportInterface ? Maybe we could commit this CL "as is" and discuss receiving() and SignalReceivingState in a dedicated CL https://codereview.webrtc.org/2444793003/ .
Yes, please land this CL as-is and then we can address the receiving() parts in a separate CL.
The CQ bit was checked by johan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org, deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2416023002/#ps180001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by johan@webrtc.org
The CQ bit was checked by johan@webrtc.org
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 ========== Introduce rtc::PacketTransportInterface and let cricket::TransportChannel inherit. Introduce rtc::PacketTransportInterface. Refactor cricket::TransportChannel. Fix signal slots parameter types in all related code. BUG=webrtc:6531 ========== to ========== Introduce rtc::PacketTransportInterface and let cricket::TransportChannel inherit. Introduce rtc::PacketTransportInterface. Refactor cricket::TransportChannel. Fix signal slots parameter types in all related code. BUG=webrtc:6531 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Introduce rtc::PacketTransportInterface and let cricket::TransportChannel inherit. Introduce rtc::PacketTransportInterface. Refactor cricket::TransportChannel. Fix signal slots parameter types in all related code. BUG=webrtc:6531 ========== to ========== Introduce rtc::PacketTransportInterface and let cricket::TransportChannel inherit. Introduce rtc::PacketTransportInterface. Refactor cricket::TransportChannel. Fix signal slots parameter types in all related code. BUG=webrtc:6531 Committed: https://crrev.com/d89ab145cd07c052664ed24873878335652a6e36 Cr-Commit-Position: refs/heads/master@{#14778} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/d89ab145cd07c052664ed24873878335652a6e36 Cr-Commit-Position: refs/heads/master@{#14778} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
