Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2025573002:
Use continual gathering to restore backup connections (Closed)
DescriptionIf continual gathering is enabled,
we will periodically check if any network does not have any connection on it and if yes, attempt to re-gather on those networks.
BUG=
R=pthatcher@webrtc.org
Committed: https://crrev.com/5622c5eae547dc079763865a31d7da951bb9cd76
Cr-Commit-Position: refs/heads/master@{#13367}
Patch Set 1 : #Patch Set 2 : Add tests #
Total comments: 18
Patch Set 3 : Address Taylor comments #
Total comments: 34
Patch Set 4 : Review #Patch Set 5 : . #Patch Set 6 : . #
Total comments: 8
Patch Set 7 : . #Patch Set 8 : Merge branch 'master' into enhanced_cg #
Total comments: 14
Patch Set 9 : Address comments and revised tests #
Total comments: 19
Patch Set 10 : Added SessionState so that it won't regather if it is currently gathering. #Patch Set 11 : Merge #Patch Set 12 : Merge branch 'master' into enhanced_cg #Patch Set 13 : Fix a win-compiler warning #Patch Set 14 : Merge again #
Total comments: 4
Patch Set 15 : Change INPROGRESS to GATHERING, mark port has no pairable candidates if its candidates are removed. #Patch Set 16 : Merge again. #Patch Set 17 : Remove candidates on pruned ports when network failed #Patch Set 18 : Merge #
Messages
Total messages: 69 (36 generated)
Description was changed from ========== If continual gathering is enabled, re-gathering if the best connection is deleted. BUG= ========== to ========== If continual gathering is enabled, 1. re-gathering if the best connection is deleted. 2. Periodically check if there are any networks that do not have any connection and if yes, attempt to re-gathering on those networks. BUG= ==========
Description was changed from ========== If continual gathering is enabled, 1. re-gathering if the best connection is deleted. 2. Periodically check if there are any networks that do not have any connection and if yes, attempt to re-gathering on those networks. BUG= ========== to ========== If continual gathering is enabled, 1. re-gathering if the best connection is deleted and there is no other non-failed connections. 2. Periodically check if there are any networks that do not have any connection on it and if yes, attempt to re-gathering on those networks. BUG= ==========
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: + deadbeef@webrtc.org, pthatcher@webrtc.org
I tested this by manually removing all or backup connections. I am working on the test. It will be great if you can start to look at the CL.
Patchset #2 (id:130001) has been deleted
Added tests. PTAL.
Just a friendly reminder for the review.
https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1261: // external world. When you say "external world", do you mean applications (so they see ICE "connected" instead of "disconnected"), or do you mean the audio/video engines? I think in this situation, an application *should* see "disconnected". If they've turned continual gathering on, they should expect this, and just wait for it to resolve itself, and maybe display a message to the user in the meantime. https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1584: // if the best connecion was removed and it was writable. If it was not "connecion" => "connection" https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1638: candidates_removed_.push_back(candidate); Something I didn't notice until now: This needs to sanitize candidates just like BasicPortAllocator does. By which I mean: don't signal candidates that don't match the candidate filter, and zero out the "related address" if necessary. See: https://cs.chromium.org/chromium/src/third_party/webrtc/p2p/client/basicporta... If we don't do this, then we'll end up leaking IPs in the "candidate removed" event. https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:207: void set_check_restore_backup_connection_interval(int interval); Can you add a TODO to remove this method and use the fake clock instead to test this (which can instantly be advanced by 5 minutes)? https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:213: // regathering on all active networks. "regathering" => "start regathering" https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:172: virtual void InactivateNetworksInExistingSequences( Should probably be "Deactivate". https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:218: void BasicPortAllocatorSession::InactivateNetworksInExistingSequences( On second thought... If this method is always intended to be used along with "StartGettingPorts" in order to re-gather ports for specific networks, it would be better to just call the method "RegatherPortsForNetworks()". That creates a looser dependency on the implementation of the PortAllocator; developers may implement their own PortAllocator, where it's not clear what they're supposed to do when "InactivateNetworks" is called. Also, document in the header file what a nullptr parameter means. https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.h (right): https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.h:48: void GetNetworks(std::vector<rtc::Network*>* networks) const override { Instead of going through BasicPortAllocator, P2PTransportChannel should just use the network manager directly.
By the way, haven't reviewed unit tests yet.
Thanks! PTAL. https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1261: // external world. On 2016/06/06 18:18:37, Taylor Brandstetter wrote: > When you say "external world", do you mean applications (so they see ICE > "connected" instead of "disconnected"), or do you mean the audio/video engines? > I think in this situation, an application *should* see "disconnected". If > they've turned continual gathering on, they should expect this, and just wait > for it to resolve itself, and maybe display a message to the user in the > meantime. I meant the TransportController and the layers above. If the application see the "disconnected", it will try to kick off "full ICE restart", which we don't want it to happen when a continual gathering may resolve the issue. I added a TODO to expose the recovering state the app, although the app won't do much about it except that "it maybe display a message to the user". https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1584: // if the best connecion was removed and it was writable. If it was not On 2016/06/06 18:18:37, Taylor Brandstetter wrote: > "connecion" => "connection" Done. https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1638: candidates_removed_.push_back(candidate); On 2016/06/06 18:18:37, Taylor Brandstetter wrote: > Something I didn't notice until now: This needs to sanitize candidates just like > BasicPortAllocator does. By which I mean: don't signal candidates that don't > match the candidate filter, and zero out the "related address" if necessary. > See: > https://cs.chromium.org/chromium/src/third_party/webrtc/p2p/client/basicporta... > > If we don't do this, then we'll end up leaking IPs in the "candidate removed" > event. Done. https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:207: void set_check_restore_backup_connection_interval(int interval); On 2016/06/06 18:18:37, Taylor Brandstetter wrote: > Can you add a TODO to remove this method and use the fake clock instead to test > this (which can instantly be advanced by 5 minutes)? Done. https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:213: // regathering on all active networks. On 2016/06/06 18:18:37, Taylor Brandstetter wrote: > "regathering" => "start regathering" Done. https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:172: virtual void InactivateNetworksInExistingSequences( On 2016/06/06 18:18:37, Taylor Brandstetter wrote: > Should probably be "Deactivate". I looked it up in the online dictionary and googled these two words. It appears that both are valid words and have similar meanings. See an example discussion here. http://english.stackexchange.com/questions/568/is-inactivate-really-a-word I slightly prefer inactivate because it has the same prefix as inactive, making me feel it is more consistent with the usage of variable "inactive". https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:218: void BasicPortAllocatorSession::InactivateNetworksInExistingSequences( On 2016/06/06 18:18:37, Taylor Brandstetter wrote: > On second thought... If this method is always intended to be used along with > "StartGettingPorts" in order to re-gather ports for specific networks, it would > be better to just call the method "RegatherPortsForNetworks()". That creates a > looser dependency on the implementation of the PortAllocator; developers may > implement their own PortAllocator, where it's not clear what they're supposed to > do when "InactivateNetworks" is called. > > Also, document in the header file what a nullptr parameter means. Done, except the name. We are not re-gathering "port". What it does is to create ports and re-gathering candidates. I use the name GetPortsOnNetworks (similar to StartGettingPorts). https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.h (right): https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.h:48: void GetNetworks(std::vector<rtc::Network*>* networks) const override { On 2016/06/06 18:18:37, Taylor Brandstetter wrote: > Instead of going through BasicPortAllocator, P2PTransportChannel should just use > the network manager directly. Done.
https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1261: // external world. On 2016/06/07 16:42:06, honghaiz3 wrote: > On 2016/06/06 18:18:37, Taylor Brandstetter wrote: > > When you say "external world", do you mean applications (so they see ICE > > "connected" instead of "disconnected"), or do you mean the audio/video > engines? > > I think in this situation, an application *should* see "disconnected". If > > they've turned continual gathering on, they should expect this, and just wait > > for it to resolve itself, and maybe display a message to the user in the > > meantime. > > I meant the TransportController and the layers above. > If the application see the "disconnected", it will try to kick off "full ICE > restart", which we don't want it to happen when a continual gathering may > resolve the issue. I added a TODO to expose the recovering state the app, > although the app won't do much about it except that "it maybe display a message > to the user". If the application is using continual gathering, it should know that "disconnected" can resolve itself. It should expect a sequence of these states: connected/not gathering -> disconnected/gathering -> checking/not gathering -> connected/not gathering If at the end, the state is "disconnected/not gathering", *then* the application may want to do a full ICE restart. However, we don't even switch to "checking" properly right now. So I'm ok with the code as it is now. Just be aware that we may need to change it later. https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2025573002/diff/150001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:172: virtual void InactivateNetworksInExistingSequences( On 2016/06/07 16:42:06, honghaiz3 wrote: > On 2016/06/06 18:18:37, Taylor Brandstetter wrote: > > Should probably be "Deactivate". > I looked it up in the online dictionary and googled these two words. It appears > that both are valid words and have similar meanings. See an example discussion > here. > http://english.stackexchange.com/questions/568/is-inactivate-really-a-word > I slightly prefer inactivate because it has the same prefix as inactive, making > me feel it is more consistent with the usage of variable "inactive". Acknowledged.
I haven't checked the unit tests yet. The "***"s are notes to myself to come back and try and understand those parts. In general, I think we need a more clear division of labor between the P2pTransportChannel (aka ORTC IceTransport) and the PortAllocator (sort of like the ORTC IceGatherer). Right now it's kind of messy. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:33: MSG_CHECK_RESTORE_BACKUP_CONNECTION, It seems like a better name would be MSG_REGATHER_FAILED_NETWORKS https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:34: MSG_SIGNAL_CANDIDATES_REMOVED Why does this require a thread post? To coalesce candidate removals? https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:234: static const int CHECK_RESTORE_BACKUP_CONNECTION_INTERVAL = 5 * 60 * 1000; I think a better name would be DEFAULT_REGATHER_FAILED_NETWORKS_INTERVAL.? https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:259: CHECK_RESTORE_BACKUP_CONNECTION_INTERVAL), Can you put this in config_ instead of as its own thing? https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:297: candidates_removed_.clear(); This feels like implicit magic to me. Could we instead know when we're removing candidates because of an ICE restart and then suppress them from being signaled in the first place instead of clearing them here? https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:361: if (active_connections.empty() && !IsRecoveringConnectivity()) { I don't like the look of the complexity added to hide the disconnected state from the application. I think it would be better instead to keep it simple and just expose the disconnected state to applications that opt into continual gathering. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1387: allocator_->network_manager()->GetNetworks(&networks); I'm not a fan of reaching into the allocator like this. Would it make sense, instead, to have a GetFailedNetworks method on the Allocator? The Allocator has the Ports. The Ports have the Connections. The Connections have the states. So the Allocator should know which ports have non-failed connections. I think the code would end up being more simple and would have a more clear division of labor between the P2pTransportChannel and the PortAllocator. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1408: networks.erase(network_end, networks.end()); This logic seems a bit complicated. Would it make more sense to have one std::remove_if with networks, checking if any non-failed connection has a matching network name? https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1411: } It seems like a lot of this logic should be in a separate methods called RegatherNetworksWithAllFailedConnections or RegatherFailedNetworks, unless we move most of it to PortAllocator::GetFailedNetworks(), in which case it would be StartRegathering(allocator->GetFailedNetworks()). In fact, why even have StartRegathering logic in P2pTransportChannel? Why not have that in PortAllocator? Why not have the method be PortAllocator::RegatherFailedNetworks()? That would seem even more clear to me. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1484: void P2PTransportChannel::set_check_restore_backup_connection_interval( It sounds like this method should be called RegatherFailedNetworksEvery(int interval). https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1545: } *** https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1595: recovering_start_time_ = rtc::TimeMillis(); Should this be last_regather_time_, and should it be set inside of StartRegathering? https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1597: } Can we put this in a separate methods called MaybeRegather() or RegatherIfAllConnectionsJustFailed()? https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1635: port->FailAndDestroyConnections(); It feels like this should be Close(), which should then fire SignalCandidatesRemoved, which we can listen to. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1643: thread()->Post(this, MSG_SIGNAL_CANDIDATES_REMOVED); Why does this need a thread hop? https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1670: OnPortNetworkInactive(port); I feel like we're confounding two things by triggering OnPortNetworkInactive directly here. If we have a different signal and that triggers similar behavior, I think that's OK. But confounding the two gets too complex. For example, we could, here, call port->Close() which would then fire a port->SignalCandidatesRemoved, which we could listen to. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:48: static const int MAX_RECOVERING_TIME = 5000; // 5 seconds. I think this needs a more clear name. This isn't a time limit on recovering, it's a time limit on how long we hide the fact that we're not connected from the application. So perhaps something like CONTINUAL_GATHERING_DISCONNECTED_STATE_DELAY. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:208: void set_check_restore_backup_connection_interval(int interval); Can we just use the fake clock now? This looks kind of ugly. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:215: void StartRegathering(std::vector<rtc::Network*>* networks); It think it would be more clear to have two methods: RegatherAllNetworks() RegatherNetworks(const std::vector<rtc::Network*>& networks) https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:311: } *** https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:175: virtual void GetPortsOnNetworks(const std::vector<rtc::Network*>* networks) {} I think this would be more clear with two methods: virtual void RegatherAllNetworks() virtual void RegatherNetworks(const std::vector<rtc::Network*>& networks) https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:276: virtual rtc::NetworkManager* network_manager() const = 0; *** https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/portinte... File webrtc/p2p/base/portinterface.h (right): https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/portinte... webrtc/p2p/base/portinterface.h:126: virtual void FailAndDestroyConnections() = 0; *** https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:227: sequence->OnNetworkInactivated(); It feels like we are confounding two separate things: 1. The network went away. 2. We want to stop the current sequence and restart it as if the network went away and came back. I think instead of calling OnNetworkInactivitated, we should have a method that's something like Restart(). And the implementation of that could be to do the same thing as OnNetworkRemoved, but method we call here should be Restart(), I think. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:231: StartGettingPorts(); And the Restart() above would mean we don't need to call StartGettingPorts here, I think. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:262: std::vector<Candidate> BasicPortAllocatorSession::ReadyCandidates( Is it really so bad if the caller calls ReadyCandidates(), gets them all, and just filters for one port? It seems like it would be more simple and would be a very minor perf difference. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:532: if (!sequence->network_inactive() && I liked calling this removed before and I'd like to see if we can leave it so, and find a different way to restart a sequence without confounding these two things.
https://codereview.webrtc.org/2025573002/diff/230001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2025573002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1765: DestroyChannels(); Can you explain why this is needed? Shouldn't the destructor of the test clean everything up? https://codereview.webrtc.org/2025573002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:1905: ep2_ch1()->SetIceConfig(CreateIceConfig(1000, true)); Would be nice to have a "CreateContinualGatheringConfig" helper method, since the bool parameter is not obvious. https://codereview.webrtc.org/2025573002/diff/230001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2059: 5000); Why 5000 specifically? https://codereview.webrtc.org/2025573002/diff/230001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2025573002/diff/230001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:29: } // namespace rtc Is this still needed? https://codereview.webrtc.org/2025573002/diff/230001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:175: virtual bool RegatherOnFailedNetworks() {} Default implementation should return false. https://codereview.webrtc.org/2025573002/diff/230001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:278: virtual rtc::NetworkManager* network_manager() const = 0; I didn't notice this before, but the PortAllocator should not be exposing its network manager. It's an implementation detail of BasicPortAllocator. Is this still used by anything? https://codereview.webrtc.org/2025573002/diff/230001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2025573002/diff/230001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:200: running_ = STATE_RUNNING; Should be "state_ = STATE_RUNNING"? Also can "running_" be removed, if state_ is being used instead? https://codereview.webrtc.org/2025573002/diff/230001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:884: network_inactive_ = true; nit: Would be better to store this bool as "network_active_"
I had another thought about this. Assuming "PortAllocator" becomes the "IceGatherer", would it make more sense to have the PortAllocator completely handle the re-gathering by itself? Instead of P2PTransportChannel keeping a timer and telling PortAllocator to regather on necessary networks, the PortAllocator could just do this on its own.
Patchset #7 (id:250001) has been deleted
Patchset #7 (id:270001) has been deleted
Patchset #7 (id:290001) has been deleted
Patchset #7 (id:310001) has been deleted
Patchset #7 (id:330001) has been deleted
Patchset #7 (id:350001) has been deleted
I finally re-wrote this CL and put most of the logic in the BasicPortAllocatorSession although the controlling is at the p2ptransportchannel https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:33: MSG_CHECK_RESTORE_BACKUP_CONNECTION, On 2016/06/07 18:54:33, pthatcher1 wrote: > It seems like a better name would be MSG_REGATHER_FAILED_NETWORKS MSG_REGATHER_ON_FAILED_NETWORKS better? https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:34: MSG_SIGNAL_CANDIDATES_REMOVED On 2016/06/07 18:54:33, pthatcher1 wrote: > Why does this require a thread post? To coalesce candidate removals? Yes. We can merge multiple candidate removals into one. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1408: networks.erase(network_end, networks.end()); On 2016/06/07 18:54:33, pthatcher1 wrote: > This logic seems a bit complicated. Would it make more sense to have one > std::remove_if with networks, checking if any non-failed connection has a > matching network name? Done. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1411: } On 2016/06/07 18:54:33, pthatcher1 wrote: > It seems like a lot of this logic should be in a separate methods called > RegatherNetworksWithAllFailedConnections or RegatherFailedNetworks, unless we > move most of it to PortAllocator::GetFailedNetworks(), in which case it would be > StartRegathering(allocator->GetFailedNetworks()). > > In fact, why even have StartRegathering logic in P2pTransportChannel? Why not > have that in PortAllocator? Why not have the method be > PortAllocator::RegatherFailedNetworks()? That would seem even more clear to > me. Done. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1595: recovering_start_time_ = rtc::TimeMillis(); On 2016/06/07 18:54:33, pthatcher1 wrote: > Should this be last_regather_time_, and should it be set inside of > StartRegathering? Done. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1597: } On 2016/06/07 18:54:33, pthatcher1 wrote: > Can we put this in a separate methods called MaybeRegather() or > RegatherIfAllConnectionsJustFailed()? Removed. https://codereview.webrtc.org/2025573002/diff/170001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1643: thread()->Post(this, MSG_SIGNAL_CANDIDATES_REMOVED); On 2016/06/07 18:54:33, pthatcher1 wrote: > Why does this need a thread hop? We can combine multiple candidate removal signals into one.
Description was changed from ========== If continual gathering is enabled, 1. re-gathering if the best connection is deleted and there is no other non-failed connections. 2. Periodically check if there are any networks that do not have any connection on it and if yes, attempt to re-gathering on those networks. BUG= ========== to ========== If continual gathering is enabled, p2ptransportchannel will periodically check if there are any networks that do not have any connection on it and if yes, attempt to re-gather on those networks. BUG= ==========
You may want to start look at this CL. Please focus on the non-test code for now as I may do some small improvement on the testing code (the tests all pass though).
Description was changed from ========== If continual gathering is enabled, p2ptransportchannel will periodically check if there are any networks that do not have any connection on it and if yes, attempt to re-gather on those networks. BUG= ========== to ========== If continual gathering is enabled, we will periodically check if any network does not have any connection on it and if yes, attempt to re-gather on those networks. BUG= ==========
This looks much better. I haven't check the unit tests yet. It's all nits, except I don't understand how this change replaces the need to listen to a network going inactive. https://codereview.webrtc.org/2025573002/diff/390001/webrtc/api/webrtcsession.cc File webrtc/api/webrtcsession.cc (right): https://codereview.webrtc.org/2025573002/diff/390001/webrtc/api/webrtcsession... webrtc/api/webrtcsession.cc:1151: -1 /* regather_on_failed_networks_interval */); This seems less readable to me. Setting them each, one per line, seemed more readable, especially for values that you just keep the default (like the stable writable connection ping interval). https://codereview.webrtc.org/2025573002/diff/390001/webrtc/api/webrtcsession... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/2025573002/diff/390001/webrtc/api/webrtcsession... webrtc/api/webrtcsession_unittest.cc:2246: false, -1, true, -1)); Since these are almost all the defaults, why not use cricket::IceConfig() and just set the ones that aren't defaults? https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:362: config.regather_on_failed_networks_interval != Can we use an rtc::optional in config rather than a -1? https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:389: MSG_REGATHER_ON_FAILED_NETWORKS); Taylor just refactored this :) https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (left): https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:198: network_->SignalInactive.connect(this, &Port::OnNetworkInactive); Why do we no longer listen to a network being inactive? https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:221: // A network interface may have both Ipv4 and IPv6 networks. Only if Ipv or IPv? Please be consistent. https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:224: std::map<std::string, bool> network_has_connection; Wouldn't a std::set work? Then just check if it's in the set.
Revised tests and addressed comments. We now handle the network inactive in allocator session and trigger ports and candidates removing from allocator session. https://codereview.webrtc.org/2025573002/diff/390001/webrtc/api/webrtcsession.cc File webrtc/api/webrtcsession.cc (right): https://codereview.webrtc.org/2025573002/diff/390001/webrtc/api/webrtcsession... webrtc/api/webrtcsession.cc:1151: -1 /* regather_on_failed_networks_interval */); On 2016/06/29 18:53:27, pthatcher1 wrote: > This seems less readable to me. Setting them each, one per line, seemed more > readable, especially for values that you just keep the default (like the stable > writable connection ping interval). Done. https://codereview.webrtc.org/2025573002/diff/390001/webrtc/api/webrtcsession... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/2025573002/diff/390001/webrtc/api/webrtcsession... webrtc/api/webrtcsession_unittest.cc:2246: false, -1, true, -1)); On 2016/06/29 18:53:27, pthatcher1 wrote: > Since these are almost all the defaults, why not use cricket::IceConfig() and > just set the ones that aren't defaults? Done. https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:362: config.regather_on_failed_networks_interval != On 2016/06/29 18:53:27, pthatcher1 wrote: > Can we use an rtc::optional in config rather than a -1? Done. https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:389: MSG_REGATHER_ON_FAILED_NETWORKS); On 2016/06/29 18:53:27, pthatcher1 wrote: > Taylor just refactored this :) Will merge when his CL is in. https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (left): https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:198: network_->SignalInactive.connect(this, &Port::OnNetworkInactive); On 2016/06/29 18:53:27, pthatcher1 wrote: > Why do we no longer listen to a network being inactive? Because when a network becomes inactive, it will be detected in the session. The whole purpose of SignalNetworkInactive is to remove ports and remote candidates. Doing that in the basicportallocatorsession allows a more uniform processing. Plus, doing that from the session avoids a potential leakage of IP address when sending candidate removals as Taylor pointed out. https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:221: // A network interface may have both Ipv4 and IPv6 networks. Only if On 2016/06/29 18:53:27, pthatcher1 wrote: > Ipv or IPv? Please be consistent. Done. https://codereview.webrtc.org/2025573002/diff/390001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:224: std::map<std::string, bool> network_has_connection; On 2016/06/29 18:53:27, pthatcher1 wrote: > Wouldn't a std::set work? Then just check if it's in the set. Done.
lgtm OK, that makes sense. https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:224: std::set<std::string> networks_having_connection; networks_with_connections would be a better name.
Basically just nits about the wording of comments. Great work on this. https://codereview.webrtc.org/2025573002/diff/410001/webrtc/api/webrtcsession... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/2025573002/diff/410001/webrtc/api/webrtcsession... webrtc/api/webrtcsession_unittest.cc:2247: config.presume_writable_when_fully_relayed = true; I don't think presume_writable_when_fully_relayed needs to be true for this test. I accidentally left the parameter as "true" because it switched from being "fully_relayed_requires_create_permission" to "presume_writable_when_fully_relayed", which are inverses of each other. https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:296: void OnRegatherOnFailedNetworks(); Is this still needed? https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:171: // network. Only if all networks of an interface has no connection, has -> have https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:172: // we will start re-gather on all networks of that interface. re-gather => re-gathering https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:172: // we will start re-gather on all networks of that interface. This is an interface that a third party can implement and pass into CreatePeerConnection. So the description of its methods should be phrased like "the implementation should" rather than "we will". https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:188: SignalPortsRemoved; It would be nice to have comments explaining when ports and candidates should be signaled as removed. https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:236: // considered failed. What if a connection just hasn't been made yet? https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:252: // Mark a sequence as "network failed" if its network in the list of failed "if its network is in the..."
Patchset #10 (id:430001) has been deleted
I add a SessionState so that it won't regather when it is currently gathering. https://codereview.webrtc.org/2025573002/diff/410001/webrtc/api/webrtcsession... File webrtc/api/webrtcsession_unittest.cc (right): https://codereview.webrtc.org/2025573002/diff/410001/webrtc/api/webrtcsession... webrtc/api/webrtcsession_unittest.cc:2247: config.presume_writable_when_fully_relayed = true; On 2016/06/29 22:12:27, Taylor Brandstetter wrote: > I don't think presume_writable_when_fully_relayed needs to be true for this > test. I accidentally left the parameter as "true" because it switched from being > "fully_relayed_requires_create_permission" to > "presume_writable_when_fully_relayed", which are inverses of each other. Done. https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:171: // network. Only if all networks of an interface has no connection, On 2016/06/29 22:12:27, Taylor Brandstetter wrote: > has -> have Done. Thanks! https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:172: // we will start re-gather on all networks of that interface. On 2016/06/29 22:12:27, Taylor Brandstetter wrote: > re-gather => re-gathering Done. https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:172: // we will start re-gather on all networks of that interface. On 2016/06/29 22:12:27, Taylor Brandstetter wrote: > re-gather => re-gathering Done. https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:172: // we will start re-gather on all networks of that interface. On 2016/06/29 22:12:27, Taylor Brandstetter wrote: > re-gather => re-gathering Done. https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:188: SignalPortsRemoved; On 2016/06/29 22:12:27, Taylor Brandstetter wrote: > It would be nice to have comments explaining when ports and candidates should be > signaled as removed. Done. https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:224: std::set<std::string> networks_having_connection; On 2016/06/29 21:34:12, pthatcher1 wrote: > networks_with_connections would be a better name. Done. https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:236: // considered failed. On 2016/06/29 22:12:27, Taylor Brandstetter wrote: > What if a connection just hasn't been made yet? This is a good point. I added an enum SessionState to address this issue. https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:252: // Mark a sequence as "network failed" if its network in the list of failed On 2016/06/29 22:12:27, Taylor Brandstetter wrote: > "if its network is in the..." Done.
https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/2025573002/diff/410001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:236: // considered failed. On 2016/06/30 01:04:22, honghaiz3 wrote: > On 2016/06/29 22:12:27, Taylor Brandstetter wrote: > > What if a connection just hasn't been made yet? > This is a good point. > I added an enum SessionState to address this issue. > To elaborate it a little bit, that could only happen when the session is just started gathering, or is starting an ICE restart. So I defined a session could be in one of the three states, INPROGRESS, CLEARED and STOPPED. CLEARED is the state that when at least connection had become writable, but the session can gather continually. And then we only gather on failed networks if the session is at state CLEARED.
Patchset #11 (id:470001) has been deleted
The CQ bit was checked by honghaiz@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: android_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/1...) ios64_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/910) ios64_gn_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_rel/builds/913) ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/8659) ios_api_framework on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_api_framework/build...) ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/10976) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/10900) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/16210) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/14857) linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...) linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/16310) linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/13318) linux_ubsan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/3202) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/4363) mac_gn_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/10577) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/16297)
The CQ bit was checked by honghaiz@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: win_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_compile_dbg/builds/...) win_x64_gn_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_rel/builds/1...)
On 2016/07/01 04:12:27, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > win_compile_dbg on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/win_compile_dbg/builds/...) > win_x64_gn_rel on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_gn_rel/builds/1...) Will you take another look at this?
https://codereview.webrtc.org/2025573002/diff/550001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2025573002/diff/550001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:92: INPROGRESS, // Actively allocating ports and candidates. Can we call this GATHERING? The comment about "StartGettingPorts" says "starts gathering", so having it be GATHERING would make sense, I think. https://codereview.webrtc.org/2025573002/diff/550001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:255: SessionState state_ = SessionState::INPROGRESS; Why is this the state before we start? Shouldn't we have an INIT state? Or would CLEARED make sense for the initial state?
Patchset #15 (id:570001) has been deleted
PTAL. https://codereview.webrtc.org/2025573002/diff/550001/webrtc/p2p/base/portallo... File webrtc/p2p/base/portallocator.h (right): https://codereview.webrtc.org/2025573002/diff/550001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:92: INPROGRESS, // Actively allocating ports and candidates. On 2016/07/01 18:09:59, pthatcher1 wrote: > Can we call this GATHERING? > > The comment about "StartGettingPorts" says "starts gathering", so having it be > GATHERING would make sense, I think. Done. https://codereview.webrtc.org/2025573002/diff/550001/webrtc/p2p/base/portallo... webrtc/p2p/base/portallocator.h:255: SessionState state_ = SessionState::INPROGRESS; On 2016/07/01 18:09:59, pthatcher1 wrote: > Why is this the state before we start? Shouldn't we have an INIT state? Or > would CLEARED make sense for the initial state? Done. I use CLEARED because INIT won't have use anywhere else.
The CQ bit was checked by honghaiz@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: ios64_gn_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_gn_dbg/builds/951) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/14898) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/15806) mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
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/2025573002/#ps630001 (title: "Remove candidates on pruned ports when network failed")
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
Try jobs failed on following builders: android_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) win_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_rel/builds/4532)
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/2025573002/#ps650001 (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 unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_clang_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_rel/build...)
Description was changed from ========== If continual gathering is enabled, we will periodically check if any network does not have any connection on it and if yes, attempt to re-gather on those networks. BUG= ========== to ========== If continual gathering is enabled, we will periodically check if any network does not have any connection on it and if yes, attempt to re-gather on those networks. BUG= R=pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/5622c5eae547dc079763865a3... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:650001) manually as 5622c5eae547dc079763865a31d7da951bb9cd76 (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== If continual gathering is enabled, we will periodically check if any network does not have any connection on it and if yes, attempt to re-gather on those networks. BUG= R=pthatcher@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/5622c5eae547dc079763865a3... ========== to ========== If continual gathering is enabled, we will periodically check if any network does not have any connection on it and if yes, attempt to re-gather on those networks. BUG= R=pthatcher@webrtc.org Committed: https://crrev.com/5622c5eae547dc079763865a31d7da951bb9cd76 Cr-Commit-Position: refs/heads/master@{#13367} ==========
Message was sent while issue was closed.
Patchset #19 (id:670001) has been deleted |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
