Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1274013002:
Bug 4865: Enable connectivity when the remote peer is on public internet. (Closed)
|
Created:
5 years, 4 months ago by guoweis_webrtc Modified:
5 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionWebRTC Bug 4865
Bug 4865: even without STUN/TURN, as long as the peer is on the open internet, the connectivity should work. This is actually a regression even for hangouts.
We need to issue the 0.0.0.0 candidate into Port::candidates_ and filter it out later. The reason is that when we create connection, we need a local candidate to match the remote candidate.
The same connection later will be updated with the prflx local candidate once the STUN ping response is received.
BUG=webrtc:4865
R=juberti@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/38f8893235f3b80ae9ca89db66d62ca819b51c01
Committed: https://chromium.googlesource.com/external/webrtc/+/1147702958b7639eda2f7fe072103b95be59e5c0
Patch Set 1 #
Total comments: 30
Patch Set 2 : #Patch Set 3 : #
Total comments: 25
Patch Set 4 : #
Total comments: 17
Patch Set 5 : address Peter's comments. #Patch Set 6 : #
Total comments: 19
Patch Set 7 : #
Total comments: 1
Patch Set 8 : #
Messages
Total messages: 21 (3 generated)
guoweis@webrtc.org changed reviewers: + juberti@webrtc.org, pthatcher@webrtc.org
Now it's much simplified. PTAL.
On 2015/08/06 00:04:41, guoweis wrote: > Now it's much simplified. PTAL. [1:33:0805/170046:INFO:port.cc(905)] Jingle:Conn[0x1099b7497420:audio:hipifZ4i:1:0:local:udp:0.0.0.0:58068->q6eYX3D4:1:2130714367:local:udp:54.160.208.69:11716|C--W|9079290933605842430|-]: Connection created [1:33:0805/170046:INFO:p2ptransportchannel.cc(774)] Jingle:Channel[audio|1|__]: Created connection with origin=2, (1 total) later... [1:33:0805/170047:INFO:port.cc(1333)] Jingle:Conn[0x1099b7497420:audio:hipifZ4i:1:0:local:udp:0.0.0.0:58068->q6eYX3D4:1:2130714367:local:udp:54.160.208.69:11716|C-WS|9079290933605842430|-]: Received STUN ping response, id=5a4f6c2b5272745563593343, code=0, rtt=68, use_candidate=0, pings_since_last_response= [1:33:0805/170047:VERBOSE1:port.cc(1035)] Jingle:Conn[0x1099b7497420:audio:iyGjMrZ5:1:0:prflx:udp:74.125.59.73:20602->q6eYX3D4:1:2130714367:local:udp:54.160.208.69:11716|C-WS|7926369428998995454|2267]: Received STUN ping, id=e8634f004f01ad2c14766415
overall this looks like the right change. Several minor comments. https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... File webrtc/base/virtualsocketserver.cc (right): https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... webrtc/base/virtualsocketserver.cc:289: // HACK(any_address): when the incoming packet is from a binding of the any Can this be done on the send side? i.e. in AddPacketToNetwork? https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... webrtc/base/virtualsocketserver.cc:675: // HACK(any_address): If we can't find a binding, maybe this is resulted from Is this still needed if we do the change I suggest above? https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... File webrtc/base/virtualsocketserver.h (right): https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... webrtc/base/virtualsocketserver.h:48: const IPAddress default_interface(int family) { Style: if it takes arguments, it's usually not lowercase. Suggest making this either GetDefaultInterface(family), and perhaps making it private, or splitting these routines into (set_)default_interface_v4/(set_)default_interface_v6 https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... webrtc/p2p/client/basicportallocator.cc:454: SignalCandidatesReady(this, candidates); Will we push 0.0.0.0 to the application here? We want to avoid that. https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... webrtc/p2p/client/basicportallocator.cc:457: // Moving to READY state as we have at least one candidate from the port or This comment could be worded better. It's important to understand why candidate_allowed_to_send is false for the any address. https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... File webrtc/p2p/client/portallocator_unittest.cc (right): https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... webrtc/p2p/client/portallocator_unittest.cc:246: void CheckDisableAdapterEnumerationNonSharedSocket() { can we just fix this to use shared socket? non-shared socket is going to disappear very soon. https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... webrtc/p2p/client/portallocator_unittest.cc:1175: // Test that when adapter enumeration is disabled, STUN is the only candidate Isn't this already tested by the tests above? https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... webrtc/p2p/client/portallocator_unittest.cc:1177: TEST_F(PortAllocatorTest, TestAdapterEnumerationDisabledWithoutNat) { move this next to other adapter-enum disabled tests?
https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... File webrtc/base/virtualsocketserver.cc (right): https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... webrtc/base/virtualsocketserver.cc:289: // HACK(any_address): when the incoming packet is from a binding of the any On 2015/08/06 00:35:26, juberti1 wrote: > Can this be done on the send side? i.e. in AddPacketToNetwork? I was thinking the same thing. That would be more clear to me. https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... webrtc/base/virtualsocketserver.cc:1099: const SocketAddress VirtualSocketServer::TryMapAnyAddressToDefault( I think a better name would be GetDefaultRouteIfNecessary https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... File webrtc/base/virtualsocketserver.h (right): https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... webrtc/base/virtualsocketserver.h:48: const IPAddress default_interface(int family) { I think this should be GetDefaultRoute. https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... webrtc/base/virtualsocketserver.h:57: void set_default_interface(const IPAddress& ipaddr) { And this should be SetDefaultRoute. https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... webrtc/base/virtualsocketserver.h:251: IPAddress default_interface_v6_; And this default_route_vX_; https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... webrtc/p2p/client/basicportallocator.cc:454: SignalCandidatesReady(this, candidates); On 2015/08/06 00:35:26, juberti1 wrote: > Will we push 0.0.0.0 to the application here? We want to avoid that. I believe candidate_allowed_to_send will be false because CheckCandidateFilter filters it out. https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... webrtc/p2p/client/basicportallocator.cc:463: // possible if the remote peer has a routable IP address on public Internet. I think we can re-word this in terms of "we're ready if we have a candidate that can send or a candidate that can receive" (see my next comment). https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... webrtc/p2p/client/basicportallocator.cc:465: (c.address().IsAnyIP() && port->SharedSocket()))) { I think this would be a lot more readable if we broke up CheckCandidateFilter and this logic into two predicate functions: CandidateCanReceive (worth signalling) CandidateCanSend (worth pinging from) That way, much of this method can be somewhat like: if (CandidateCanReceive(c)) { std::vector<Candidate> candidates; candidates.push_back(c); SignalCandidatesReady(this, candidates); } if ((CandidateCanReceive(c) || CandidateCanSend(c)) && !data->ready()) { data->set_ready(); SignalPortReady(this, port); } https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... File webrtc/p2p/client/portallocator_unittest.cc (right): https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... webrtc/p2p/client/portallocator_unittest.cc:113: void AddInterfaceAsDefault(const SocketAddress& addr) { Would SetDefaultRoute make more sense? https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... webrtc/p2p/client/portallocator_unittest.cc:1175: // Test that when adapter enumeration is disabled, STUN is the only candidate On 2015/08/06 00:35:26, juberti1 wrote: > Isn't this already tested by the tests above? I think this one is different because it is shared socket and PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION. The other is not shared socket. But we should probably just delete the non-shared socket test.
PTAL. https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... File webrtc/base/virtualsocketserver.cc (right): https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... webrtc/base/virtualsocketserver.cc:289: // HACK(any_address): when the incoming packet is from a binding of the any On 2015/08/06 01:06:18, pthatcher1 wrote: > On 2015/08/06 00:35:26, juberti1 wrote: > > Can this be done on the send side? i.e. in AddPacketToNetwork? > > I was thinking the same thing. That would be more clear to me. Done. https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... webrtc/base/virtualsocketserver.cc:675: // HACK(any_address): If we can't find a binding, maybe this is resulted from On 2015/08/06 00:35:26, juberti1 wrote: > Is this still needed if we do the change I suggest above? yes, since the binding of the default route just doesn't exist. https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... webrtc/base/virtualsocketserver.cc:1099: const SocketAddress VirtualSocketServer::TryMapAnyAddressToDefault( On 2015/08/06 01:06:18, pthatcher1 wrote: > I think a better name would be GetDefaultRouteIfNecessary Done. https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... File webrtc/base/virtualsocketserver.h (right): https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... webrtc/base/virtualsocketserver.h:48: const IPAddress default_interface(int family) { On 2015/08/06 01:06:19, pthatcher1 wrote: > I think this should be GetDefaultRoute. Done. https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... webrtc/p2p/client/basicportallocator.cc:454: SignalCandidatesReady(this, candidates); On 2015/08/06 01:06:19, pthatcher1 wrote: > On 2015/08/06 00:35:26, juberti1 wrote: > > Will we push 0.0.0.0 to the application here? We want to avoid that. > > I believe candidate_allowed_to_send will be false because CheckCandidateFilter > filters it out. Correct. It'll be filtered out https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... webrtc/p2p/client/basicportallocator.cc:457: // Moving to READY state as we have at least one candidate from the port or On 2015/08/06 00:35:26, juberti1 wrote: > This comment could be worded better. It's important to understand why > candidate_allowed_to_send is false for the any address. Done. https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... webrtc/p2p/client/basicportallocator.cc:463: // possible if the remote peer has a routable IP address on public Internet. On 2015/08/06 01:06:19, pthatcher1 wrote: > I think we can re-word this in terms of "we're ready if we have a candidate that > can send or a candidate that can receive" (see my next comment). Done. https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... webrtc/p2p/client/basicportallocator.cc:465: (c.address().IsAnyIP() && port->SharedSocket()))) { On 2015/08/06 01:06:19, pthatcher1 wrote: > I think this would be a lot more readable if we broke up CheckCandidateFilter > and this logic into two predicate functions: > > CandidateCanReceive (worth signalling) > CandidateCanSend (worth pinging from) > > > That way, much of this method can be somewhat like: > > if (CandidateCanReceive(c)) { > std::vector<Candidate> candidates; > candidates.push_back(c); > SignalCandidatesReady(this, candidates); > } > > if ((CandidateCanReceive(c) || CandidateCanSend(c)) && !data->ready()) { > data->set_ready(); > SignalPortReady(this, port); > } I rephrase it a bit. Send and Receive is not very clear to me. TCP active candidate is worth to signal but can't receive, for example. https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... File webrtc/p2p/client/portallocator_unittest.cc (right): https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... webrtc/p2p/client/portallocator_unittest.cc:113: void AddInterfaceAsDefault(const SocketAddress& addr) { On 2015/08/06 01:06:19, pthatcher1 wrote: > Would SetDefaultRoute make more sense? I renamed it as AddInterfaceAsDefaultRoute, since it's doing the AddInterface part. https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... webrtc/p2p/client/portallocator_unittest.cc:246: void CheckDisableAdapterEnumerationNonSharedSocket() { On 2015/08/06 00:35:26, juberti1 wrote: > can we just fix this to use shared socket? non-shared socket is going to > disappear very soon. Done. https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... webrtc/p2p/client/portallocator_unittest.cc:1175: // Test that when adapter enumeration is disabled, STUN is the only candidate On 2015/08/06 01:06:19, pthatcher1 wrote: > On 2015/08/06 00:35:26, juberti1 wrote: > > Isn't this already tested by the tests above? > > I think this one is different because it is shared socket and > PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION. The other is not shared socket. But > we should probably just delete the non-shared socket test. this one is different because it doesn't have a NAT. It's testing the endpoint sitting on the public internet. https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... webrtc/p2p/client/portallocator_unittest.cc:1177: TEST_F(PortAllocatorTest, TestAdapterEnumerationDisabledWithoutNat) { On 2015/08/06 00:35:26, juberti1 wrote: > move this next to other adapter-enum disabled tests? Done.
On 2015/08/06 08:50:07, guoweis wrote: > PTAL. > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... > File webrtc/base/virtualsocketserver.cc (right): > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... > webrtc/base/virtualsocketserver.cc:289: // HACK(any_address): when the incoming > packet is from a binding of the any > On 2015/08/06 01:06:18, pthatcher1 wrote: > > On 2015/08/06 00:35:26, juberti1 wrote: > > > Can this be done on the send side? i.e. in AddPacketToNetwork? > > > > I was thinking the same thing. That would be more clear to me. > > Done. > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... > webrtc/base/virtualsocketserver.cc:675: // HACK(any_address): If we can't find a > binding, maybe this is resulted from > On 2015/08/06 00:35:26, juberti1 wrote: > > Is this still needed if we do the change I suggest above? > > yes, since the binding of the default route just doesn't exist. > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... > webrtc/base/virtualsocketserver.cc:1099: const SocketAddress > VirtualSocketServer::TryMapAnyAddressToDefault( > On 2015/08/06 01:06:18, pthatcher1 wrote: > > I think a better name would be GetDefaultRouteIfNecessary > > Done. > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... > File webrtc/base/virtualsocketserver.h (right): > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/base/virtualsocketserv... > webrtc/base/virtualsocketserver.h:48: const IPAddress default_interface(int > family) { > On 2015/08/06 01:06:19, pthatcher1 wrote: > > I think this should be GetDefaultRoute. > > Done. > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... > File webrtc/p2p/client/basicportallocator.cc (right): > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... > webrtc/p2p/client/basicportallocator.cc:454: SignalCandidatesReady(this, > candidates); > On 2015/08/06 01:06:19, pthatcher1 wrote: > > On 2015/08/06 00:35:26, juberti1 wrote: > > > Will we push 0.0.0.0 to the application here? We want to avoid that. > > > > I believe candidate_allowed_to_send will be false because CheckCandidateFilter > > filters it out. > > Correct. It'll be filtered out > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... > webrtc/p2p/client/basicportallocator.cc:457: // Moving to READY state as we have > at least one candidate from the port or > On 2015/08/06 00:35:26, juberti1 wrote: > > This comment could be worded better. It's important to understand why > > candidate_allowed_to_send is false for the any address. > > Done. > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... > webrtc/p2p/client/basicportallocator.cc:463: // possible if the remote peer has > a routable IP address on public Internet. > On 2015/08/06 01:06:19, pthatcher1 wrote: > > I think we can re-word this in terms of "we're ready if we have a candidate > that > > can send or a candidate that can receive" (see my next comment). > > Done. > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/basicportal... > webrtc/p2p/client/basicportallocator.cc:465: (c.address().IsAnyIP() && > port->SharedSocket()))) { > On 2015/08/06 01:06:19, pthatcher1 wrote: > > I think this would be a lot more readable if we broke up CheckCandidateFilter > > and this logic into two predicate functions: > > > > CandidateCanReceive (worth signalling) > > CandidateCanSend (worth pinging from) > > > > > > That way, much of this method can be somewhat like: > > > > if (CandidateCanReceive(c)) { > > std::vector<Candidate> candidates; > > candidates.push_back(c); > > SignalCandidatesReady(this, candidates); > > } > > > > if ((CandidateCanReceive(c) || CandidateCanSend(c)) && !data->ready()) { > > data->set_ready(); > > SignalPortReady(this, port); > > } > > I rephrase it a bit. Send and Receive is not very clear to me. TCP active > candidate is worth to signal but can't receive, for example. > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... > File webrtc/p2p/client/portallocator_unittest.cc (right): > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... > webrtc/p2p/client/portallocator_unittest.cc:113: void > AddInterfaceAsDefault(const SocketAddress& addr) { > On 2015/08/06 01:06:19, pthatcher1 wrote: > > Would SetDefaultRoute make more sense? > > I renamed it as AddInterfaceAsDefaultRoute, since it's doing the AddInterface > part. > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... > webrtc/p2p/client/portallocator_unittest.cc:246: void > CheckDisableAdapterEnumerationNonSharedSocket() { > On 2015/08/06 00:35:26, juberti1 wrote: > > can we just fix this to use shared socket? non-shared socket is going to > > disappear very soon. > > Done. > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... > webrtc/p2p/client/portallocator_unittest.cc:1175: // Test that when adapter > enumeration is disabled, STUN is the only candidate > On 2015/08/06 01:06:19, pthatcher1 wrote: > > On 2015/08/06 00:35:26, juberti1 wrote: > > > Isn't this already tested by the tests above? > > > > I think this one is different because it is shared socket and > > PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION. The other is not shared socket. > But > > we should probably just delete the non-shared socket test. > > this one is different because it doesn't have a NAT. It's testing the endpoint > sitting on the public internet. > > https://codereview.webrtc.org/1274013002/diff/1/webrtc/p2p/client/portallocat... > webrtc/p2p/client/portallocator_unittest.cc:1177: TEST_F(PortAllocatorTest, > TestAdapterEnumerationDisabledWithoutNat) { > On 2015/08/06 00:35:26, juberti1 wrote: > > move this next to other adapter-enum disabled tests? > > Done. PIng. I'd like to get this in for M46.
Suggest adding a VSS unit test to test that 0.0.0.0 can send to 1.2.3.4, and 1.2.3.4 sees the default route source address, as well as 1.2.3.4 can send to 0.0.0.0 via the default route https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... File webrtc/base/virtualsocketserver.cc (right): https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.cc:669: // HACK(any_address): If we can't find a binding, maybe this is resulted from I don't think this is a hack. If a packet is sent to the interface corresponding to the default route, it should match a binding with the correct port to the any address. I think we should just explain that in the comment here. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.cc:881: // HACK(any_address): when the incoming packet is from a binding of the any I don't really think this is a hack - this seems like an entirely reasonable solution. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.cc:1099: const SocketAddress VirtualSocketServer::GetDefaultRouteIfNecessary( Nothing else calls this function, so I think it would be clearer to just include this code in AddPacketToNetwork. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... File webrtc/base/virtualsocketserver.h (right): https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.h:41: // Default route will be used when a binding is done against the any address. // The default route indicates which local address to use when a socket is bound to the 'any' address, e.g. 0.0.0.0. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.h:42: const IPAddress GetDefaultRoute(int family); 'const' not useful on by-value types https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.h:43: void SetDefaultRoute(const IPAddress& ipaddr); Make it clear this is the source address, i.e. call this local_addr or from_addr https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.h:133: const SocketAddress GetDefaultRouteIfNecessary(const SocketAddress& addr); see comment above about const https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/basicpo... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:445: bool candidate_allowed_to_send = CheckCandidateFilter(c); I think the terminology here is confusing. |candidate_allowed_to_send| really means "the candidate can be surfaced to the application". Maybe call this |candidate_type_enabled| instead. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:446: bool port_bound_to_any_address = I don't think this is right. The port can be bound to an any address, but may expose candidates that are not the any address (e.g. STUN/TURN). So this should really be |candidate_is_any_address|. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:475: if (candidate_allowed_to_send || port_bound_to_any_address) { Rewording: Move the port to the READY state, either because we have a usable candidate from the port, or simply because the port is bound to the any address and therefore has no host candidate. This will trigger the port to start creating candidate pairs (connections) and issue connectivity checks. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/portall... File webrtc/p2p/client/portallocator_unittest.cc (right): https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/portall... webrtc/p2p/client/portallocator_unittest.cc:482: TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationMultipleInterfaces) { TestDisableAdapterEnumerationBehindNatMultipleInterfaces https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/portall... webrtc/p2p/client/portallocator_unittest.cc:493: TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationWithoutNat) { can we just parameterize CheckDisableAdapterEnumeration to do this? https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/portall... webrtc/p2p/client/portallocator_unittest.cc:497: cricket::PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION | since we have no turn servers, why do we need to disable relay? https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/portall... webrtc/p2p/client/portallocator_unittest.cc:513: TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationWithoutNat_NoCandidate) { see above
PTAL. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... File webrtc/base/virtualsocketserver.cc (right): https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.cc:669: // HACK(any_address): If we can't find a binding, maybe this is resulted from On 2015/08/06 23:43:21, juberti1 wrote: > I don't think this is a hack. If a packet is sent to the interface corresponding > to the default route, it should match a binding with the correct port to the any > address. > > I think we should just explain that in the comment here. Remove the HACK comment. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.cc:881: // HACK(any_address): when the incoming packet is from a binding of the any On 2015/08/06 23:43:21, juberti1 wrote: > I don't really think this is a hack - this seems like an entirely reasonable > solution. Done. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.cc:1099: const SocketAddress VirtualSocketServer::GetDefaultRouteIfNecessary( On 2015/08/06 23:43:21, juberti1 wrote: > Nothing else calls this function, so I think it would be clearer to just include > this code in AddPacketToNetwork. Done. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... File webrtc/base/virtualsocketserver.h (right): https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.h:41: // Default route will be used when a binding is done against the any address. On 2015/08/06 23:43:21, juberti1 wrote: > // The default route indicates which local address to use when a socket is bound > to the 'any' address, e.g. 0.0.0.0. Done. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.h:42: const IPAddress GetDefaultRoute(int family); On 2015/08/06 23:43:21, juberti1 wrote: > 'const' not useful on by-value types Done. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.h:43: void SetDefaultRoute(const IPAddress& ipaddr); On 2015/08/06 23:43:21, juberti1 wrote: > Make it clear this is the source address, i.e. call this local_addr or from_addr Done. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.h:133: const SocketAddress GetDefaultRouteIfNecessary(const SocketAddress& addr); On 2015/08/06 23:43:21, juberti1 wrote: > see comment above about const Done. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/basicpo... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:445: bool candidate_allowed_to_send = CheckCandidateFilter(c); On 2015/08/06 23:43:21, juberti1 wrote: > I think the terminology here is confusing. |candidate_allowed_to_send| really > means "the candidate can be surfaced to the application". Maybe call this > |candidate_type_enabled| instead. Done. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:446: bool port_bound_to_any_address = On 2015/08/06 23:43:21, juberti1 wrote: > I don't think this is right. The port can be bound to an any address, but may > expose candidates that are not the any address (e.g. STUN/TURN). So this should > really be |candidate_is_any_address|. Done. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:475: if (candidate_allowed_to_send || port_bound_to_any_address) { On 2015/08/06 23:43:21, juberti1 wrote: > Rewording: > > Move the port to the READY state, either because we have a usable candidate from > the port, or simply because the port is bound to the any address and therefore > has no host candidate. This will trigger the port to start creating candidate > pairs (connections) and issue connectivity checks. Done, thanks. https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/portall... File webrtc/p2p/client/portallocator_unittest.cc (right): https://codereview.webrtc.org/1274013002/diff/40001/webrtc/p2p/client/portall... webrtc/p2p/client/portallocator_unittest.cc:482: TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationMultipleInterfaces) { On 2015/08/06 23:43:21, juberti1 wrote: > TestDisableAdapterEnumerationBehindNatMultipleInterfaces Done.
Patchset #4 (id:60001) has been deleted
Just style/naming stuff now :) https://codereview.webrtc.org/1274013002/diff/80001/webrtc/base/virtualsocket... File webrtc/base/virtualsocket_unittest.cc (right): https://codereview.webrtc.org/1274013002/diff/80001/webrtc/base/virtualsocket... webrtc/base/virtualsocket_unittest.cc:188: // shouldn't be the destination. Can we just remove the current behavior, or do we rely on it? If we rely on it, please put a comment why. If not, please put a TODO for removing it (or just remove it). https://codereview.webrtc.org/1274013002/diff/80001/webrtc/base/virtualsocket... File webrtc/base/virtualsocketserver.cc (right): https://codereview.webrtc.org/1274013002/diff/80001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.cc:668: } I think this would be more clear as: if (it != bindings_->end()) { return it->second; } if (addr.ipaddr() == GetDefaultRoute(addr.ipaddr().family()) { // If we can't find a binding for the packet which is sent to the interface // corresponding to the default route, it should match a binding with the // correct port to the any address. SocketAddress sock_addr = EmptySocketAddressWithFamily(addr.ipaddr().family()); sock_addr.SetPort(addr.port()); return LookupBinding(sock_addr); } return nullptr; https://codereview.webrtc.org/1274013002/diff/80001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.cc:884: SocketAddress addr = sender->local_addr_; I think "sender_addr" would be more clear than just "addr". https://codereview.webrtc.org/1274013002/diff/80001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.cc:887: addr.SetIP(default_ip); Since default_ip isn't used outside this scope, why not just: sender_addr.SetIP(GetDefaultRoute(send_addr.ipaddr().family()) ? https://codereview.webrtc.org/1274013002/diff/80001/webrtc/p2p/client/basicpo... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/1274013002/diff/80001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:445: bool candidate_type_enabled = CheckCandidateFilter(c); I think candidate_signalable would be a good name here. https://codereview.webrtc.org/1274013002/diff/80001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:446: bool candidate_is_any_address = c.address().IsAnyIP() && port->SharedSocket(); I think something like candidate_unsignalable_but_pairable would be good, to indicate that we wan't signal it, but we still want to pair it with signalled candidates from the remote side. Or if that name is too long, you could do: bool candidate_pairable = candidate_signalable || (c.address().IsAnyIP() && port->SharedSocket()); https://codereview.webrtc.org/1274013002/diff/80001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:453: DCHECK(!candidate_type_enabled || !candidate_is_any_address); DCHECK(!(candidate_type_enabled && candidate_is_any_address)); Would be more clear, and match the comment more clearly. Or even better: DCHECK(!(candidate_signalable && candidate_unsignalable_but_pairable)) Or just remove the DCHECK, since it's really covered by CheckCandidateFilter anyway. https://codereview.webrtc.org/1274013002/diff/80001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:455: if (candidate_type_enabled && candidate_protocol_enabled) { if (candidate_signalable && candidate_protocol_enabled) { // ... } Would look nice here https://codereview.webrtc.org/1274013002/diff/80001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:470: if (candidate_type_enabled || candidate_is_any_address) { if (candidate_pairable) { // ... } Would look nice here.
Patchset #6 (id:120001) has been deleted
added TCP support. PTAL. https://codereview.webrtc.org/1274013002/diff/80001/webrtc/base/virtualsocket... File webrtc/base/virtualsocket_unittest.cc (right): https://codereview.webrtc.org/1274013002/diff/80001/webrtc/base/virtualsocket... webrtc/base/virtualsocket_unittest.cc:188: // shouldn't be the destination. On 2015/08/07 21:28:19, pthatcher1 wrote: > Can we just remove the current behavior, or do we rely on it? If we rely on it, > please put a comment why. If not, please put a TODO for removing it (or just > remove it). Done. https://codereview.webrtc.org/1274013002/diff/80001/webrtc/base/virtualsocket... File webrtc/base/virtualsocketserver.cc (right): https://codereview.webrtc.org/1274013002/diff/80001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.cc:668: } On 2015/08/07 21:28:19, pthatcher1 wrote: > I think this would be more clear as: > > if (it != bindings_->end()) { > return it->second; > } > > if (addr.ipaddr() == GetDefaultRoute(addr.ipaddr().family()) { > // If we can't find a binding for the packet which is sent to the interface > // corresponding to the default route, it should match a binding with the > // correct port to the any address. > SocketAddress sock_addr = > EmptySocketAddressWithFamily(addr.ipaddr().family()); > sock_addr.SetPort(addr.port()); > return LookupBinding(sock_addr); > > } > > return nullptr; Yes, it is much better. Thanks. https://codereview.webrtc.org/1274013002/diff/80001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.cc:884: SocketAddress addr = sender->local_addr_; On 2015/08/07 21:28:19, pthatcher1 wrote: > I think "sender_addr" would be more clear than just "addr". Done. https://codereview.webrtc.org/1274013002/diff/80001/webrtc/base/virtualsocket... webrtc/base/virtualsocketserver.cc:887: addr.SetIP(default_ip); On 2015/08/07 21:28:19, pthatcher1 wrote: > Since default_ip isn't used outside this scope, why not just: > > sender_addr.SetIP(GetDefaultRoute(send_addr.ipaddr().family()) > > ? I still want to check whether IPIsUnspec. Yes, When IPIsUnspec, it's probably the any address any way but it's too subtle. https://codereview.webrtc.org/1274013002/diff/80001/webrtc/p2p/client/basicpo... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/1274013002/diff/80001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:445: bool candidate_type_enabled = CheckCandidateFilter(c); On 2015/08/07 21:28:19, pthatcher1 wrote: > I think candidate_signalable would be a good name here. Done. https://codereview.webrtc.org/1274013002/diff/80001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:446: bool candidate_is_any_address = c.address().IsAnyIP() && port->SharedSocket(); On 2015/08/07 21:28:19, pthatcher1 wrote: > I think something like candidate_unsignalable_but_pairable would be good, to > indicate that we wan't signal it, but we still want to pair it with signalled > candidates from the remote side. > > Or if that name is too long, you could do: > > bool candidate_pairable = candidate_signalable || (c.address().IsAnyIP() && > port->SharedSocket()); I don't mind the long name. I think candidate_pairable is a bit too subtle. https://codereview.webrtc.org/1274013002/diff/80001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:453: DCHECK(!candidate_type_enabled || !candidate_is_any_address); On 2015/08/07 21:28:19, pthatcher1 wrote: > DCHECK(!(candidate_type_enabled && candidate_is_any_address)); > > Would be more clear, and match the comment more clearly. > > Or even better: > > DCHECK(!(candidate_signalable && candidate_unsignalable_but_pairable)) > > > Or just remove the DCHECK, since it's really covered by CheckCandidateFilter > anyway. Done. https://codereview.webrtc.org/1274013002/diff/80001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:455: if (candidate_type_enabled && candidate_protocol_enabled) { On 2015/08/07 21:28:19, pthatcher1 wrote: > if (candidate_signalable && candidate_protocol_enabled) { > // ... > } > > Would look nice here Done.
Overall lgtm. Just minor comments. https://codereview.webrtc.org/1274013002/diff/140001/webrtc/base/virtualsocke... File webrtc/base/virtualsocketserver.cc (right): https://codereview.webrtc.org/1274013002/diff/140001/webrtc/base/virtualsocke... webrtc/base/virtualsocketserver.cc:676: return LookupBinding(sock_addr); Is there any chance this could infinitely recurse if the default route isn't set properly? Should this just do another bindings_->find()? https://codereview.webrtc.org/1274013002/diff/140001/webrtc/base/virtualsocke... webrtc/base/virtualsocketserver.cc:885: // to the default route here such that the STUN server will see the default "STUN server" -> "recipient" https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:445: bool candidate_signable = CheckCandidateFilter(c); I think you want "signalable" (that's what was in Peter's comment) https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:446: bool candidate_unsignable_but_pairable = I like Peter's idea of candidate_pairable = candidate_signable || ... That makes the code below simpler. https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... File webrtc/p2p/client/portallocator_unittest.cc (right): https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... webrtc/p2p/client/portallocator_unittest.cc:142: nat_socket_factory_.reset(new rtc::BasicPacketSocketFactory()); why do we need to change the socket factory here? https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... webrtc/p2p/client/portallocator_unittest.cc:264: bool expect_stun_candidate, can we omit the second param? We can just check if stun_candidate_addr.IsNil. https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... webrtc/p2p/client/portallocator_unittest.cc:267: bool expect_relay_tcp_candidate) { It's not really a TCP candidate. maybe call this expect_relay_candidate_udp_transport and expect_relay_candidate_tcp_transport. Consider passing in the actual relay candidate addresses here too. https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... webrtc/p2p/client/portallocator_unittest.cc:278: total_candidates++; Style: prefer preincrement (here and below) https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... webrtc/p2p/client/portallocator_unittest.cc:563: TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationWithoutNat_NoCandidate) { Call this ...WithoutNatOrServers
Message was sent while issue was closed.
Committed patchset #7 (id:160001) manually as 38f8893235f3b80ae9ca89db66d62ca819b51c01 (presubmit successful).
Message was sent while issue was closed.
landing it now. https://codereview.webrtc.org/1274013002/diff/140001/webrtc/base/virtualsocke... File webrtc/base/virtualsocketserver.cc (right): https://codereview.webrtc.org/1274013002/diff/140001/webrtc/base/virtualsocke... webrtc/base/virtualsocketserver.cc:676: return LookupBinding(sock_addr); On 2015/08/13 21:23:59, juberti1 wrote: > Is there any chance this could infinitely recurse if the default route isn't set > properly? Should this just do another bindings_->find()? If the addr is Unspec, yes, it'll be a infinite loop. I added a check to make sure the default route is specified. https://codereview.webrtc.org/1274013002/diff/140001/webrtc/base/virtualsocke... webrtc/base/virtualsocketserver.cc:885: // to the default route here such that the STUN server will see the default On 2015/08/13 21:23:59, juberti1 wrote: > "STUN server" -> "recipient" Done. https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:445: bool candidate_signable = CheckCandidateFilter(c); On 2015/08/13 21:23:59, juberti1 wrote: > I think you want "signalable" (that's what was in Peter's comment) Done. https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:446: bool candidate_unsignable_but_pairable = On 2015/08/13 21:23:59, juberti1 wrote: > I like Peter's idea of candidate_pairable = candidate_signable || ... > > That makes the code below simpler. Done. https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... File webrtc/p2p/client/portallocator_unittest.cc (right): https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... webrtc/p2p/client/portallocator_unittest.cc:142: nat_socket_factory_.reset(new rtc::BasicPacketSocketFactory()); On 2015/08/13 21:23:59, juberti1 wrote: > why do we need to change the socket factory here? nat_socket_factory assumes there is NAT. https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... webrtc/p2p/client/portallocator_unittest.cc:264: bool expect_stun_candidate, On 2015/08/13 21:23:59, juberti1 wrote: > can we omit the second param? We can just check if stun_candidate_addr.IsNil. Done. https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... webrtc/p2p/client/portallocator_unittest.cc:267: bool expect_relay_tcp_candidate) { On 2015/08/13 21:23:59, juberti1 wrote: > It's not really a TCP candidate. maybe call this > expect_relay_candidate_udp_transport and expect_relay_candidate_tcp_transport. > > Consider passing in the actual relay candidate addresses here too. Done. https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... webrtc/p2p/client/portallocator_unittest.cc:278: total_candidates++; On 2015/08/13 21:23:59, juberti1 wrote: > Style: prefer preincrement (here and below) Done. https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... webrtc/p2p/client/portallocator_unittest.cc:563: TEST_F(PortAllocatorTest, TestDisableAdapterEnumerationWithoutNat_NoCandidate) { On 2015/08/13 21:23:59, juberti1 wrote: > Call this ...WithoutNatOrServers Done.
Message was sent while issue was closed.
lgtm, a couple minor comments to address in a followup https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... File webrtc/p2p/client/portallocator_unittest.cc (right): https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... webrtc/p2p/client/portallocator_unittest.cc:142: nat_socket_factory_.reset(new rtc::BasicPacketSocketFactory()); On 2015/08/14 05:24:25, guoweis wrote: > On 2015/08/13 21:23:59, juberti1 wrote: > > why do we need to change the socket factory here? > > nat_socket_factory assumes there is NAT. OK, I was confused by the name, since typically you use a STUN server when there is a NAT. I think this should be called ResetWithStunServerNoNat, and the previous function called ResetWithStunServerAndNat. We should probably also rename ResetWithTurnServers to be ResetWithTurnServersNoNat, and ResetWithNoServers to be ResetWithNoServersOrNat. Alternatively, we could just add comments to the ResetWithXServers functions saying that there is no NAT present, although I think that is a bit unexpected. https://codereview.webrtc.org/1274013002/diff/160001/webrtc/p2p/client/portal... File webrtc/p2p/client/portallocator_unittest.cc (right): https://codereview.webrtc.org/1274013002/diff/160001/webrtc/p2p/client/portal... webrtc/p2p/client/portallocator_unittest.cc:277: if (!IPIsUnspec(stun_candidate_addr)) { I think we typically use .IsNil for this
On 2015/08/14 23:22:43, juberti1 wrote: > lgtm, a couple minor comments to address in a followup > > https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... > File webrtc/p2p/client/portallocator_unittest.cc (right): > > https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... > webrtc/p2p/client/portallocator_unittest.cc:142: nat_socket_factory_.reset(new > rtc::BasicPacketSocketFactory()); > On 2015/08/14 05:24:25, guoweis wrote: > > On 2015/08/13 21:23:59, juberti1 wrote: > > > why do we need to change the socket factory here? > > > > nat_socket_factory assumes there is NAT. > > OK, I was confused by the name, since typically you use a STUN server when there > is a NAT. > > I think this should be called ResetWithStunServerNoNat, and the previous > function called ResetWithStunServerAndNat. > > We should probably also rename ResetWithTurnServers to be > ResetWithTurnServersNoNat, and ResetWithNoServers to be ResetWithNoServersOrNat. > Alternatively, we could just add comments to the ResetWithXServers functions > saying that there is no NAT present, although I think that is a bit unexpected. Done > > https://codereview.webrtc.org/1274013002/diff/160001/webrtc/p2p/client/portal... > File webrtc/p2p/client/portallocator_unittest.cc (right): > > https://codereview.webrtc.org/1274013002/diff/160001/webrtc/p2p/client/portal... > webrtc/p2p/client/portallocator_unittest.cc:277: if > (!IPIsUnspec(stun_candidate_addr)) { > I think we typically use .IsNil for this There is only SocketAddress.IsNil. I added IPAddress.IsNil which is basically calling IPIsUnspec, along with one test.
On 2015/08/14 23:22:43, juberti1 wrote: > lgtm, a couple minor comments to address in a followup > > https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... > File webrtc/p2p/client/portallocator_unittest.cc (right): > > https://codereview.webrtc.org/1274013002/diff/140001/webrtc/p2p/client/portal... > webrtc/p2p/client/portallocator_unittest.cc:142: nat_socket_factory_.reset(new > rtc::BasicPacketSocketFactory()); > On 2015/08/14 05:24:25, guoweis wrote: > > On 2015/08/13 21:23:59, juberti1 wrote: > > > why do we need to change the socket factory here? > > > > nat_socket_factory assumes there is NAT. > > OK, I was confused by the name, since typically you use a STUN server when there > is a NAT. > > I think this should be called ResetWithStunServerNoNat, and the previous > function called ResetWithStunServerAndNat. > > We should probably also rename ResetWithTurnServers to be > ResetWithTurnServersNoNat, and ResetWithNoServers to be ResetWithNoServersOrNat. > Alternatively, we could just add comments to the ResetWithXServers functions > saying that there is no NAT present, although I think that is a bit unexpected. Done > > https://codereview.webrtc.org/1274013002/diff/160001/webrtc/p2p/client/portal... > File webrtc/p2p/client/portallocator_unittest.cc (right): > > https://codereview.webrtc.org/1274013002/diff/160001/webrtc/p2p/client/portal... > webrtc/p2p/client/portallocator_unittest.cc:277: if > (!IPIsUnspec(stun_candidate_addr)) { > I think we typically use .IsNil for this There is only SocketAddress.IsNil. I added IPAddress.IsNil which is basically calling IPIsUnspec, along with one test.
Message was sent while issue was closed.
Committed patchset #8 (id:180001) manually as 1147702958b7639eda2f7fe072103b95be59e5c0 (presubmit successful).
Message was sent while issue was closed.
lgtm - thanks for doing that cleanup |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
