Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2212683002:
Do not switch a connection if the new connection is not ready to send packets. (Closed)
DescriptionDo not switch a connection if the new connection is not ready to send packets.
There is no benefit of making such switches.
R=deadbeef@webrtc.org, pthatcher@webrtc.org
Committed: https://crrev.com/e05bcc22b370ab8458d88807df0122e3e16393ba
Cr-Commit-Position: refs/heads/master@{#13789}
Patch Set 1 : Updated tests #
Total comments: 12
Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : Merge #
Messages
Total messages: 26 (14 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Do not switch a connection if the new connection is not ready to send packets. There is no point to make such switches except for creating confusions. BUG= ========== to ========== Do not switch a connection if the new connection is not ready to send packets. There is no benefit to make such switches. BUG= ==========
honghaiz@webrtc.org changed reviewers: + deadbeef@webrtc.org, pthatcher@webrtc.org
PTAL.
lgtm
By the way, you may also want to change "make" to "making" in the CL description and remove the empty "BUG=". https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (left): https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:3338: EXPECT_EQ(conn2, last_selected_candidate_pair()); Why were these expectations removed? https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:865: } I think these unit tests should move away from accessing "connections_" directly, since it's an implementation detail. Could the tests that depend on this method be rewritten so they don't need it? https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1186: ASSERT_TRUE_WAIT((top_connection = GetTopConnection(ep1_ch1())) != NULL, For example, in this test I think we can do the EXPECTS after the connection is selected (after SetRemoteIceCredentials but before ResumeCandidates). What this test really wants to verify is that a prflx connection can be used before a candidate is signaled, and that its remote candidate is replaced once one is received. https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:3078: // A connection need to be writable before it is selected for transmission. nit: "needs"
Description was changed from ========== Do not switch a connection if the new connection is not ready to send packets. There is no benefit to make such switches. BUG= ========== to ========== Do not switch a connection if the new connection is not ready to send packets. There is no benefit of making such switches. ==========
Patchset #2 (id:80001) has been deleted
I have also updated the CL description. https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (left): https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:3338: EXPECT_EQ(conn2, last_selected_candidate_pair()); On 2016/08/08 23:01:05, Taylor Brandstetter wrote: > Why were these expectations removed? I thought it is kind of redundant with the previous test because both check that Data receiving has higher precedence than priority. But it has slight difference. In this test, both connections are nominated while they are not in the previous one. So I put it back (with a little fix). https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:865: } On 2016/08/08 23:01:05, Taylor Brandstetter wrote: > I think these unit tests should move away from accessing "connections_" > directly, since it's an implementation detail. Could the tests that depend on > this method be rewritten so they don't need it? Done. https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1186: ASSERT_TRUE_WAIT((top_connection = GetTopConnection(ep1_ch1())) != NULL, On 2016/08/08 23:01:05, Taylor Brandstetter wrote: > For example, in this test I think we can do the EXPECTS after the connection is > selected (after SetRemoteIceCredentials but before ResumeCandidates). What this > test really wants to verify is that a prflx connection can be used before a > candidate is signaled, and that its remote candidate is replaced once one is > received. Done. https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:3078: // A connection need to be writable before it is selected for transmission. On 2016/08/08 23:01:05, Taylor Brandstetter wrote: > nit: "needs" Done.
https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1204: EXPECT_EQ(1u, top_connection->remote_candidate().generation()); Previously this tested that a prflx candidate's generation would be updated once ICE credentials are received, allowing it to be replaced by a non-prflx candidate (there used to be bugs for this flow). If that coverage is being removed from these tests, could it be added as a separate test?
https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1204: EXPECT_EQ(1u, top_connection->remote_candidate().generation()); On 2016/08/15 17:55:49, Taylor Brandstetter wrote: > Previously this tested that a prflx candidate's generation would be updated once > ICE credentials are received, allowing it to be replaced by a non-prflx > candidate (there used to be bugs for this flow). If that coverage is being > removed from these tests, could it be added as a separate test? It is more natural to test all these (ufrag, pwd, and generation) at one place when ice_credentials are updated. And if we want to do the same test as the original one (basically the client get the stun ping request before getting any signaling (offer/candidate)), it is best to just peek into the internal state (i.e., connection() and TopConnection) as I did in the first revision. And I do not think it is super-critical to avoid that. WDYT?
https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1204: EXPECT_EQ(1u, top_connection->remote_candidate().generation()); On 2016/08/16 17:29:15, honghaiz3 wrote: > On 2016/08/15 17:55:49, Taylor Brandstetter wrote: > > Previously this tested that a prflx candidate's generation would be updated > once > > ICE credentials are received, allowing it to be replaced by a non-prflx > > candidate (there used to be bugs for this flow). If that coverage is being > > removed from these tests, could it be added as a separate test? > It is more natural to test all these (ufrag, pwd, and generation) at one place > when ice_credentials are updated. And if we want to do the same test as the > original one (basically the client get the stun ping request before getting any > signaling (offer/candidate)), it is best to just peek into the internal state > (i.e., connection() and TopConnection) as I did in the first revision. And I do > not think it is super-critical to avoid that. WDYT? I don't think we need to peek at the internal state for this. If we want to wait until endpoint 1 receives a STUN ping, we can just wait for endpoint 2 to become writable, or even use GetStats.
Patchset #3 (id:120001) has been deleted
PTAL. https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2212683002/diff/60001/webrtc/p2p/base/p2ptransp... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1204: EXPECT_EQ(1u, top_connection->remote_candidate().generation()); On 2016/08/16 18:48:31, Taylor Brandstetter wrote: > On 2016/08/16 17:29:15, honghaiz3 wrote: > > On 2016/08/15 17:55:49, Taylor Brandstetter wrote: > > > Previously this tested that a prflx candidate's generation would be updated > > once > > > ICE credentials are received, allowing it to be replaced by a non-prflx > > > candidate (there used to be bugs for this flow). If that coverage is being > > > removed from these tests, could it be added as a separate test? > > It is more natural to test all these (ufrag, pwd, and generation) at one place > > when ice_credentials are updated. And if we want to do the same test as the > > original one (basically the client get the stun ping request before getting > any > > signaling (offer/candidate)), it is best to just peek into the internal state > > (i.e., connection() and TopConnection) as I did in the first revision. And I > do > > not think it is super-critical to avoid that. WDYT? > > I don't think we need to peek at the internal state for this. If we want to wait > until endpoint 1 receives a STUN ping, we can just wait for endpoint 2 to become > writable, or even use GetStats. Done. True if we only need to check the ICE generation after remote ICE candidate is received.
lgtm
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2212683002/#ps140001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2212683002/#ps160001 (title: "Merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Description was changed from ========== Do not switch a connection if the new connection is not ready to send packets. There is no benefit of making such switches. ========== to ========== Do not switch a connection if the new connection is not ready to send packets. There is no benefit of making such switches. R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/e05bcc22b370ab8458d88807d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:160001) manually as e05bcc22b370ab8458d88807df0122e3e16393ba (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Do not switch a connection if the new connection is not ready to send packets. There is no benefit of making such switches. R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/e05bcc22b370ab8458d88807d... ========== to ========== Do not switch a connection if the new connection is not ready to send packets. There is no benefit of making such switches. R=deadbeef@webrtc.org, pthatcher@webrtc.org Committed: https://crrev.com/e05bcc22b370ab8458d88807df0122e3e16393ba Cr-Commit-Position: refs/heads/master@{#13789} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
