Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2951803003:
Listen for Wifi-Direct networks and include them in the network list (Closed)
DescriptionListen for Wifi-Direct networks and include them in the network list
BUG=webrtc:7708
TBR=magjed@webrtc.org
Review-Url: https://codereview.webrtc.org/2951803003
Cr-Commit-Position: refs/heads/master@{#18839}
Committed: https://chromium.googlesource.com/external/webrtc/+/06b47c520dc9814da6a0fcaaf548b65ffea7cab8
Patch Set 1 #
Total comments: 15
Patch Set 2 : Update from cl comments #Patch Set 3 : Move wifi p2p handle constant into delegate #
Total comments: 3
Patch Set 4 : Fix cl comments #
Total comments: 1
Patch Set 5 : Suppress InlinedApi warning #
Messages
Total messages: 32 (17 generated)
Description was changed from ========== Merge remote-tracking branch 'refs/remotes/origin/master' into android_wifi_direct Listen for Wifi-Direct networks and include them in the network list BUG=webrtc:7708 ========== to ========== Listen for Wifi-Direct networks and include them in the network list BUG=webrtc:7708 ==========
bdodson@google.com changed reviewers: + deadbeef@google.com
bdodson@google.com changed reviewers: + pthatcher@google.com
https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java (right): https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:449: public NetworkMonitorAutoDetect(Observer observer, Context context) { Can you make this new functionality opt-in by having some kind of enableWifiDirect() method that enables it, but it's off by default? That would reduce the risk of something going wrong with existing mobile apps that don't want this. https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:461: intentFilter.addAction(WifiP2pManager.WIFI_P2P_CONNECTION_CHANGED_ACTION); Can we move this into wifiManagerDelegate? Its already registering for similar wifi-specific intents and handling them. Same with the code below that handles it. In fact, it might be a good idea to make a wifiP2pDelegate to go along with the other two to separate the normal wifi and the wifi direct. https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:513: result.add(wifiP2pNetworkInfo); If we have a separate wifiDirectDelegate, then that could check for wifiDirectDelegate.getActiveNetworkList() or (if there can only be one) wifiDirectDelegate.getActiveNetwork(). Then this is basically just merging lists. https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:623: if (intent.getIntExtra(WifiP2pManager.EXTRA_WIFI_STATE, 0) Where does this 0 come from? https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:627: } Might as well by symmetric with the other line: state = intent.getIntExtra(WifiP2pManager.EXTRA_WIFI_STATE, 0); onWifiP2pStateChange(state); https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:657: } If we move this into a wifiDirectDelegate, we could just pass the observer into the delegate to let it fire onNetworkConnect and onNetworkDisconnect.
https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java (right): https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:449: public NetworkMonitorAutoDetect(Observer observer, Context context) { On 2017/06/22 22:58:23, pthatcher wrote: > Can you make this new functionality opt-in by having some kind of > enableWifiDirect() method that enables it, but it's off by default? That would > reduce the risk of something going wrong with existing mobile apps that don't > want this. Done. https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:461: intentFilter.addAction(WifiP2pManager.WIFI_P2P_CONNECTION_CHANGED_ACTION); On 2017/06/22 22:58:22, pthatcher wrote: > Can we move this into wifiManagerDelegate? Its already registering for similar > wifi-specific intents and handling them. Same with the code below that handles > it. > > In fact, it might be a good idea to make a wifiP2pDelegate to go along with the > other two to separate the normal wifi and the wifi direct. Done. https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:513: result.add(wifiP2pNetworkInfo); On 2017/06/22 22:58:22, pthatcher wrote: > If we have a separate wifiDirectDelegate, then that could check for > wifiDirectDelegate.getActiveNetworkList() or (if there can only be one) > wifiDirectDelegate.getActiveNetwork(). Then this is basically just merging > lists. Done. https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:623: if (intent.getIntExtra(WifiP2pManager.EXTRA_WIFI_STATE, 0) On 2017/06/22 22:58:22, pthatcher wrote: > Where does this 0 come from? Done. https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:627: } On 2017/06/22 22:58:23, pthatcher wrote: > Might as well by symmetric with the other line: > > state = intent.getIntExtra(WifiP2pManager.EXTRA_WIFI_STATE, 0); > onWifiP2pStateChange(state); Done. https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:657: } On 2017/06/22 22:58:22, pthatcher wrote: > If we move this into a wifiDirectDelegate, we could just pass the observer into > the delegate to let it fire onNetworkConnect and onNetworkDisconnect. Done.
deadbeef@webrtc.org changed reviewers: + deadbeef@webrtc.org
https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java (right): https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:449: public NetworkMonitorAutoDetect(Observer observer, Context context) { On 2017/06/22 22:58:23, pthatcher wrote: > Can you make this new functionality opt-in by having some kind of > enableWifiDirect() method that enables it, but it's off by default? That would > reduce the risk of something going wrong with existing mobile apps that don't > want this. I don't see the risk, and don't like adding a new static method. And it would be nice if this just started working by default for all apps without opting in. What risk do you see, Peter? https://codereview.webrtc.org/2951803003/diff/40001/webrtc/sdk/android/api/or... File webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java (right): https://codereview.webrtc.org/2951803003/diff/40001/webrtc/sdk/android/api/or... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:413: // (NETWORK_UNSPECIFIED) for these addresses. In PhysicalSocket::Bind, it's assumed that if BindSocketToNetwork succeeds, the socket is bound to a specific network using some OS-specific mechanism (currently, only android_setsocknetwork), and the system "bind" call used to allocate a port can use INADDR_ANY. See: https://cs.chromium.org/chromium/src/third_party/webrtc/base/physicalsocketse... But in this case, BindSocketToNetwork will *not* bind the socket to the Wi-Fi Direct network. And so ::bind *should* use the IP address (assuming that works). So to fix this, BindSocketToNetwork should return NetworkBindingResult::NOT_IMPLEMENTED or some new return code, instead of calling android_setsocknetwork with NETWORK_UNSPECIFIED.
I think I'm good with it, except for Taylor's comment and perhaps the static method thing. https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java (right): https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:449: public NetworkMonitorAutoDetect(Observer observer, Context context) { On 2017/06/23 15:02:05, Taylor Brandstetter wrote: > On 2017/06/22 22:58:23, pthatcher wrote: > > Can you make this new functionality opt-in by having some kind of > > enableWifiDirect() method that enables it, but it's off by default? That > would > > reduce the risk of something going wrong with existing mobile apps that don't > > want this. > > I don't see the risk, and don't like adding a new static method. And it would be > nice if this just started working by default for all apps without opting in. > What risk do you see, Peter? I feel like the risk is that we'll have more network interfaces and new candidates and new network activity that hasn't existed before, which could combine with existing behavior in unexpected ways. For example, what if make a wifi direct candidate pair and send a packet on ICE check on it, and that causes some cell phones to fail for all WiFi? Remember bugs like ipv6 failing when the screen is turned off or Verizon shutting down sockets when we don't use the socket properly? Weird things happen when new, unused code paths are introduced, and this feels like an area where things like that could happen. In fact, all of our experience of rolling out ipv6 has been a chain of problems, and WiFi direct could be similar. Things like this ought to be rolled out with experiments and tracking metrics, which means a way to turn it on and off. https://codereview.webrtc.org/2951803003/diff/40001/webrtc/sdk/android/api/or... File webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java (right): https://codereview.webrtc.org/2951803003/diff/40001/webrtc/sdk/android/api/or... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:529: enableWifiDirectNetworks = enabled; Hmmm... not excited about this being static. But otherwise, you'd have to get a hold of the NetworkMonitor and use it to set it. I kind think of a good solution.
https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... File webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java (right): https://codereview.webrtc.org/2951803003/diff/1/webrtc/sdk/android/api/org/we... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:449: public NetworkMonitorAutoDetect(Observer observer, Context context) { On 2017/06/23 21:51:30, pthatcher wrote: > On 2017/06/23 15:02:05, Taylor Brandstetter wrote: > > On 2017/06/22 22:58:23, pthatcher wrote: > > > Can you make this new functionality opt-in by having some kind of > > > enableWifiDirect() method that enables it, but it's off by default? That > > would > > > reduce the risk of something going wrong with existing mobile apps that > don't > > > want this. > > > > I don't see the risk, and don't like adding a new static method. And it would > be > > nice if this just started working by default for all apps without opting in. > > What risk do you see, Peter? > > I feel like the risk is that we'll have more network interfaces and new > candidates and new network activity that hasn't existed before, which could > combine with existing behavior in unexpected ways. For example, what if make a > wifi direct candidate pair and send a packet on ICE check on it, and that > causes some cell phones to fail for all WiFi? Remember bugs like ipv6 failing > when the screen is turned off or Verizon shutting down sockets when we don't use > the socket properly? Weird things happen when new, unused code paths are > introduced, and this feels like an area where things like that could happen. In > fact, all of our experience of rolling out ipv6 has been a chain of problems, > and WiFi direct could be similar. Things like this ought to be rolled out with > experiments and tracking metrics, which means a way to turn it on and off. I'm actually happier to have a flag of some kind myself, since I can't test it on tons of different configurations very easily. We'll get better coverage in our apps fishfood/dogfood process and then I'll feel better about making it default. I have moved the flag over to use the initializeFieldTrials though, since that seems to be the pattern for this in WebRTC. https://codereview.webrtc.org/2951803003/diff/40001/webrtc/sdk/android/api/or... File webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java (right): https://codereview.webrtc.org/2951803003/diff/40001/webrtc/sdk/android/api/or... webrtc/sdk/android/api/org/webrtc/NetworkMonitorAutoDetect.java:413: // (NETWORK_UNSPECIFIED) for these addresses. On 2017/06/23 15:02:06, Taylor Brandstetter wrote: > In PhysicalSocket::Bind, it's assumed that if BindSocketToNetwork succeeds, the > socket is bound to a specific network using some OS-specific mechanism > (currently, only android_setsocknetwork), and the system "bind" call used to > allocate a port can use INADDR_ANY. See: > https://cs.chromium.org/chromium/src/third_party/webrtc/base/physicalsocketse... > > But in this case, BindSocketToNetwork will *not* bind the socket to the Wi-Fi > Direct network. And so ::bind *should* use the IP address (assuming that works). > > So to fix this, BindSocketToNetwork should return > NetworkBindingResult::NOT_IMPLEMENTED or some new return code, instead of > calling android_setsocknetwork with NETWORK_UNSPECIFIED. I have changed the BindSocketToNetwork to see this value and return NOT_IMPLEMENTED. This seems to work fine.
lgtm https://codereview.webrtc.org/2951803003/diff/60001/webrtc/sdk/android/src/jn... File webrtc/sdk/android/src/jni/androidnetworkmonitor_jni.cc (right): https://codereview.webrtc.org/2951803003/diff/60001/webrtc/sdk/android/src/jn... webrtc/sdk/android/src/jni/androidnetworkmonitor_jni.cc:231: nit: Would be nice to have a comment saying something like "If we have an unspecified network handle (currently the case for Wi-Fi direct networks), we aren't able to actually bind the socket to the network. So we should return 'NOT_IMPLEMENTED' and allow the caller to fall back to some other form of binding."
pthatcher@webrtc.org changed reviewers: + pthatcher@webrtc.org
lgtm
The CQ bit was checked by bdodson@google.com
The CQ bit was unchecked by bdodson@google.com
The CQ bit was checked by pthatcher@webrtc.org
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18621)
bdodson@google.com changed reviewers: + glaznev@webrtc.org, magjed@webrtc.org
The CQ bit was checked by bdodson@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org, pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2951803003/#ps80001 (title: "Suppress InlinedApi warning")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18667)
Pinging this to see if I can get an owners review.
Description was changed from ========== Listen for Wifi-Direct networks and include them in the network list BUG=webrtc:7708 ========== to ========== Listen for Wifi-Direct networks and include them in the network list BUG=webrtc:7708 TBR=magjed@webrtc.org ==========
Owners are OOO; I think this is isolated enough to land with a TBR though.
The CQ bit was checked by deadbeef@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498751566647310,
"parent_rev": "8cf398d4e1dddaf2d5485a9f3090eca58c8fe839", "commit_rev":
"06b47c520dc9814da6a0fcaaf548b65ffea7cab8"}
Message was sent while issue was closed.
Description was changed from ========== Listen for Wifi-Direct networks and include them in the network list BUG=webrtc:7708 TBR=magjed@webrtc.org ========== to ========== Listen for Wifi-Direct networks and include them in the network list BUG=webrtc:7708 TBR=magjed@webrtc.org Review-Url: https://codereview.webrtc.org/2951803003 Cr-Commit-Position: refs/heads/master@{#18839} Committed: https://chromium.googlesource.com/external/webrtc/+/06b47c520dc9814da6a0fcaaf... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/06b47c520dc9814da6a0fcaaf... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
