Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1421433003:
Fix CreateNetworks to stop it from signaling duplicate networks changed events (Closed)
DescriptionFix two problems in network.cc:
1. It signals network changed events whenever there are more than one IP address in a network.
2. It does not signal network changed events if a network disconnects and connects again.
Also changed DumpNetworks for better debugging.
BUG=webrtc:5096
Committed: https://crrev.com/db8cf50c5994a1ca37f85d7ea17cadb96c1fb93a
Cr-Commit-Position: refs/heads/master@{#11107}
Patch Set 1 #Patch Set 2 : Fix the other half of the bug in network manager #
Total comments: 20
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
Total comments: 2
Messages
Total messages: 28 (18 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Description was changed from ========== Fix CreateNetworks in network.cc to stop it from signaling networks changed events unnecessarily. BUG=5096 ========== to ========== Fix CreateNetworks in network.cc to stop it from signaling networks changed events unnecessarily. BUG=webrtc:5096 ==========
Patchset #3 (id:180001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Fix CreateNetworks in network.cc to stop it from signaling networks changed events unnecessarily. BUG=webrtc:5096 ========== to ========== Fix two problems in network.cc: 1. It signals network changed events whenever there are more than one IP address in a network. 2. It does not signal network changed events if a network disconnects and connects again. BUG=webrtc:5096 ==========
honghaiz@webrtc.org changed reviewers: + juberti@google.com
Description was changed from ========== Fix two problems in network.cc: 1. It signals network changed events whenever there are more than one IP address in a network. 2. It does not signal network changed events if a network disconnects and connects again. BUG=webrtc:5096 ========== to ========== Fix two problems in network.cc: 1. It signals network changed events whenever there are more than one IP address in a network. 2. It does not signal network changed events if a network disconnects and connects again. Also changed DumpNetworks for better debugging. BUG=webrtc:5096 ==========
Patchset #2 (id:200001) has been deleted
https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network.cc File webrtc/base/network.cc (right): https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network.cc#n... webrtc/base/network.cc:295: } Is kv.second.active() always true? If not, should this be "if (existing_net->active() != kv.second.active())"? If so, should we have an ASSERT(kv.second.active())? https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network.cc#n... webrtc/base/network.cc:304: // If the network list changes, we re-assign and resort it. By re-assign you mean reset the active state? If so, please just state that. https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network.cc#n... webrtc/base/network.cc:309: kv.second->set_active(false); So, we leave it in the map even if it's not active? Is that what we did before? https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network.cc#n... webrtc/base/network.cc:431: current_networks[key] = network.get(); Can you explain why this is necessary, here and below? And maybe leave a comment explaining. It's not immediately obvious. https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network.h#ne... webrtc/base/network.h:322: void set_active(bool active) { active_ = active; } Can you put some comments/documentation about what it means for a Network to be active/inactive? https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network_unit... File webrtc/base/network_unittest.cc (right): https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:754: // This ensures releasing the objects created in CallConvertIfAddrs. This ensures releasing => this ensures we realase https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:855: EXPECT_TRUE(changed); Should we test what's in the manager right here? https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:863: EXPECT_EQ(1U, list.size()); Should we test what's in there, and not just the size? https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:865: // Now network1 is inactive. Try to merge it again. What makes network1 inactive? https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:873: EXPECT_TRUE(list.front()->active()); Nit: I find list[0].active() more readable.
PTAL. Thanks. https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network.cc File webrtc/base/network.cc (right): https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network.cc#n... webrtc/base/network.cc:295: } On 2015/12/08 19:40:23, pthatcher1 wrote: > Is kv.second.active() always true? If not, should this be "if > (existing_net->active() != kv.second.active())"? If so, should we have an > ASSERT(kv.second.active())? It should be always active because active_ is true by default. https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network.cc#n... webrtc/base/network.cc:304: // If the network list changes, we re-assign and resort it. On 2015/12/08 19:40:23, pthatcher1 wrote: > By re-assign you mean reset the active state? If so, please just state that. I meant re-assign networks_ to the merged list. Changed the comments. https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network.cc#n... webrtc/base/network.cc:309: kv.second->set_active(false); On 2015/12/08 19:40:23, pthatcher1 wrote: > So, we leave it in the map even if it's not active? Is that what we did before? Yes. Previously it keeps all networks in the map and it does not update its active state even if a network was gone and come back again. https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network.cc#n... webrtc/base/network.cc:431: current_networks[key] = network.get(); On 2015/12/08 19:40:23, pthatcher1 wrote: > Can you explain why this is necessary, here and below? And maybe leave a > comment explaining. It's not immediately obvious. If you look at the if condition one-level outside, "if (existing_network == current_networks.end()) " It is fairly obvious to add this because it tried to check whether this key is in the map but never added any key to the map. I just added the key to map so that next time if the same key is seen, it will find this is an existing network. https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network.h#ne... webrtc/base/network.h:322: void set_active(bool active) { active_ = active; } On 2015/12/08 19:40:23, pthatcher1 wrote: > Can you put some comments/documentation about what it means for a Network to be > active/inactive? Done. https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network_unit... File webrtc/base/network_unittest.cc (right): https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:754: // This ensures releasing the objects created in CallConvertIfAddrs. On 2015/12/08 19:40:24, pthatcher1 wrote: > This ensures releasing => this ensures we realase Done. https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:855: EXPECT_TRUE(changed); On 2015/12/08 19:40:24, pthatcher1 wrote: > Should we test what's in the manager right here? Done. https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:863: EXPECT_EQ(1U, list.size()); On 2015/12/08 19:40:24, pthatcher1 wrote: > Should we test what's in there, and not just the size? Done. https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:865: // Now network1 is inactive. Try to merge it again. On 2015/12/08 19:40:24, pthatcher1 wrote: > What makes network1 inactive? When you merge list2 into an existing list1, If a network appeared in list1 but not in list2, it means the network is gone, so it becomes inactive. We keep it because if the same network come back again, it can reuse the Network object. We cannot delete it because some port may be using the Network pointer. https://codereview.webrtc.org/1421433003/diff/220001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:873: EXPECT_TRUE(list.front()->active()); On 2015/12/08 19:40:24, pthatcher1 wrote: > Nit: I find list[0].active() more readable. Done.
https://codereview.webrtc.org/1421433003/diff/240001/webrtc/base/network.cc File webrtc/base/network.cc (right): https://codereview.webrtc.org/1421433003/diff/240001/webrtc/base/network.cc#n... webrtc/base/network.cc:466: current_networks[key] = network.get(); OK, I understand this now. But it makes me think that you're fixing a bug where we currently won't group IPs of the same network into the same network. If you are fixing this bug, and it looks like you are, can you add a unit test that fails with the current code but passes with your fixed code? It appears our test coverage is lacking if this has been here incorrect for so long. https://codereview.webrtc.org/1421433003/diff/240001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1421433003/diff/240001/webrtc/base/network.h#ne... webrtc/base/network.h:361: // a dormant state by the OS (to conserve energy). Would it make sense to say something like "when we enumerate networks and find a network is missing, we do not remove the network. Instead, we mark it inactive. That way, if it returns we know it's an reactived network, not a new network."?
https://codereview.webrtc.org/1421433003/diff/240001/webrtc/base/network.cc File webrtc/base/network.cc (right): https://codereview.webrtc.org/1421433003/diff/240001/webrtc/base/network.cc#n... webrtc/base/network.cc:466: current_networks[key] = network.get(); On 2015/12/15 08:13:09, pthatcher1 wrote: > OK, I understand this now. But it makes me think that you're fixing a bug where > we currently won't group IPs of the same network into the same network. If you > are fixing this bug, and it looks like you are, can you add a unit test that > fails with the current code but passes with your fixed code? It appears our > test coverage is lacking if this has been here incorrect for so long. I have a test there already. Note that this does not cause big problems though because later in MergeNetworkList, they combine the networks with the same interface, except that it will signal a network change event. We can simply the code MergeNetworkList once this is landed as that is not harmful (and less change now). https://codereview.webrtc.org/1421433003/diff/240001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1421433003/diff/240001/webrtc/base/network.h#ne... webrtc/base/network.h:361: // a dormant state by the OS (to conserve energy). On 2015/12/15 08:13:09, pthatcher1 wrote: > Would it make sense to say something like "when we enumerate networks and find a > network is missing, we do not remove the network. Instead, we mark it inactive. > That way, if it returns we know it's an reactived network, not a new network."? Done, with small changes.
lgtm https://codereview.webrtc.org/1421433003/diff/260001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1421433003/diff/260001/webrtc/base/network.h#ne... webrtc/base/network.h:364: void set_active(bool active) { active_ = active; } Does set_active need to be public? Since NetworkManager is a friend class, it might not be needed.
https://codereview.webrtc.org/1421433003/diff/260001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1421433003/diff/260001/webrtc/base/network.h#ne... webrtc/base/network.h:364: void set_active(bool active) { active_ = active; } On 2015/12/18 22:03:39, pthatcher1 wrote: > Does set_active need to be public? Since NetworkManager is a friend class, it > might not be needed. It was used by NetworkManagerBase. So we cannot make it private unless we add NetworkManagerBase to the friend class as well.
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/1421433003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1421433003/260001
Message was sent while issue was closed.
Description was changed from ========== Fix two problems in network.cc: 1. It signals network changed events whenever there are more than one IP address in a network. 2. It does not signal network changed events if a network disconnects and connects again. Also changed DumpNetworks for better debugging. BUG=webrtc:5096 ========== to ========== Fix two problems in network.cc: 1. It signals network changed events whenever there are more than one IP address in a network. 2. It does not signal network changed events if a network disconnects and connects again. Also changed DumpNetworks for better debugging. BUG=webrtc:5096 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Fix two problems in network.cc: 1. It signals network changed events whenever there are more than one IP address in a network. 2. It does not signal network changed events if a network disconnects and connects again. Also changed DumpNetworks for better debugging. BUG=webrtc:5096 ========== to ========== Fix two problems in network.cc: 1. It signals network changed events whenever there are more than one IP address in a network. 2. It does not signal network changed events if a network disconnects and connects again. Also changed DumpNetworks for better debugging. BUG=webrtc:5096 Committed: https://crrev.com/db8cf50c5994a1ca37f85d7ea17cadb96c1fb93a Cr-Commit-Position: refs/heads/master@{#11107} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/db8cf50c5994a1ca37f85d7ea17cadb96c1fb93a Cr-Commit-Position: refs/heads/master@{#11107} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
