Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1270613006:
First step of passive aggressive nomination. (Closed)
DescriptionFirst step of passive aggressive nomination.
On the controlled side, a stun request without use-candidate attribute will
be used for sending media.
BUG=4900
Committed: https://crrev.com/5a3acd89648e7cff7e1b76b2da710be041be54a0
Cr-Commit-Position: refs/heads/master@{#9747}
Patch Set 1 : #
Total comments: 16
Patch Set 2 : Address comments #
Total comments: 4
Patch Set 3 : #
Total comments: 38
Patch Set 4 : Addressed comments #
Total comments: 1
Messages
Total messages: 28 (13 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
honghaiz@webrtc.org changed reviewers: + juberti@webrtc.org, pthatcher@webrtc.org
Please take a look a the CL. One note: When a connection is chosen as the best connection due to receiving a media packet, the p2p transport channel does not remember the reason, so it may happen that another candidate with a higher priority will change the best connection to a different path. I think that is a transient state. If the controlling side continues sending on the other connection, the best connection (at the controlled side) will switch over again. It should be set with use_candidate finally anyway. Let me know if that is an issue.
On 2015/08/05 23:48:18, honghaiz3 wrote: > Please take a look a the CL. > One note: When a connection is chosen as the best connection due to receiving a > media packet, the p2p transport channel does not remember the reason, so it may > happen that another candidate with a higher priority will change the best > connection to a different path. I think that is a transient state. If the > controlling side continues sending on the other connection, the best connection > (at the controlled side) will switch over again. It should be set with > use_candidate finally anyway. Let me know if that is an issue. It's not clear to me that we need received_use_candidate, since we already have pending_best_connection_ that tracks which connection the remote side wants to use. Could you explain the thinking here a bit more?
On 2015/08/06 00:06:39, juberti1 wrote: > On 2015/08/05 23:48:18, honghaiz3 wrote: > > Please take a look a the CL. > > One note: When a connection is chosen as the best connection due to receiving > a > > media packet, the p2p transport channel does not remember the reason, so it > may > > happen that another candidate with a higher priority will change the best > > connection to a different path. I think that is a transient state. If the > > controlling side continues sending on the other connection, the best > connection > > (at the controlled side) will switch over again. It should be set with > > use_candidate finally anyway. Let me know if that is an issue. > > It's not clear to me that we need received_use_candidate, since we already have > pending_best_connection_ that tracks which connection the remote side wants to > use. Could you explain the thinking here a bit more? pending_best_connection_ only exists for a short period between receiving a use_candidate and the time when the connection becomes writable. If a connection is writable when a request with use_candidate arrives, p2ptransportchannel will not set pending_best_connection_ at all. Previously at the controlled side, a connection is set to best_connection only if it is nominated by the controlling side via use_candidate. Now the best_connection might be nominated or just because it is the current best choice based on the controlled side's information. When the controlled side has a new connection (not via use_candidate), it needs to know whether the current best connection is nominated (and pending_best_connection only remembers this until the best connection becomes writable). If the current best connection is nominated, the controlled side will not try to switch the best connection based on its own priority selection, and vice versa.
Thanks for getting it out so quickly. I still need to look at the unit tests. https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:596: connection->set_received_use_candidate(true); I think we should call this set_nominated. https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:599: OnUseCandidate(connection); We can call OnUseCandidate *before* calling AddConnection? That seems backwards. I was thinking that instead of calling OnUseCandidate here, we would have AddConnection do something like: if (connection->nominated()) { OnUseCandidate(connection); } https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1039: !(best_connection_ && best_connection_->received_use_candidate()))) { I think a best_nominated_connection() == (best_connection_ && best_connection_->nominated()) ? best_connection_ : nullptr would be helpful here. Then this could just be: if (protocol_type_ != ICEPROTO_RFC5245 || ice_role_ = ICEROLE_CONTROLLING || !best_nominated_connection()) { // ... } https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1040: if (ShouldSwitch(best_connection_, top_connection)) { I think we should move all of this logic together into ShouldSwitch (checking the role and if best_connection_ is nominated) https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1060: (best_connection_ && best_connection_->received_use_candidate())) { I think we want to make this if (best_connection_nominated()) { PruneLowerPriorityCandidates(); } 1. Having a helper method would make this more readable. 2. The controlling side should mark the best_connection_ as nominated when it nominates it. Then the logic is very easy to read. https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1441: connection->writable()) { I think if this would be easier to read: if (ice_role_ == ICEROLE_CONTROLLED && connection != best_connection_nominated()) { SwitchBestConnectionTo(connection) } https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:510: received_use_candidate_ = received; I think nominated_ would be better. https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:607: // although we are moving toward passive-aggressive nomination. I don't think this comment change is necessary.
Patchset #2 (id:140001) has been deleted
Thanks! PTAL. https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:596: connection->set_received_use_candidate(true); On 2015/08/06 01:41:42, pthatcher1 wrote: > I think we should call this set_nominated. Done. https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:599: OnUseCandidate(connection); On 2015/08/06 01:41:42, pthatcher1 wrote: > We can call OnUseCandidate *before* calling AddConnection? That seems > backwards. I was thinking that instead of calling OnUseCandidate here, we > would have AddConnection do something like: > > if (connection->nominated()) { > OnUseCandidate(connection); > } I move this after AddConnection. Plus, hid the call to set_nominated() method. https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1039: !(best_connection_ && best_connection_->received_use_candidate()))) { On 2015/08/06 01:41:42, pthatcher1 wrote: > I think a best_nominated_connection() == (best_connection_ && > best_connection_->nominated()) ? best_connection_ : nullptr would be helpful > here. > > Then this could just be: > > if (protocol_type_ != ICEPROTO_RFC5245 || ice_role_ = ICEROLE_CONTROLLING || > !best_nominated_connection()) { > // ... > } Done. https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1040: if (ShouldSwitch(best_connection_, top_connection)) { On 2015/08/06 01:41:42, pthatcher1 wrote: > I think we should move all of this logic together into ShouldSwitch (checking > the role and if best_connection_ is nominated) ShouldSwitch is defined as a helper function outside of the class. Unless we want to change it to a member function of the class, we cannot check the role there. I tried to change that to a member function and tried to add a separate function for it. But at the end, I feel it is best to just leave it there, especially if you see in the next lines we have had similar conditions (if you put some tests in a separate function and some tests here, it does not improve readability much. https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1060: (best_connection_ && best_connection_->received_use_candidate())) { On 2015/08/06 01:41:42, pthatcher1 wrote: > I think we want to make this > > if (best_connection_nominated()) { > PruneLowerPriorityCandidates(); > } > > 1. Having a helper method would make this more readable. > > 2. The controlling side should mark the best_connection_ as nominated when it > nominates it. Then the logic is very easy to read. Done. I think calling it PruneConnections is probably enough. When you prune connections, you only prune low priority or equal priority ones. https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1441: connection->writable()) { On 2015/08/06 01:41:42, pthatcher1 wrote: > I think if this would be easier to read: > > if (ice_role_ == ICEROLE_CONTROLLED && connection != > best_connection_nominated()) { > SwitchBestConnectionTo(connection) > } That is different from what I put. If the connection is not writable but receiving data, we should not use it to send. If the connection and best_connection is equal but the best_connection is not nominated, we should not (need not) switch. I put the best_connection_ != connection as the first test as most of time this will return false and shortcut the rest of the tests. https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:510: received_use_candidate_ = received; On 2015/08/06 01:41:42, pthatcher1 wrote: > I think nominated_ would be better. Done. https://codereview.webrtc.org/1270613006/diff/120001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:607: // although we are moving toward passive-aggressive nomination. On 2015/08/06 01:41:42, pthatcher1 wrote: > I don't think this comment change is necessary. Done.
I like the new patch set. With this change, can we eliminate pending_best_connection? I think that logic is now largely redundant. https://codereview.webrtc.org/1270613006/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1270613006/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1025: // Note that top_connection don't have to be writable to be don't ->doesn't https://codereview.webrtc.org/1270613006/diff/160001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1270613006/diff/160001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1480: SignalUseCandidate(this); This seems like an unexpected side effect. I would suggest that the code that calls set_nominated just invoke the relevant OnUseCandidate logic directly.
Patchset #3 (id:180001) has been deleted
There is some difference between with and without pending_best_connection. Without pending_best_connection, we can now check whether a connection is nominated. If it becomes writable and it has been nominated, then we set it to the best connection. But pending_best_connection remembers the last nominated connection. If both con1 and con2 are nominated in the order but not writable yet. Only con2 will be set to the best connection when it becomes writable. Without pending_best_connection, both will be set as the best_connection, The one which becomes writable later will win. That's the difference. However, if the controlling side will keep on sending stun request only on the best connection with use_candidate, then the difference is minor and we are OK to remove pending_best_connection. https://codereview.webrtc.org/1270613006/diff/160001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1270613006/diff/160001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1025: // Note that top_connection don't have to be writable to be On 2015/08/07 01:31:19, juberti1 wrote: > don't ->doesn't Done. https://codereview.webrtc.org/1270613006/diff/160001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/1270613006/diff/160001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1480: SignalUseCandidate(this); On 2015/08/07 01:31:19, juberti1 wrote: > This seems like an unexpected side effect. I would suggest that the code that > calls set_nominated just invoke the relevant OnUseCandidate logic directly. Done.
Mostly style things https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:602: protocol_type_ == ICEPROTO_RFC5245) { We are already in a "== ICEPROTO_RFC5245" block, so you can remove that. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:604: OnUseCandidate(connection); Firing OnUseCandidate after we call AddConnection is kind of nice. While you're there, do you think you could rename OnUseCandidate to be OnNominated? It would match so much better now. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1024: // Note that top_connection doesn't have to be writable to be |top_connection| https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1025: // switched to although it will have higher priority if it is writable. to be switched to => to become the best_connection_ https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1027: // was not nominated yet. Controlled side => The controlled side if the best connection was not nominated yet => if the current |best_connection_| has not been nominated by the controlling side. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1080: // use_candidate yet, do not prune the connections because it may delete the I'd change "it has not received requests with use_candidate yet" to "and the controlling side has not nominated a connection yet", https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1081: // connection that will be selected by the controlling side. Shouldn't you move this NOTE to where we call PruneConnections, since that is where the code to not prune is? https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1086: networks.insert(connections_[i]->port()->Network()); Might as well switch go c++11: for (const Connection* conn : connections_) { networks.insert(conn->port()->Network()); } https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1087: for (auto network : networks) { I'd prefer "for (const Network* network : networks)". https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1090: continue; Can you add some {}s around this? https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1092: for (auto connection : connections_) { Same here: for (Connection* conn : connections_) { } https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1440: !best_nominated_connection() && connection->writable()) { I think the order would be a little more clear as: if (ice_role_ == ICEROLE_CONTROLLED && !best_nominated_connection() && connection->writable() && best_connection_ != connection) { } https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1888: // When the p2p transport channel with a controlled role received a candidate, You could just shorten this to 'The controlled side will select a connection as the "best connection" based on priority until the controlling side nominates a connection, at which point the controlled side will select that connection as the "best connection". https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1893: TEST_F(P2PTransportChannelPingTest, TestPassiveAggressiveNomination) { It's not yet passive aggressive nomination. It's more like "SelectConnectionBeforeNominationBaseOnPrioirty" https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1923: conn3->SignalUseCandidate(conn3); Maybe we should just fire SignalNominated within set_nominated. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1948: // will be treated as a nomination from the controlling side. I think you could shorten this to "The controlled side will select a connection before the controlled side nominates peer reflexive candidates, and will honor nominations from peer reflexive candidates." https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1949: TEST_F(P2PTransportChannelPingTest, TestReceivingUnknownAddress) { This is kind of two tests in one: SelectConnectionBeforeNominationWithPeerReflexCandidates and HonorNominatedPeerReflexCandidates. Maybe just TestNominationOfPeerReflexCandidates. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2013: // it will not switch the best connection (the sending path). This could be shortened to 'The controlled side will select a connection as the "best connection" based on media received until the controlling side nominates a connection, at which point the controlled side will select that connection as the "best connection". https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2014: TEST_F(P2PTransportChannelPingTest, TestSendingPathMirrorReceivingPath) { This might be called SelectConnectionBeforeNominationBaseOnMediaReceived
Patchset #4 (id:220001) has been deleted
Thanks! PTAL. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:602: protocol_type_ == ICEPROTO_RFC5245) { On 2015/08/07 22:50:22, pthatcher1 wrote: > We are already in a "== ICEPROTO_RFC5245" block, so you can remove that. Done. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:604: OnUseCandidate(connection); On 2015/08/07 22:50:22, pthatcher1 wrote: > Firing OnUseCandidate after we call AddConnection is kind of nice. > > While you're there, do you think you could rename OnUseCandidate to be > OnNominated? It would match so much better now. Done. Also changed the SignalUseCandidate to SignalNominated. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1024: // Note that top_connection doesn't have to be writable to be On 2015/08/07 22:50:22, pthatcher1 wrote: > |top_connection| Done. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1025: // switched to although it will have higher priority if it is writable. On 2015/08/07 22:50:22, pthatcher1 wrote: > to be switched to => to become the best_connection_ Done. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1027: // was not nominated yet. On 2015/08/07 22:50:22, pthatcher1 wrote: > Controlled side => The controlled side > if the best connection was not nominated yet => if the current > |best_connection_| has not been nominated by the controlling side. Done. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1080: // use_candidate yet, do not prune the connections because it may delete the On 2015/08/07 22:50:22, pthatcher1 wrote: > I'd change "it has not received requests with use_candidate yet" to "and the > controlling side has not nominated a connection yet", Done. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1081: // connection that will be selected by the controlling side. On 2015/08/07 22:50:22, pthatcher1 wrote: > Shouldn't you move this NOTE to where we call PruneConnections, since that is > where the code to not prune is? Done. And merged with the comments there. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1086: networks.insert(connections_[i]->port()->Network()); On 2015/08/07 22:50:22, pthatcher1 wrote: > Might as well switch go c++11: > > for (const Connection* conn : connections_) { > networks.insert(conn->port()->Network()); > } Done. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1087: for (auto network : networks) { On 2015/08/07 22:50:22, pthatcher1 wrote: > I'd prefer "for (const Network* network : networks)". Done. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1090: continue; On 2015/08/07 22:50:22, pthatcher1 wrote: > Can you add some {}s around this? Done. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1092: for (auto connection : connections_) { On 2015/08/07 22:50:22, pthatcher1 wrote: > Same here: > > for (Connection* conn : connections_) { > } Done. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1440: !best_nominated_connection() && connection->writable()) { On 2015/08/07 22:50:22, pthatcher1 wrote: > I think the order would be a little more clear as: > > if (ice_role_ == ICEROLE_CONTROLLED && > !best_nominated_connection() && > connection->writable() && > best_connection_ != connection) { > } Done. I put it in that order because I thought in most cases, best_connection_ will be equal to connection so the rest tests need not be checked. But it looks like that the readability is more important. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1888: // When the p2p transport channel with a controlled role received a candidate, On 2015/08/07 22:50:22, pthatcher1 wrote: > You could just shorten this to 'The controlled side will select a connection as > the "best connection" based on priority until the controlling side nominates a > connection, at which point the controlled side will select that connection as > the "best connection". Done. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1893: TEST_F(P2PTransportChannelPingTest, TestPassiveAggressiveNomination) { On 2015/08/07 22:50:22, pthatcher1 wrote: > It's not yet passive aggressive nomination. It's more like > "SelectConnectionBeforeNominationBaseOnPrioirty" Done. Shortened the name a little bit. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1923: conn3->SignalUseCandidate(conn3); On 2015/08/07 22:50:22, pthatcher1 wrote: > Maybe we should just fire SignalNominated within set_nominated. That's what I did in a previous patch. But Justin preferred this way. Justin, with the name changes of the signal, do you think it is reasonable to put the SignalNominated inside set_nominated? https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1948: // will be treated as a nomination from the controlling side. On 2015/08/07 22:50:22, pthatcher1 wrote: > I think you could shorten this to "The controlled side will select a connection > before the controlled side nominates peer reflexive candidates, and will honor > nominations from peer reflexive candidates." Done. Thanks! https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1949: TEST_F(P2PTransportChannelPingTest, TestReceivingUnknownAddress) { On 2015/08/07 22:50:22, pthatcher1 wrote: > This is kind of two tests in one: > SelectConnectionBeforeNominationWithPeerReflexCandidates and > HonorNominatedPeerReflexCandidates. Maybe just > TestNominationOfPeerReflexCandidates. I think this is more on nominations from Unknown Address. I just happened to choose the peer reflexive priority for the request although that should not matter that much (I want it to be higher than the one in nominated to test that even if the nominated one has lower priority, it should still be selected). Revised names and comments a little bit (all three added tests have name SelectionConnection.... https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2013: // it will not switch the best connection (the sending path). On 2015/08/07 22:50:22, pthatcher1 wrote: > This could be shortened to 'The controlled side will select a connection as the > "best connection" based on media received until the controlling side nominates a > connection, at which point the controlled side will select that connection as > the "best connection". Done. https://codereview.webrtc.org/1270613006/diff/200001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2014: TEST_F(P2PTransportChannelPingTest, TestSendingPathMirrorReceivingPath) { On 2015/08/07 22:50:22, pthatcher1 wrote: > This might be called SelectConnectionBeforeNominationBaseOnMediaReceived Shortened a little bit. Since the comments have described the tests in more details, guess don't have to make the name capture everything.
Patchset #4 (id:240001) has been deleted
Patchset #4 (id:260001) has been deleted
https://codereview.webrtc.org/1270613006/diff/280001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/1270613006/diff/280001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:488: void set_nominated(bool nominated) { nominated_ = nominated; } Should set_nominated fire SignalNominated?
lgtm
Justin, Do you want to take another look at the CL? If you do not have time to look at it, I may submit it tomorrow (Thursday).
The CQ bit was checked by honghaiz@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1270613006/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1270613006/280001
Message was sent while issue was closed.
Committed patchset #4 (id:280001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/5a3acd89648e7cff7e1b76b2da710be041be54a0 Cr-Commit-Position: refs/heads/master@{#9747} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
