Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1284113003:
Move the concept of multiple route into Network (Closed)
|
Created:
5 years, 4 months ago by guoweis_webrtc Modified:
5 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionIn the past, P2PPortAllocator.enable_multiple_routes is the indicator whether we should bind to the any address. It's easy to translate that into a port allocator flag in P2PPortAllocator's ctor. Going forward, we have to depend on an asynchronous permission check to determine whether gathering local address is allowed or not, hence the current way of passing it through constructor approach won't work any more. The asynchronous check will trigger SignalNetowrksChanged so we could only check that inside DoAllocate.
Adapter enumeration disable should be a concept from Network. Network will be hooked up with media permission (mic/camera) to check whether gathering local address is allowed.
BUG=crbug.com/520101
R=juberti@webrtc.org, pthatcher@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/ba9ab4cd8d2e8fbc068dc36b5e6f6331d7deeccf
Committed: https://chromium.googlesource.com/external/webrtc/+/47872ec90c99ce9c37fb37a8f623ce0e65a32f61
Committed: https://chromium.googlesource.com/external/webrtc/+/86cb923c20e8e50dbb9e3d7984ec1bd5d2455b6a
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 12
Patch Set 5 : Merge from master and address Justin's comments. #
Total comments: 1
Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #Patch Set 9 : #
Total comments: 12
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : Fix build break in chromium.fyi #Patch Set 14 : #Messages
Total messages: 33 (4 generated)
guoweis@webrtc.org changed reviewers: + juberti@webrtc.org, pthatcher@webrtc.org
Going forward, the decision of whether to enumerate adapters is going to not only base on the preference, but also on the permission granted for the site. Moving the concept into network layer instead. The design is that in renderer process, we'll have a wrapper exposing rtc::NetworkManager which will gather all information and expose the right network sets.
https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h#new... webrtc/base/network.h:80: virtual void GetNetworks(NetworkList* networks) const = 0; suggest that GetNetworks return a bool so that you can return false to indicate enumeration failed (as opposed to just having zero networks) https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h#new... webrtc/base/network.h:86: virtual bool adapter_enumeration_disabled() const { If it's virtual, typically it won't have the all-lowercase style used for trivial accessors. Also, you might want to call this something like can_get_networks(), and have it initially false, but only set it to true once SignalNetworksChanged AND adapter enumeration is allowed. (That would allow you to make it non-virtual). https://codereview.webrtc.org/1284113003/diff/60001/webrtc/p2p/client/basicpo... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/1284113003/diff/60001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:310: if (allocator_->network_manager()->adapter_enumeration_disabled()) { It seems odd to set a flag based on some other condition. I would suggest doing this as if (flags() & PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION || allocator_->network_manager()->IsAdapterEnumerationDisabled())
https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h#new... webrtc/base/network.h:80: virtual void GetNetworks(NetworkList* networks) const = 0; On 2015/08/13 20:45:58, juberti1 wrote: > suggest that GetNetworks return a bool so that you can return false to indicate > enumeration failed (as opposed to just having zero networks) Do you mean returning false here to reprenset the idea of "adapter enumeration disabled"? https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h#new... webrtc/base/network.h:86: virtual bool adapter_enumeration_disabled() const { On 2015/08/13 20:45:58, juberti1 wrote: > If it's virtual, typically it won't have the all-lowercase style used for > trivial accessors. > > Also, you might want to call this something like can_get_networks(), and have it > initially false, but only set it to true once SignalNetworksChanged AND adapter > enumeration is allowed. (That would allow you to make it non-virtual). > I think "can_get_networks" doesn't seem to convey the idea of using the AnyAddress networks. 1. GetNetworks shouldn't be called before SignalNetworksChanged. ==> return false 2. GetNetworks will return empty if there is really no networks OR if the adapter enumeration is disabled. https://codereview.webrtc.org/1284113003/diff/60001/webrtc/p2p/client/basicpo... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/1284113003/diff/60001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:310: if (allocator_->network_manager()->adapter_enumeration_disabled()) { On 2015/08/13 20:45:58, juberti1 wrote: > It seems odd to set a flag based on some other condition. I would suggest doing > this as > > if (flags() & PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION || > allocator_->network_manager()->IsAdapterEnumerationDisabled()) There are other places in the code which depends on this flag. If the pattern of setting the flag inside the webrtc code base is discouraged, I'll modify these other places as well.
https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h#new... webrtc/base/network.h:80: virtual void GetNetworks(NetworkList* networks) const = 0; On 2015/08/13 20:54:14, guoweis wrote: > On 2015/08/13 20:45:58, juberti1 wrote: > > suggest that GetNetworks return a bool so that you can return false to > indicate > > enumeration failed (as opposed to just having zero networks) > > Do you mean returning false here to reprenset the idea of "adapter enumeration > disabled"? yes, or any other sort of error occurred. The API below can be used to verify if enumeration is disabled. https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h#new... webrtc/base/network.h:86: virtual bool adapter_enumeration_disabled() const { On 2015/08/13 20:54:14, guoweis wrote: > On 2015/08/13 20:45:58, juberti1 wrote: > > If it's virtual, typically it won't have the all-lowercase style used for > > trivial accessors. > > > > Also, you might want to call this something like can_get_networks(), and have > it > > initially false, but only set it to true once SignalNetworksChanged AND > adapter > > enumeration is allowed. (That would allow you to make it non-virtual). > > > > I think "can_get_networks" doesn't seem to convey the idea of using the > AnyAddress networks. > > 1. GetNetworks shouldn't be called before SignalNetworksChanged. > ==> return false > 2. GetNetworks will return empty if there is really no networks OR if the > adapter enumeration is disabled. > If enumeration is disabled, can_get_networks will remain false. But maybe we need three states: the initial state, before SignalNetworksChanged is called, and then the disabled/enabled states. https://codereview.webrtc.org/1284113003/diff/60001/webrtc/p2p/client/basicpo... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/1284113003/diff/60001/webrtc/p2p/client/basicpo... webrtc/p2p/client/basicportallocator.cc:310: if (allocator_->network_manager()->adapter_enumeration_disabled()) { On 2015/08/13 20:54:14, guoweis wrote: > On 2015/08/13 20:45:58, juberti1 wrote: > > It seems odd to set a flag based on some other condition. I would suggest > doing > > this as > > > > if (flags() & PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION || > > allocator_->network_manager()->IsAdapterEnumerationDisabled()) > > There are other places in the code which depends on this flag. If the pattern of > setting the flag inside the webrtc code base is discouraged, I'll modify these > other places as well. I see. In that case this seems like the best approach; just add a comment to say that if the network manager has disabled enumeration, we just act as if the PA flag has been set. Or would it be possible to remove the PA flag and just use the NetworkManager setting?
https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h#new... webrtc/base/network.h:80: virtual void GetNetworks(NetworkList* networks) const = 0; there are existing override in chromium which prevents me from just modifying the signature here. I'll add a new function NetworkState RetrieveNetworks(NetworkList* networks) const = 0 NetworkState is a tri state. Sounds reasonable? https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h#new... webrtc/base/network.h:86: virtual bool adapter_enumeration_disabled() const { On 2015/08/13 22:10:11, juberti1 wrote: > On 2015/08/13 20:54:14, guoweis wrote: > > On 2015/08/13 20:45:58, juberti1 wrote: > > > If it's virtual, typically it won't have the all-lowercase style used for > > > trivial accessors. > > > > > > Also, you might want to call this something like can_get_networks(), and > have > > it > > > initially false, but only set it to true once SignalNetworksChanged AND > > adapter > > > enumeration is allowed. (That would allow you to make it non-virtual). > > > > > > > I think "can_get_networks" doesn't seem to convey the idea of using the > > AnyAddress networks. > > > > 1. GetNetworks shouldn't be called before SignalNetworksChanged. > > ==> return false > > 2. GetNetworks will return empty if there is really no networks OR if the > > adapter enumeration is disabled. > > > > If enumeration is disabled, can_get_networks will remain false. But maybe we > need three states: the initial state, before SignalNetworksChanged is called, > and then the disabled/enabled states. I'll just use RetrieveNetworks, instead of introducing a new function here.
https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h#new... webrtc/base/network.h:80: virtual void GetNetworks(NetworkList* networks) const = 0; On 2015/08/13 22:49:36, guoweis wrote: > there are existing override in chromium which prevents me from just modifying > the signature here. > > I'll add a new function > NetworkState RetrieveNetworks(NetworkList* networks) const = 0 > > NetworkState is a tri state. > > Sounds reasonable? If you have to add a new function, I suggest adding something like enum NetworkPermissionState { STATE_UNKNOWN, STATE_ALLOWED, STATE_BLOCKED } NetworkPermissionState GetState() I prefer that to adding a new function that does essentially the same thing as GetNetworks.
On 2015/08/13 23:05:27, juberti1 wrote: > https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h > File webrtc/base/network.h (right): > > https://codereview.webrtc.org/1284113003/diff/60001/webrtc/base/network.h#new... > webrtc/base/network.h:80: virtual void GetNetworks(NetworkList* networks) const > = 0; > On 2015/08/13 22:49:36, guoweis wrote: > > there are existing override in chromium which prevents me from just modifying > > the signature here. > > > > I'll add a new function > > NetworkState RetrieveNetworks(NetworkList* networks) const = 0 > > > > NetworkState is a tri state. > > > > Sounds reasonable? > > If you have to add a new function, I suggest adding something like > > enum NetworkPermissionState { STATE_UNKNOWN, STATE_ALLOWED, STATE_BLOCKED } > NetworkPermissionState GetState() > > I prefer that to adding a new function that does essentially the same thing as > GetNetworks. PTAL
just a minor comment https://codereview.webrtc.org/1284113003/diff/80001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1284113003/diff/80001/webrtc/base/network.h#new... webrtc/base/network.h:115: protected: Can this go down into NetworkManagerBase? This class is more like an interface, and not sure it should have data members. Either way, protected member vars are discouraged. This var, wherever it lives, should be updated by a setter.
PTAL. Moved to the NetworkManagerBase.
code lgtm. unit test?
On 2015/08/14 21:19:37, juberti1 wrote: > code lgtm. unit test? added in one existing test case to verify the state.
https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network.cc File webrtc/base/network.cc (right): https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network.cc#n... webrtc/base/network.cc:662: void BasicNetworkManager::StopUpdating() { Should this reset the permission state? https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network_unit... File webrtc/base/network_unittest.cc (right): https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network_unit... webrtc/base/network_unittest.cc:223: EXPECT_EQ(manager.network_permission_state(), NetworkManager::STATE_UNKNOWN); I think this should go into TestUpdateNetworks (which should have its function comment modified to indicate it tests Start/StopUpdating).
Patchset #8 (id:140001) has been deleted
Patchset #8 (id:160001) has been deleted
On 2015/08/14 23:25:55, juberti1 wrote: > https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network.cc > File webrtc/base/network.cc (right): > > https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network.cc#n... > webrtc/base/network.cc:662: void BasicNetworkManager::StopUpdating() { > Should this reset the permission state? > > https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network_unit... > File webrtc/base/network_unittest.cc (right): > > https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network_unit... > webrtc/base/network_unittest.cc:223: > EXPECT_EQ(manager.network_permission_state(), NetworkManager::STATE_UNKNOWN); > I think this should go into TestUpdateNetworks (which should have its function > comment modified to indicate it tests Start/StopUpdating). PTAL.
On 2015/08/14 23:25:55, juberti1 wrote: > https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network.cc > File webrtc/base/network.cc (right): > > https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network.cc#n... > webrtc/base/network.cc:662: void BasicNetworkManager::StopUpdating() { > Should this reset the permission state? > > https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network_unit... > File webrtc/base/network_unittest.cc (right): > > https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network_unit... > webrtc/base/network_unittest.cc:223: > EXPECT_EQ(manager.network_permission_state(), NetworkManager::STATE_UNKNOWN); > I think this should go into TestUpdateNetworks (which should have its function > comment modified to indicate it tests Start/StopUpdating). PTAL.
On 2015/08/15 16:30:09, guoweis wrote: > On 2015/08/14 23:25:55, juberti1 wrote: > > https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network.cc > > File webrtc/base/network.cc (right): > > > > > https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network.cc#n... > > webrtc/base/network.cc:662: void BasicNetworkManager::StopUpdating() { > > Should this reset the permission state? > > > > > https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network_unit... > > File webrtc/base/network_unittest.cc (right): > > > > > https://codereview.webrtc.org/1284113003/diff/120001/webrtc/base/network_unit... > > webrtc/base/network_unittest.cc:223: > > EXPECT_EQ(manager.network_permission_state(), NetworkManager::STATE_UNKNOWN); > > I think this should go into TestUpdateNetworks (which should have its function > > comment modified to indicate it tests Start/StopUpdating). > > PTAL. Ping
https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.cc File webrtc/base/network.cc (right): https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.cc#n... webrtc/base/network.cc:317: network_permission_state_ = STATE_ALLOWED; I don't understand. Why is it not allowed until after the network list is merged? Why isn't it allowed right from the start? https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.cc#n... webrtc/base/network.cc:676: set_network_permission_state(NetworkManager::STATE_UNKNOWN); Why does the permission state change when we stop updating? It seems like we're mixing up "ready state" with "permission state". https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.h#ne... webrtc/base/network.h:65: // should be used instead. I think a better name might be: enum EnumerationPermission { ENUMERATION_PENDING ENUMERATION_ALLOWED, ENUMERATION_DISALLOWED } Actually, the style of enums is supposed to be: enum EnumerationPermission { kEnumerationPending, kEnumerationAllowed, kEnumerationDisallowed, } https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.h#ne... webrtc/base/network.h:89: virtual void GetNetworks(NetworkList* networks) const = 0; We might consider calling this EnumerateNetworks. https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.h#ne... webrtc/base/network.h:92: virtual NetworkPermissionState network_permission_state() const = 0; enumeration_permission() would work well here as a name. https://codereview.webrtc.org/1284113003/diff/190001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/1284113003/diff/190001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:314: rtc::NetworkManager::STATE_BLOCKED) { In a release build, we would grant access if in the STATE_UNKNOWN. I think it would be safer to test "!= ALLOWED" rather than "== BLOCKED". https://codereview.webrtc.org/1284113003/diff/190001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:315: set_flags(flags() | PORTALLOCATOR_DISABLE_ADAPTER_ENUMERATION); So what if the the permission changes? Should we have a signal that listens to that and starts enumeration after it changes?
https://codereview.webrtc.org/1284113003/diff/190001/webrtc/p2p/client/basicp... File webrtc/p2p/client/basicportallocator.cc (right): https://codereview.webrtc.org/1284113003/diff/190001/webrtc/p2p/client/basicp... webrtc/p2p/client/basicportallocator.cc:316: } Can you add a unit test for this?
On 2015/08/18 18:47:59, pthatcher1 wrote: > https://codereview.webrtc.org/1284113003/diff/190001/webrtc/p2p/client/basicp... > File webrtc/p2p/client/basicportallocator.cc (right): > > https://codereview.webrtc.org/1284113003/diff/190001/webrtc/p2p/client/basicp... > webrtc/p2p/client/basicportallocator.cc:316: } > Can you add a unit test for this? PTAL.
Did you look at my suggestions for name changes?
On 2015/08/18 21:35:41, pthatcher1 wrote: > Did you look at my suggestions for name changes? PTAL.
On 2015/08/18 21:55:32, guoweis wrote: > On 2015/08/18 21:35:41, pthatcher1 wrote: > > Did you look at my suggestions for name changes? > > PTAL. lgtm
Message was sent while issue was closed.
Committed patchset #12 (id:250001) manually as ba9ab4cd8d2e8fbc068dc36b5e6f6331d7deeccf (presubmit successful).
Message was sent while issue was closed.
Committed patchset #13 (id:270001) manually as 47872ec90c99ce9c37fb37a8f623ce0e65a32f61 (presubmit successful).
Patchset #14 (id:290001) has been deleted
Message was sent while issue was closed.
Committed patchset #14 (id:310001) manually as 86cb923c20e8e50dbb9e3d7984ec1bd5d2455b6a (presubmit successful).
Message was sent while issue was closed.
What happened with this CL? The design changed nontrivially and the unit test seems to be gone now. https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.cc File webrtc/base/network.cc (right): https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.cc#n... webrtc/base/network.cc:317: network_permission_state_ = STATE_ALLOWED; On 2015/08/18 18:34:19, pthatcher1 wrote: > I don't understand. Why is it not allowed until after the network list is > merged? Why isn't it allowed right from the start? I'm not sure we know if it's allowed initially. https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.h#ne... webrtc/base/network.h:65: // should be used instead. On 2015/08/18 18:34:19, pthatcher1 wrote: > I think a better name might be: > > enum EnumerationPermission { > ENUMERATION_PENDING > ENUMERATION_ALLOWED, > ENUMERATION_DISALLOWED > } > > Actually, the style of enums is supposed to be: > > enum EnumerationPermission { > kEnumerationPending, > kEnumerationAllowed, > kEnumerationDisallowed, > } Peter, we (and Chrome) use old-style enum macros everywhere. I don't understand why we would use k-style here.
Message was sent while issue was closed.
The only big change was that we removed the non-Chrome code from having an "unknown" state, and removed the "unknown" state altogether. On 2015/08/20 06:31:04, juberti1 wrote: > What happened with this CL? The design changed nontrivially and the unit test > seems to be gone now. > > https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.cc > File webrtc/base/network.cc (right): > > https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.cc#n... > webrtc/base/network.cc:317: network_permission_state_ = STATE_ALLOWED; > On 2015/08/18 18:34:19, pthatcher1 wrote: > > I don't understand. Why is it not allowed until after the network list is > > merged? Why isn't it allowed right from the start? > > I'm not sure we know if it's allowed initially. > > https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.h > File webrtc/base/network.h (right): > > https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.h#ne... > webrtc/base/network.h:65: // should be used instead. > On 2015/08/18 18:34:19, pthatcher1 wrote: > > I think a better name might be: > > > > enum EnumerationPermission { > > ENUMERATION_PENDING > > ENUMERATION_ALLOWED, > > ENUMERATION_DISALLOWED > > } > > > > Actually, the style of enums is supposed to be: > > > > enum EnumerationPermission { > > kEnumerationPending, > > kEnumerationAllowed, > > kEnumerationDisallowed, > > } > > Peter, we (and Chrome) use old-style enum macros everywhere. I don't understand > why we would use k-style here.
Message was sent while issue was closed.
https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.cc File webrtc/base/network.cc (right): https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.cc#n... webrtc/base/network.cc:317: network_permission_state_ = STATE_ALLOWED; On 2015/08/20 06:31:04, juberti1 wrote: > On 2015/08/18 18:34:19, pthatcher1 wrote: > > I don't understand. Why is it not allowed until after the network list is > > merged? Why isn't it allowed right from the start? > > I'm not sure we know if it's allowed initially. Outside of Chrome it is. Chrome can change the state for its uses. https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.h File webrtc/base/network.h (right): https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.h#ne... webrtc/base/network.h:65: // should be used instead. On 2015/08/20 06:31:04, juberti1 wrote: > On 2015/08/18 18:34:19, pthatcher1 wrote: > > I think a better name might be: > > > > enum EnumerationPermission { > > ENUMERATION_PENDING > > ENUMERATION_ALLOWED, > > ENUMERATION_DISALLOWED > > } > > > > Actually, the style of enums is supposed to be: > > > > enum EnumerationPermission { > > kEnumerationPending, > > kEnumerationAllowed, > > kEnumerationDisallowed, > > } > > Peter, we (and Chrome) use old-style enum macros everywhere. I don't understand > why we would use k-style here. We switched back to old-style enums? I wasn't aware. Thanks for letting me know. Sorry, Guowei. I guess we should switch it back.
Message was sent while issue was closed.
On 2015/08/20 06:43:37, pthatcher1 wrote: > https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.cc > File webrtc/base/network.cc (right): > > https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.cc#n... > webrtc/base/network.cc:317: network_permission_state_ = STATE_ALLOWED; > On 2015/08/20 06:31:04, juberti1 wrote: > > On 2015/08/18 18:34:19, pthatcher1 wrote: > > > I don't understand. Why is it not allowed until after the network list is > > > merged? Why isn't it allowed right from the start? > > > > I'm not sure we know if it's allowed initially. > > Outside of Chrome it is. Chrome can change the state for its uses. NetworkManager is an interface (it should really be called NetworkManagerInterface). I don't think we should be setting the behavior in the interface (i.e. to always return Allowed). We can do it in the concrete impl not used by Chrome, of course. > > https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.h > File webrtc/base/network.h (right): > > https://codereview.webrtc.org/1284113003/diff/190001/webrtc/base/network.h#ne... > webrtc/base/network.h:65: // should be used instead. > On 2015/08/20 06:31:04, juberti1 wrote: > > On 2015/08/18 18:34:19, pthatcher1 wrote: > > > I think a better name might be: > > > > > > enum EnumerationPermission { > > > ENUMERATION_PENDING > > > ENUMERATION_ALLOWED, > > > ENUMERATION_DISALLOWED > > > } > > > > > > Actually, the style of enums is supposed to be: > > > > > > enum EnumerationPermission { > > > kEnumerationPending, > > > kEnumerationAllowed, > > > kEnumerationDisallowed, > > > } > > > > Peter, we (and Chrome) use old-style enum macros everywhere. I don't > understand > > why we would use k-style here. > > We switched back to old-style enums? I wasn't aware. Thanks for letting me > know. Sorry, Guowei. I guess we should switch it back.
Message was sent while issue was closed.
BTW, how will enumeration_permission be used? |
