Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2750453002:
Add DiscoveryNetworkMonitor implementation (Closed)
DescriptionAdd DiscoveryNetworkMonitor implementation
This changes adds a singleton class DiscoveryNetworkMonitor which
provides general information to services doing device discovery on the
local network.
BUG=698943
Review-Url: https://codereview.chromium.org/2750453002
Cr-Commit-Position: refs/heads/master@{#477378}
Committed: https://chromium.googlesource.com/chromium/src/+/2bf972dc59af2654f71f577122e39fed4c0a986e
Patch Set 1 #
Total comments: 47
Patch Set 2 : Respond to mfoltz' and imcheng's comments #
Total comments: 38
Patch Set 3 : Respond to mfoltz' comments, add chrome.dial API back with tests #
Total comments: 20
Patch Set 4 : Respond to mfoltz' comments, remove chrome.dial API #Patch Set 5 : Remove extension test files #
Total comments: 42
Patch Set 6 : Rebase #Patch Set 7 : Respond to mfoltz' comments #
Total comments: 41
Patch Set 8 : Respond to mfoltz' comments #Patch Set 9 : Respond to imcheng's comments #
Total comments: 16
Patch Set 10 : Respond to mfoltz and imcheng's comments #Patch Set 11 : Temporarily fix Windows and Mac compilation #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 47 (17 generated)
btolsch@chromium.org changed reviewers: + imcheng@chromium.org, mfoltz@chromium.org
This is a singleton version of monitoring network connectivity changes. I'm still a little unsure about the testing, the file placement, and what is best for both chrome.dial and the sink discovery services to come. It also still needs a mac and windows implementation of GetDiscoveryNetworkIdList(), which I will start working on now. So PTAL, thanks!
I am not familiar with the platform-specific logic (though they look reasonable enough from checking with manuals). I imagine this will need to be tested manually before rolling out. It looks like a good start. Some high level comments: - The DiscoveryNetworkMonitor will be shared by DIAL and Cast MediaSinkServices. - We won't need to expose a chrome.dial API. - I am leaning towards putting it in chrome/browser/media/router/discovery right now since it's only used by Media Router. - Can we reuse existing functions to perform some of the lower level processing? (some suggestions in-line) https://codereview.chromium.org/2750453002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/dial/dial_api.h (right): https://codereview.chromium.org/2750453002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/dial_api.h:161: class DialGetNetworkIdFunction : public AsyncExtensionFunction { Mark - I don't believe we need this anymore. The network change monitor will only be used in browser code. Is that correct? https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... File chrome/browser/net/discovery_network_list_posix.cc (right): https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_posix.cc:61: {name, GetMacAddressAsString(ll_addr->sll_addr, ll_addr->sll_halen)}); Can this be done with a combination of base::ToLowerASCII and base::HexEncode? See example: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/music_mana... https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... File chrome/browser/net/discovery_network_list_wifi_linux.cc (right): https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_wifi_linux.cc:16: bool MaybeGetWifiSSID(const char* if_name, std::string* ssid) { Can this be reused? https://cs.chromium.org/chromium/src/net/base/network_interfaces_linux.cc?typ... https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... File chrome/browser/net/discovery_network_monitor.cc (right): https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:48: matching_info_it->network_id = network_id.network_id; So it looks like we are combining information from two lists. Can we actually figure out the "network_id" using only information from |interface_list|? If it's Ethernet, then you can figure out the MAC address with SIOCGIFHWADDR: http://www.microhowto.info/howto/get_the_mac_address_of_an_ethernet_interface... Otherwise it is WiFi, it seems like you want to get its SSID. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:51: return network_info_list; Do we know if the networks in the returned list are all connected? https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:164: return network_id; Is the purpose of this to mimic the Cast MRP logic to determine the "combined" network ID? Does the order of the networks in network_info_list matter? https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... File chrome/browser/net/discovery_network_monitor.h (right): https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.h:114: mutable base::Lock network_info_lock_; // Guards network_id_ and networks_. Does this need lock if we restricted usage of this class to IO thread only?
Overall looks good - mostly localized comments. If possible let's try to re-use existing code to extract network info and convert it to strings as Derek suggests. For net/ reviewers, a short design doc explaining the problem we are trying to solve might be helpful. I will review the unittest and usage of the POSIX APIs more closely in a second pass. I am not sure how to quantify the stability risk here. To err on the conservative side, we may want to put this behind a flag/experiment while we are testing. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/dial/dial_api.h (right): https://codereview.chromium.org/2750453002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/dial_api.h:161: class DialGetNetworkIdFunction : public AsyncExtensionFunction { On 2017/03/14 at 22:45:34, imcheng wrote: > Mark - I don't believe we need this anymore. The network change monitor will only be used in browser code. Is that correct? It's up to you - if we add this, we could test it in the current component (and possibly get performance benefits by removing other event listeners. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... File chrome/browser/net/discovery_network_list_posix.cc (right): https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_posix.cc:40: struct ifaddrs* if_list, const https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_posix.cc:43: for (; if_list != NULL; if_list = if_list->ifa_next) { IF these are guaranteed to be returned in the same order, then we may not have to worry about sorting the id components by name. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_posix.cc:44: struct sockaddr* addr = if_list->ifa_addr; const https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_posix.cc:50: if (MaybeGetWifiSSID(name.data(), &ssid)) { Is there any way to further filter on the interface type before trying to get its WiFi address? https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_posix.cc:55: struct sockaddr_ll* ll_addr = reinterpret_cast<struct sockaddr_ll*>(addr); const https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_posix.cc:71: if (getifaddrs(&if_list)) { If non-zero, DVLOG(2) the result (possibly using net_error_list to translate). https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... File chrome/browser/net/discovery_network_monitor.cc (right): https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:138: return std::string("disconnected"); Can these special network id constants be defined at the file level? https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:150: folded_ids ^= network_ids[i] << (8 * (i % 2)); What is this computing? Maybe you can just compute a SHA-1 of the string. It doesn't have to be secure, just rare to have collisions. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:167: void DiscoveryNetworkMonitor::UpdateNetworkInfo() { Will this get called when WiFi SSIDs change? https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:183: observers_->Notify(FROM_HERE, &Observer::OnNetworksChanged, Do you only want to do this if the network list / id had changed? https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:187: void DiscoveryNetworkMonitor::UpdateNetworkInfoWithCallback( Maybe UpdateNetworkInfo() could always update observers if the underlying id has changed, and take an optional callback to always invoke with the current id? https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... File chrome/browser/net/discovery_network_monitor.h (right): https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.h:42: // (TODO: How to get on Mac?) Move to discovery_network_list_wifi.h? https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.h:44: // (TODO: How to get?) Remove TODO? https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.h:56: // Tracks the set of active network interfaces that can be used for DIAL s/DIAL/local/ https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.h:63: using NetworkInfoStrategy = base::Callback<std::vector<NetworkInfo>()>; NetworkInfoCallback to avoid confusion with the interface defined above https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.h:109: const std::vector<NetworkInfo>& network_info_list); When would this be computed over anything but |networks_|?
I responded to the comments, though some things are still uncertain, so PTAL. The next step will be windows and mac-specific code as well. I will also put together a short design doc for net/ reviewers per Mark's suggestion. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/dial/dial_api.h (right): https://codereview.chromium.org/2750453002/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/dial/dial_api.h:161: class DialGetNetworkIdFunction : public AsyncExtensionFunction { On 2017/03/18 19:58:21, mark a. foltz wrote: > On 2017/03/14 at 22:45:34, imcheng wrote: > > Mark - I don't believe we need this anymore. The network change monitor will > only be used in browser code. Is that correct? > > It's up to you - if we add this, we could test it in the current component (and > possibly get performance benefits by removing other event listeners. I removee it since it sounds like it would only be used temporarily. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... File chrome/browser/net/discovery_network_list_posix.cc (right): https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_posix.cc:40: struct ifaddrs* if_list, On 2017/03/18 19:58:21, mark a. foltz wrote: > const Done. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_posix.cc:43: for (; if_list != NULL; if_list = if_list->ifa_next) { On 2017/03/18 19:58:21, mark a. foltz wrote: > IF these are guaranteed to be returned in the same order, then we may not have > to worry about sorting the id components by name. Acknowledged. I don't know whether that's guaranteed. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_posix.cc:44: struct sockaddr* addr = if_list->ifa_addr; On 2017/03/18 19:58:21, mark a. foltz wrote: > const Done. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_posix.cc:50: if (MaybeGetWifiSSID(name.data(), &ssid)) { On 2017/03/18 19:58:21, mark a. foltz wrote: > Is there any way to further filter on the interface type before trying to get > its WiFi address? We can also check the hardware address to see if it's ethernet. Done. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_posix.cc:55: struct sockaddr_ll* ll_addr = reinterpret_cast<struct sockaddr_ll*>(addr); On 2017/03/18 19:58:21, mark a. foltz wrote: > const Done. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_posix.cc:61: {name, GetMacAddressAsString(ll_addr->sll_addr, ll_addr->sll_halen)}); On 2017/03/14 22:45:34, imcheng wrote: > Can this be done with a combination of base::ToLowerASCII and base::HexEncode? > See example: > https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/music_mana... Done. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_posix.cc:71: if (getifaddrs(&if_list)) { On 2017/03/18 19:58:21, mark a. foltz wrote: > If non-zero, DVLOG(2) the result (possibly using net_error_list to translate). Done. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... File chrome/browser/net/discovery_network_list_wifi_linux.cc (right): https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_list_wifi_linux.cc:16: bool MaybeGetWifiSSID(const char* if_name, std::string* ssid) { On 2017/03/14 22:45:35, imcheng wrote: > Can this be reused? > https://cs.chromium.org/chromium/src/net/base/network_interfaces_linux.cc?typ... Done. Should I move it out of the internal namespace though? https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... File chrome/browser/net/discovery_network_monitor.cc (right): https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:48: matching_info_it->network_id = network_id.network_id; On 2017/03/14 22:45:35, imcheng wrote: > So it looks like we are combining information from two lists. Can we actually > figure out the "network_id" using only information from |interface_list|? > > If it's Ethernet, then you can figure out the MAC address with SIOCGIFHWADDR: > http://www.microhowto.info/howto/get_the_mac_address_of_an_ethernet_interface... > > Otherwise it is WiFi, it seems like you want to get its SSID. Although this is possible on linux, as best as I can tell for mac and windows so far, you can't get the MAC address for a single interface by itself; you have to get a list of interface info for the whole system and then search the list. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:51: return network_info_list; On 2017/03/14 22:45:35, imcheng wrote: > Do we know if the networks in the returned list are all connected? Yes, it looks like net::GetNetworkList only returns interfaces that are connected. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:138: return std::string("disconnected"); On 2017/03/18 19:58:21, mark a. foltz wrote: > Can these special network id constants be defined at the file level? Done. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:150: folded_ids ^= network_ids[i] << (8 * (i % 2)); On 2017/03/18 19:58:21, mark a. foltz wrote: > What is this computing? Maybe you can just compute a SHA-1 of the string. It > doesn't have to be secure, just rare to have collisions. > Done. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:164: return network_id; On 2017/03/14 22:45:35, imcheng wrote: > Is the purpose of this to mimic the Cast MRP logic to determine the "combined" > network ID? > > Does the order of the networks in network_info_list matter? The order shouldn't matter. I added a sort to the IDs before the hash. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:167: void DiscoveryNetworkMonitor::UpdateNetworkInfo() { On 2017/03/18 19:58:21, mark a. foltz wrote: > Will this get called when WiFi SSIDs change? I guess that depends on whether NetworkChangeNotifier sees the connect/disconnect. Is it possible to change a network's SSID without the clients disconnecting? https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:183: observers_->Notify(FROM_HERE, &Observer::OnNetworksChanged, On 2017/03/18 19:58:21, mark a. foltz wrote: > Do you only want to do this if the network list / id had changed? Yes. Done. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.cc:187: void DiscoveryNetworkMonitor::UpdateNetworkInfoWithCallback( On 2017/03/18 19:58:21, mark a. foltz wrote: > Maybe UpdateNetworkInfo() could always update observers if the underlying id has > changed, and take an optional callback to always invoke with the current id? Done. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... File chrome/browser/net/discovery_network_monitor.h (right): https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.h:42: // (TODO: How to get on Mac?) On 2017/03/18 19:58:22, mark a. foltz wrote: > Move to discovery_network_list_wifi.h? Done. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.h:44: // (TODO: How to get?) On 2017/03/18 19:58:22, mark a. foltz wrote: > Remove TODO? Done. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.h:56: // Tracks the set of active network interfaces that can be used for DIAL On 2017/03/18 19:58:22, mark a. foltz wrote: > s/DIAL/local/ Done. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.h:63: using NetworkInfoStrategy = base::Callback<std::vector<NetworkInfo>()>; On 2017/03/18 19:58:22, mark a. foltz wrote: > NetworkInfoCallback to avoid confusion with the interface defined above Done. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.h:109: const std::vector<NetworkInfo>& network_info_list); On 2017/03/18 19:58:21, mark a. foltz wrote: > When would this be computed over anything but |networks_|? To minimize the lock section, this is computed on a separate vector and then both are just .swap()ed under the lock. After removing the lock, I left it this way because I keep both vectors around to check for a change of networks. https://codereview.chromium.org/2750453002/diff/1/chrome/browser/net/discover... chrome/browser/net/discovery_network_monitor.h:114: mutable base::Lock network_info_lock_; // Guards network_id_ and networks_. On 2017/03/14 22:45:35, imcheng wrote: > Does this need lock if we restricted usage of this class to IO thread only? No we wouldn't. I was thinking it was simpler to make this thread-safe and let the extension function and sink discovery services use whichever threads they wanted. I removed this and restricted calls to the IO thread instead.
btolsch@chromium.org changed reviewers: + rsleevi@chromium.org
+rsleevi: PTAL at the use of the net::internal code (especially if it should be moved out of net::internal to be used like this) and the general design of the network monitor. For background, a brief design doc for this patch, including a link to the broader design doc, is here: https://docs.google.com/a/google.com/document/d/1h_d5Cg16wZygMQ_LBMOXgK6-8CWm...
rsleevi@chromium.org changed reviewers: + pauljensen@chromium.org
Paul's probably a better reviewer, but our whole goal of signalling with net::internal is that consumers should not be relying on this, and we're not supporting that API beyond internal uses. I'm not sure if we're willing or able to take on the external support there, but Paul probably knows better here.
On 2017/04/13 18:06:54, Ryan Sleevi wrote: > Paul's probably a better reviewer, but our whole goal of signalling with > net::internal is that consumers should not be relying on this, and we're not > supporting that API beyond internal uses. > > I'm not sure if we're willing or able to take on the external support there, but > Paul probably knows better here. In 40 minutes I'm going OOO until the 24th. There is a lot of different WiFi SSID reading code in net/, some of it is using deprecated methods, some not. What O/Ss do you want this to work on? Sorry I don't know anything about chrome/browser/media/router.
On 2017/04/13 18:23:26, pauljensen wrote: > On 2017/04/13 18:06:54, Ryan Sleevi wrote: > > Paul's probably a better reviewer, but our whole goal of signalling with > > net::internal is that consumers should not be relying on this, and we're not > > supporting that API beyond internal uses. > > > > I'm not sure if we're willing or able to take on the external support there, > but > > Paul probably knows better here. > > In 40 minutes I'm going OOO until the 24th. There is a lot of different WiFi > SSID reading code in net/, some of it is using deprecated methods, some not. > What O/Ss do you want this to work on? Sorry I don't know anything about > chrome/browser/media/router. I believe we're targeting ChromeOS/Mac/Windows/Linux, but Mark and/or Derek can chime in if that's wrong. In PS1, I wrote essentially the same ioctl code as the net::internal function, so we can always go back to the duplication if you'd rather we not touch, depend on, or move net::internal code.
On 2017/04/13 18:35:13, btolsch wrote: > On 2017/04/13 18:23:26, pauljensen wrote: > > On 2017/04/13 18:06:54, Ryan Sleevi wrote: > > > Paul's probably a better reviewer, but our whole goal of signalling with > > > net::internal is that consumers should not be relying on this, and we're not > > > supporting that API beyond internal uses. > > > > > > I'm not sure if we're willing or able to take on the external support there, > > but > > > Paul probably knows better here. > > > > In 40 minutes I'm going OOO until the 24th. There is a lot of different WiFi > > SSID reading code in net/, some of it is using deprecated methods, some not. > > What O/Ss do you want this to work on? Sorry I don't know anything about > > chrome/browser/media/router. > > I believe we're targeting ChromeOS/Mac/Windows/Linux, but Mark and/or Derek can > chime in if that's wrong. In PS1, I wrote essentially the same ioctl code as > the net::internal function, so we can always go back to the duplication if > you'd rather we not touch, depend on, or move net::internal code. There is the non-internal net::GetWifiSSID() method that works on Linux, ChromeOS, Android and Windows. Can you explain why you need to know for a particular interface?
On 2017/04/13 18:39:12, pauljensen wrote: > On 2017/04/13 18:35:13, btolsch wrote: > > On 2017/04/13 18:23:26, pauljensen wrote: > > > On 2017/04/13 18:06:54, Ryan Sleevi wrote: > > > > Paul's probably a better reviewer, but our whole goal of signalling with > > > > net::internal is that consumers should not be relying on this, and we're > not > > > > supporting that API beyond internal uses. > > > > > > > > I'm not sure if we're willing or able to take on the external support > there, > > > but > > > > Paul probably knows better here. > > > > > > In 40 minutes I'm going OOO until the 24th. There is a lot of different > WiFi > > > SSID reading code in net/, some of it is using deprecated methods, some not. > > > > What O/Ss do you want this to work on? Sorry I don't know anything about > > > chrome/browser/media/router. > > > > I believe we're targeting ChromeOS/Mac/Windows/Linux, but Mark and/or Derek > can > > chime in if that's wrong. In PS1, I wrote essentially the same ioctl code as > > the net::internal function, so we can always go back to the duplication if > > you'd rather we not touch, depend on, or move net::internal code. > > There is the non-internal net::GetWifiSSID() method that works on Linux, > ChromeOS, Android and Windows. Can you explain why you need to know for a > particular interface? We want to know the complete set of networks we are connected to (WiFi and ethernet) in order to cache devices like Chromecasts per-network, so we can more quickly go through the discovery/resolution process when connecting to a network we've seen before. This is in the doc FWIW. There isn't really a "correctness" problem with always using a single SSID but it's less optimal (though admittedly also most common).
On 2017/04/13 18:43:56, btolsch wrote: > On 2017/04/13 18:39:12, pauljensen wrote: > > On 2017/04/13 18:35:13, btolsch wrote: > > > On 2017/04/13 18:23:26, pauljensen wrote: > > > > On 2017/04/13 18:06:54, Ryan Sleevi wrote: > > > > > Paul's probably a better reviewer, but our whole goal of signalling with > > > > > net::internal is that consumers should not be relying on this, and we're > > not > > > > > supporting that API beyond internal uses. > > > > > > > > > > I'm not sure if we're willing or able to take on the external support > > there, > > > > but > > > > > Paul probably knows better here. > > > > > > > > In 40 minutes I'm going OOO until the 24th. There is a lot of different > > WiFi > > > > SSID reading code in net/, some of it is using deprecated methods, some > not. > > > > > > What O/Ss do you want this to work on? Sorry I don't know anything about > > > > chrome/browser/media/router. > > > > > > I believe we're targeting ChromeOS/Mac/Windows/Linux, but Mark and/or Derek > > can > > > chime in if that's wrong. In PS1, I wrote essentially the same ioctl code > as > > > the net::internal function, so we can always go back to the duplication if > > > you'd rather we not touch, depend on, or move net::internal code. > > > > There is the non-internal net::GetWifiSSID() method that works on Linux, > > ChromeOS, Android and Windows. Can you explain why you need to know for a > > particular interface? > > We want to know the complete set of networks we are connected to (WiFi and > ethernet) in order to cache devices like Chromecasts per-network, so we can > more quickly go through the discovery/resolution process when connecting to a > network we've seen before. This is in the doc FWIW. > > There isn't really a "correctness" problem with always using a single SSID > but it's less optimal (though admittedly also most common). Unless you're trying to handle multihomed devices, which I hope you're not, could you use NetworkChangeNotifier::GetCurrentConnectionType() to see which network is default, and if it's WiFI, use net::GetWifiSSID()?
Overall looks good so far. Some detailed questions about the glibc code to iterate over interfaces, and a few other minor comments on documentation. I'd like to see the code in the monitor that merges data from the network lists simplified if possible. Can the two lists be put into the same order before merging? I didn't look at the unit test closely in this pass. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list.h (right): https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list.h:12: std::string interface_name; Add a comment with examples: // eth0, wlan0 https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list.h:13: std::string network_id; Add a comment explaining this briefly? https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list_posix.cc (right): https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:36: if (addr->sa_family != ARPHRD_ETHER && Won't this always be AF_PACKET per line 32? From looking at the getsockaddr interface, you'll have to parse the name (ugh) or dig around in ifa_data to detect WiFi interfaces, either of which seems dodgy. I think the network change detector *may* have some code that can do this? https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:38: network_ids->push_back({name, ssid}); Is MaybeGetWifiSSID guaranteed to return a result? The name doesn't sound promising. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:43: reinterpret_cast<const struct sockaddr_ll*>(addr); Can you add a comment about why |addr| is guaranteed to be a sockaddr_ll? Is it true for all sa_family == PACKET? https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:65: Extra newline https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:67: Ditto https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list_unittest.cc (right): https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_unittest.cc:14: auto network_ids = GetDiscoveryNetworkIdList(); It looks like you're not mocking anything out but assuming empty results are returned. Can you explain what you are testing? https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list_wifi.h (right): https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_wifi.h:10: // TODO: How to get on Mac? TODO(crbug.com/XXXX): Implement for Mac https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_monitor.cc (right): https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:34: std::vector<NetworkInfo> network_info_list; Maybe this should be a std::Map<std::string, NetworkInfo> to simplify the code below? https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:45: auto network_ids = GetDiscoveryNetworkIdList(); Or we could have this return results in the same index order as network_info_list. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:53: matching_info_it->network_id = network_id.network_id; What if an entry doesn't have a network_id in |network_ids|? https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:169: std::sort(id_list.begin(), id_list.end()); Would it be simpler to implement a comparator function over a copy of network_info_list and them iterate over that, versus extracting and sorting? https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:172: combined_ids = combined_ids + id; Nit: You may want to insert a delimeter like ! between ids to prevent aliasing. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:189: if (network_id_ != network_id || I think only this check is necessary. Because of the stable sorting, if the ids are a permutation they should generate the same id- that's an invariant we want for the network_id. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_monitor.h (right): https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:49: // Currently local discovery is only supported over IPv4. I believe mDNS is supported over IPv6 - but am not sure. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:84: void ForceNetworkInfoRefresh(NetworkRefreshCompleteCallback callback); Refresh() and document forcing behavior? https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:92: // Computes a stable string identifier from the list of active networks. s/active networks/connected interfaces/ https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:93: // Returns "disconnected" if there are no active networks or "unknown" if the Declare kConstant constexprs for these special values in the .h so client code can use them.
I've addressed the latest comments as well as added back the chrome.dial API along with tests. I also copied net::internal::GetInterfaceSSID into MaybeGetWifiSSID to avoid the net::internal dependency for now. In addition to the question about how to deal with Wifi SSIDs, I think there may be a problem with our use of NetworkChangeNotifier now. I'm putting together a design doc describing these problems which I'll send out soon. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list.h (right): https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list.h:12: std::string interface_name; On 2017/04/18 20:36:59, mark a. foltz wrote: > Add a comment with examples: > // eth0, wlan0 Done. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list.h:13: std::string network_id; On 2017/04/18 20:36:59, mark a. foltz wrote: > Add a comment explaining this briefly? Done. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list_posix.cc (right): https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:36: if (addr->sa_family != ARPHRD_ETHER && On 2017/04/18 20:37:00, mark a. foltz wrote: > Won't this always be AF_PACKET per line 32? > > From looking at the getsockaddr interface, you'll have to parse the name (ugh) > or dig around in ifa_data to detect WiFi interfaces, either of which seems > dodgy. I think the network change detector *may* have some code that can do > this? Yes, I messed up the field; it should be sll_hatype. Also, checking this actually doesn't distinguish ethernet from wifi, but rather distinguishes ethernet _and_ wifi from other interfaces. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:38: network_ids->push_back({name, ssid}); On 2017/04/18 20:37:00, mark a. foltz wrote: > Is MaybeGetWifiSSID guaranteed to return a result? The name doesn't sound > promising. Yes, if it returns true then ssid will be non-empty. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:43: reinterpret_cast<const struct sockaddr_ll*>(addr); On 2017/04/18 20:37:00, mark a. foltz wrote: > Can you add a comment about why |addr| is guaranteed to be a sockaddr_ll? Is it > true for all sa_family == PACKET? Done. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:65: On 2017/04/18 20:37:00, mark a. foltz wrote: > Extra newline Done. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:67: On 2017/04/18 20:37:00, mark a. foltz wrote: > Ditto Done. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list_unittest.cc (right): https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_unittest.cc:14: auto network_ids = GetDiscoveryNetworkIdList(); On 2017/04/18 20:37:00, mark a. foltz wrote: > It looks like you're not mocking anything out but assuming empty results are > returned. Can you explain what you are testing? I am testing that the interface name and network ID are non-empty. Since we can't mock out the OS layer and therefore can't control what we get, this is all we can really do. I added comments explaining this and also changed the expectations to hopefully be clearer. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list_wifi.h (right): https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_wifi.h:10: // TODO: How to get on Mac? On 2017/04/18 20:37:00, mark a. foltz wrote: > TODO(crbug.com/XXXX): Implement for Mac Done. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_monitor.cc (right): https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:34: std::vector<NetworkInfo> network_info_list; On 2017/04/18 20:37:00, mark a. foltz wrote: > Maybe this should be a std::Map<std::string, NetworkInfo> to simplify the code > below? Done. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:45: auto network_ids = GetDiscoveryNetworkIdList(); On 2017/04/18 20:37:00, mark a. foltz wrote: > Or we could have this return results in the same index order as > network_info_list. The only way to ensure that would be to pass |network_info_list| in and have it do essentially what we're doing here. On linux, for example, GetNetworkList uses netlink sockets and sorts its results by IP but we're using getifaddrs. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:53: matching_info_it->network_id = network_id.network_id; On 2017/04/18 20:37:00, mark a. foltz wrote: > What if an entry doesn't have a network_id in |network_ids|? I think the only option would be to set the network_id in NetworkInfo to either empty (what happens currently) or maybe to the interface name. Do you think one of these is preferable? https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:169: std::sort(id_list.begin(), id_list.end()); On 2017/04/18 20:37:00, mark a. foltz wrote: > Would it be simpler to implement a comparator function over a copy of > network_info_list and them iterate over that, versus extracting and sorting? I don't think copying and sorting with a comparator would be much simpler than this. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:172: combined_ids = combined_ids + id; On 2017/04/18 20:37:00, mark a. foltz wrote: > Nit: You may want to insert a delimeter like ! between ids to prevent aliasing. Done. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:189: if (network_id_ != network_id || On 2017/04/18 20:37:00, mark a. foltz wrote: > I think only this check is necessary. Because of the stable sorting, if the ids > are a permutation they should generate the same id- that's an invariant we want > for the network_id. Done. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_monitor.h (right): https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:49: // Currently local discovery is only supported over IPv4. On 2017/04/18 20:37:01, mark a. foltz wrote: > I believe mDNS is supported over IPv6 - but am not sure. It is although I don't know if chromium uses it. Either way, this is just leftover from the dial patch. Removed. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:84: void ForceNetworkInfoRefresh(NetworkRefreshCompleteCallback callback); On 2017/04/18 20:37:01, mark a. foltz wrote: > Refresh() and document forcing behavior? Done. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:92: // Computes a stable string identifier from the list of active networks. On 2017/04/18 20:37:01, mark a. foltz wrote: > s/active networks/connected interfaces/ Done. https://codereview.chromium.org/2750453002/diff/20001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:93: // Returns "disconnected" if there are no active networks or "unknown" if the On 2017/04/18 20:37:01, mark a. foltz wrote: > Declare kConstant constexprs for these special values in the .h so client code > can use them. Done.
Next round of comments. Did you have a link to the document handy? https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/dial/dial_api.cc (right): https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/dial/dial_api.cc:89: base::Bind(&DialAPI::NetworkIdListenerRemovedOnIOThread, this)); What if multiple event listeners are added? Do you need to keep a counter? https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/dial/dial_api.cc:334: network_id.size() == 0 ? std::string("unknown") : network_id)); Can the NetworkMonitor return "unknown" in place of the empty string, rather than translating here? https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/dial/dial_api.h (right): https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/dial/dial_api.h:51: public DiscoveryNetworkMonitor::Observer { I'd put the extensions API changes in a separate patch for review by one of the extensions OWNERs. https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/dial/dial_api.h:184: void NetworkInfoReadyCallback(); OnNetworkInfoReady https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list.h (right): https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list.h:15: // connected. For ethernet, this will simply be the ethernet adapter's MAC The SSID identifies a network, while the MAC address identifies a device. You might clarify the two cases: "For WiFi, we assume that the SSID identifies the connected network. For Ethernet, we assume the network remains the same for a given device, so we use its MAC address." https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list_posix.cc (right): https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:50: network_ids->push_back({name, base::ToLowerASCII(base::HexEncode( Is it necessary to convert the hex bytes to lower case, given it will be input to a hash anyway? https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list_wifi_linux.cc (right): https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_wifi_linux.cc:19: DCHECK(if_name); DCHECK(ssid_out) https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_wifi_linux.cc:25: strncpy(wreq.ifr_name, if_name, IFNAMSIZ - 1); Is wreq.ifr_name guaranteed to hold IFNAMSIZE bytes? Consider using base::strlcpy to guarantee null termination of ifr_name. https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_wifi_linux.cc:32: maybe_ssid.assign(ssid); Are you certain ssid will be null terminated? https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_wifi_linux.cc:35: *ssid_out = std::move(maybe_ssid); Can you just assign ssid to ssid_out directly?
Addressed more comments, PTAL. https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/dial/dial_api.cc (right): https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/dial/dial_api.cc:89: base::Bind(&DialAPI::NetworkIdListenerRemovedOnIOThread, this)); On 2017/04/26 22:25:11, mark a. foltz wrote: > What if multiple event listeners are added? Do you need to keep a counter? From testing and looking at the code it wasn't clear that would happen. After looking at other EventRouter::Observer implementations though, I guess it can. I added a counter here to check for the first and last listener. https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/dial/dial_api.cc:334: network_id.size() == 0 ? std::string("unknown") : network_id)); On 2017/04/26 22:25:11, mark a. foltz wrote: > Can the NetworkMonitor return "unknown" in place of the empty string, rather > than translating here? Yes, it already doesn't return an empty string so this was unnecessary. Done. https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/dial/dial_api.h (right): https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/dial/dial_api.h:51: public DiscoveryNetworkMonitor::Observer { On 2017/04/26 22:25:11, mark a. foltz wrote: > I'd put the extensions API changes in a separate patch for review by one of the > extensions OWNERs. Done. https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/dial/dial_api.h:184: void NetworkInfoReadyCallback(); On 2017/04/26 22:25:11, mark a. foltz wrote: > OnNetworkInfoReady Done. https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list.h (right): https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list.h:15: // connected. For ethernet, this will simply be the ethernet adapter's MAC On 2017/04/26 22:25:11, mark a. foltz wrote: > The SSID identifies a network, while the MAC address identifies a device. You > might clarify the two cases: > > "For WiFi, we assume that the SSID identifies the connected network. For > Ethernet, we assume the network remains the same for a given device, so we use > its MAC address." Done. https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list_posix.cc (right): https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:50: network_ids->push_back({name, base::ToLowerASCII(base::HexEncode( On 2017/04/26 22:25:11, mark a. foltz wrote: > Is it necessary to convert the hex bytes to lower case, given it will be input > to a hash anyway? No it's not. Removed. https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list_wifi_linux.cc (right): https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_wifi_linux.cc:19: DCHECK(if_name); On 2017/04/26 22:25:12, mark a. foltz wrote: > DCHECK(ssid_out) Done. https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_wifi_linux.cc:25: strncpy(wreq.ifr_name, if_name, IFNAMSIZ - 1); On 2017/04/26 22:25:11, mark a. foltz wrote: > Is wreq.ifr_name guaranteed to hold IFNAMSIZE bytes? > > Consider using base::strlcpy to guarantee null termination of ifr_name. Yes, wreq.ifr_name is defined as IFNAMSIZ bytes (unless this were defined as a different value in a different kernel, then we'd be in trouble). wreq = {}; strncpy(..., IFNAMSIZ - 1) also guarantees null-termination. https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_wifi_linux.cc:32: maybe_ssid.assign(ssid); On 2017/04/26 22:25:12, mark a. foltz wrote: > Are you certain ssid will be null terminated? Yes. |ssid| above is defined with an extra byte which the ioctl won't touch, unless you think I should double-defend and check that the kernel didn't write beyond the length I gave it. https://codereview.chromium.org/2750453002/diff/40001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_wifi_linux.cc:35: *ssid_out = std::move(maybe_ssid); On 2017/04/26 22:25:12, mark a. foltz wrote: > Can you just assign ssid to ssid_out directly? D'oh, bad inlining by me. Done.
Getting close! A few simplifications requested for the network monitor. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list.h (right): https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list.h:17: // same for each device, so we use the device's MAC address to identify the s/for each device/for the interface (until disconnected)/ s/the device's/the interface's/ https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list_posix.cc (right): https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:29: if (name.size() == 0 || addr->sa_family != AF_PACKET) { As a future improvement, we could interrogate the interface to see if it supports multicast (ifa_flags contains IFF_MULTICAST) and use this API to return a list of addresses to use for local discovery. However we'd have to plumb that down into the implementations of DIAL and mDNS. No changes requested for now :) https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:36: if (ll_addr->sll_hatype != ARPHRD_ETHER) { It would be interesting to see if we could detect WiFi vs. wired ethernet from |if_list|, versus trying to extract SSID for all interfaces. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:40: if (MaybeGetWifiSSID(name.data(), &ssid)) { Just pass |name|. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list_wifi.h (right): https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_wifi.h:11: bool MaybeGetWifiSSID(const char* if_name, std::string* ssid); const std::string& if_name https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_monitor.cc (right): https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:24: net::INCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES)) { Can you add a comment explaining this flag? https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:34: } Beyond checking that there is at least one ETHERNET or WIFI interface, I'm not sure I follow why we need to return information from |interface_list| and combine it with the network_id_list. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:40: interface.name, NetworkInfo{interface.interface_index, interface.type, I don't think this is needed to compute the network_id. Just drop network_info_map? https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:43: If network_info_map is empty, early return. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:45: for (const auto& network_id : network_ids) { If network_ids is empty, early return. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:151: const char DiscoveryNetworkMonitor::kNetworkIdDisconnected[] = "disconnected"; constexpr char here and below https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:155: std::string DiscoveryNetworkMonitor::ComputeNetworkId( I don't see a reference to |this|; move to anonymous namespace as a static function? https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:195: if (!std::is_permutation(networks_.begin(), networks_.end(), Can you just compare the previous and the new network_id? https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_monitor.h (right): https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:20: // Represents a single network interface that can be used for discovery. ...for local discovery https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:43: // A unique identifier for the network interface. network interface and its connected network. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:51: // DiscoveryNetworkMonitor::Observer is called with the instance of the manager. s/manager/monitor/ here and elsewhere https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:52: // Only one instance of this should be created per browser process. Please document threading assumptions for this class. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:76: // |networks_|. |callback| will be called after the refresh. Is the point to trigger OnNetworksChanged if there has been a change in the set of connected networks? Updating the internal state is an implementation detail. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:80: // Returns "disconnected" if there are no connected interfaces or "unknown" if s/"disconnected"/kNetworkIdDisconnected/ etc https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:84: return network_id_; I think the DCHECK() disqualifies this from inlining, but I don't feel strongly. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:89: net::NetworkChangeNotifier::ConnectionType type) override; Can this be private?
Getting close! A few simplifications requested for the network monitor.
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
Apologies for the drive-by question, but how does this differ from this code that discovers and detects WiFi networks? https://cs.chromium.org/chromium/src/components/wifi/wifi_service.h?type=cs
On 2017/05/16 at 17:28:24, rsesek wrote: > Apologies for the drive-by question, but how does this differ from this code that discovers and detects WiFi networks? https://cs.chromium.org/chromium/src/components/wifi/wifi_service.h?type=cs Brandon has a document outlining the differences...
I responded to the comments, though there may still be some open questions. PTAL, thanks. Also, the Windows and Mac patch is started if you are interested in that: https://codereview.chromium.org/2868213004/ Lastly, the doc to which Mark referred, which talks about some of the network information chromium already has, is here: https://docs.google.com/a/google.com/document/d/1vbjDREw9rkuB23lmmrmNefo5b2Yq... https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list.h (right): https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list.h:17: // same for each device, so we use the device's MAC address to identify the On 2017/05/04 23:16:38, mark a. foltz wrote: > s/for each device/for the interface (until disconnected)/ > s/the device's/the interface's/ Done. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list_posix.cc (right): https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:29: if (name.size() == 0 || addr->sa_family != AF_PACKET) { On 2017/05/04 23:16:39, mark a. foltz wrote: > As a future improvement, we could interrogate the interface to see if it > supports multicast (ifa_flags contains IFF_MULTICAST) and use this API to return > a list of addresses to use for local discovery. However we'd have to plumb that > down into the implementations of DIAL and mDNS. > > No changes requested for now :) Acknowledged. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:36: if (ll_addr->sll_hatype != ARPHRD_ETHER) { On 2017/05/04 23:16:39, mark a. foltz wrote: > It would be interesting to see if we could detect WiFi vs. wired ethernet from > |if_list|, versus trying to extract SSID for all interfaces. AFAIK, another ioctl is required either way. We could do an SIOCGIWNAME to check for the existence of the wireless extension on the interface, but I think we might as well just do SIOCGIWESSID (network name) in that case. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:40: if (MaybeGetWifiSSID(name.data(), &ssid)) { On 2017/05/04 23:16:38, mark a. foltz wrote: > Just pass |name|. Done. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_list_wifi.h (right): https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_list_wifi.h:11: bool MaybeGetWifiSSID(const char* if_name, std::string* ssid); On 2017/05/04 23:16:39, mark a. foltz wrote: > const std::string& if_name Done. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_monitor.cc (right): https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:24: net::INCLUDE_HOST_SCOPE_VIRTUAL_INTERFACES)) { On 2017/05/04 23:16:42, mark a. foltz wrote: > Can you add a comment explaining this flag? This is removed as I'm no longer using GetNetworkList at all. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:34: } On 2017/05/04 23:16:41, mark a. foltz wrote: > Beyond checking that there is at least one ETHERNET or WIFI interface, I'm not > sure I follow why we need to return information from |interface_list| and > combine it with the network_id_list. This served two purposes: populating |networks_| (which I now realize is no longer publicly accessible so isn't important) and checking which interfaces are up. The latter can actually be accomplished in GetDiscoveryNetworkIdList, so all this is removed. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:40: interface.name, NetworkInfo{interface.interface_index, interface.type, On 2017/05/04 23:16:41, mark a. foltz wrote: > I don't think this is needed to compute the network_id. Just drop > network_info_map? Done. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:43: On 2017/05/04 23:16:39, mark a. foltz wrote: > If network_info_map is empty, early return. Done. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:45: for (const auto& network_id : network_ids) { On 2017/05/04 23:16:41, mark a. foltz wrote: > If network_ids is empty, early return. Done. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:151: const char DiscoveryNetworkMonitor::kNetworkIdDisconnected[] = "disconnected"; On 2017/05/04 23:16:41, mark a. foltz wrote: > constexpr char here and below Done. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:155: std::string DiscoveryNetworkMonitor::ComputeNetworkId( On 2017/05/04 23:16:42, mark a. foltz wrote: > I don't see a reference to |this|; move to anonymous namespace as a static > function? Done. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.cc:195: if (!std::is_permutation(networks_.begin(), networks_.end(), On 2017/05/04 23:16:40, mark a. foltz wrote: > Can you just compare the previous and the new network_id? Not necessarily. The ID, when it isn't unknown/disconnected, is a hash which could experience collisions. On the other hand, if you feel this is too expensive, and as long as other discovery layers aren't completely dependent on this for accurate network updates (which I believe they aren't), we could just compare IDs. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... File chrome/browser/media/router/discovery/discovery_network_monitor.h (right): https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:20: // Represents a single network interface that can be used for discovery. On 2017/05/04 23:16:44, mark a. foltz wrote: > ...for local discovery Done. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:43: // A unique identifier for the network interface. On 2017/05/04 23:16:43, mark a. foltz wrote: > network interface and its connected network. Done. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:51: // DiscoveryNetworkMonitor::Observer is called with the instance of the manager. On 2017/05/04 23:16:44, mark a. foltz wrote: > s/manager/monitor/ here and elsewhere Done. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:52: // Only one instance of this should be created per browser process. On 2017/05/04 23:16:44, mark a. foltz wrote: > Please document threading assumptions for this class. Done. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:76: // |networks_|. |callback| will be called after the refresh. On 2017/05/04 23:16:43, mark a. foltz wrote: > Is the point to trigger OnNetworksChanged if there has been a change in the set > of connected networks? Updating the internal state is an implementation detail. The internal state update and observer calls could happen as a result of this call, but the point is really to have a polling-type interface for the chrome.dial API (no longer part of this patch, but will probably be added later). I updated the comment but I could also remove this until the chrome.dial API is added. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:80: // Returns "disconnected" if there are no connected interfaces or "unknown" if On 2017/05/04 23:16:43, mark a. foltz wrote: > s/"disconnected"/kNetworkIdDisconnected/ etc Done. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:84: return network_id_; On 2017/05/04 23:16:43, mark a. foltz wrote: > I think the DCHECK() disqualifies this from inlining, but I don't feel strongly. Done. https://codereview.chromium.org/2750453002/diff/80001/chrome/browser/media/ro... chrome/browser/media/router/discovery/discovery_network_monitor.h:89: net::NetworkChangeNotifier::ConnectionType type) override; On 2017/05/04 23:16:42, mark a. foltz wrote: > Can this be private? Done.
Thanks for your patience, a few more tweaks and style fixes noted. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_info.h (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_info.h:6: #define CHROME_BROWSER_MEDIA_ROUTER_DISCOVERY_DISCOVERY_NETWORK_INFO_H_ The main purpose of this file seems to be to support the API in discovery_network_list.h, so it would be possible to fold this file into that one. It depends on whether there are other files that just need the struct and not GetDiscoveryNetworkList, in which case, it would be better to keep them separate. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_info.h:15: ~DiscoveryNetworkInfo() = default; If you're declaring the default ctor in the .cc you should declare the default dtor there as well. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor.cc (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:42: std::sort(id_list.begin(), id_list.end()); 1. Suggest using std::stable_sort to preserve order of equivalents. http://www.cplusplus.com/reference/algorithm/stable_sort/ 2. Instead of transforming the original list and sorting, pass a Compare functor to implement a custom comparison function. 3. ISTM that GetDiscoveryNetworkInfoList() could be responsible for returning its results in a stable sorted order using the logic here, then this function is purely about computing the id. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:77: monitor->network_info_callback_ = base::Bind(&GetNetworkInfo); I don't think this needs to be a base::Callback; can you just store a function pointer to GetDiscoveryNetworkInfoList as network_info_function_? https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:83: DiscoveryNetworkMonitor* DiscoveryNetworkMonitor::GetInstanceForTest( Rather than have a separate factory function for testing, is it possible to have a SetNetworkInfoFunctionForTest() function to override |network_info_callback_|? https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:126: // static Please reorder definitions in this file to match declaration order in the .h https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:142: if (networks_.size() != network_info_list.size() || Can you compare network_id and the previous value of |network_id_| instead? I think the point of network_id is to hash the contents of a specific network list. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor.h (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.h:64: // Constants for the special states of network Id. Constants should be declared before functions: https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.h:80: std::string network_id_; Can you add documentation for these fields?
Thanks for the comments. I responded to them as well as a few other minor changes, so PTAL. I also have these files removed from the Android build since discovery_network_list_posix.cc depends on ifaddrs.h which Android doesn't seem to have. It's not clear to me whether we want this to work for Android (since the extension never ran on Android), so let me know if this needs to change. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_info.h (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_info.h:6: #define CHROME_BROWSER_MEDIA_ROUTER_DISCOVERY_DISCOVERY_NETWORK_INFO_H_ On 2017/05/26 21:38:54, mark a. foltz wrote: > The main purpose of this file seems to be to support the API in > discovery_network_list.h, so it would be possible to fold this file into that > one. > > It depends on whether there are other files that just need the struct and not > GetDiscoveryNetworkList, in which case, it would be better to keep them > separate. Well discovery_network_monitor.h needs this struct, but only discovery_network_monitor.cc needs GetDiscoveryNetworkInfoList. I'll leave it as is for now but I'm not opposed to merging it with discovery_network_list.h https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_info.h:15: ~DiscoveryNetworkInfo() = default; On 2017/05/26 21:38:54, mark a. foltz wrote: > If you're declaring the default ctor in the .cc you should declare the default > dtor there as well. Done. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor.cc (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:42: std::sort(id_list.begin(), id_list.end()); On 2017/05/26 21:38:54, mark a. foltz wrote: > 1. Suggest using std::stable_sort to preserve order of equivalents. > > http://www.cplusplus.com/reference/algorithm/stable_sort/ > > 2. Instead of transforming the original list and sorting, pass a Compare functor > to implement a custom comparison function. > > 3. ISTM that GetDiscoveryNetworkInfoList() could be responsible for returning > its results in a stable sorted order using the logic here, then this function is > purely about computing the id. 1. Done. 2. Done. 3. Done. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:77: monitor->network_info_callback_ = base::Bind(&GetNetworkInfo); On 2017/05/26 21:38:54, mark a. foltz wrote: > I don't think this needs to be a base::Callback; can you just store a function > pointer to GetDiscoveryNetworkInfoList as network_info_function_? Done. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:83: DiscoveryNetworkMonitor* DiscoveryNetworkMonitor::GetInstanceForTest( On 2017/05/26 21:38:54, mark a. foltz wrote: > Rather than have a separate factory function for testing, is it possible to have > a SetNetworkInfoFunctionForTest() function to override |network_info_callback_|? Done. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:126: // static On 2017/05/26 21:38:54, mark a. foltz wrote: > Please reorder definitions in this file to match declaration order in the .h Done. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:142: if (networks_.size() != network_info_list.size() || On 2017/05/26 21:38:54, mark a. foltz wrote: > Can you compare network_id and the previous value of |network_id_| instead? I > think the point of network_id is to hash the contents of a specific network > list. As mentioned before, there's a collision risk but I will compare network IDs instead. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor.h (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.h:64: // Constants for the special states of network Id. On 2017/05/26 21:38:55, mark a. foltz wrote: > Constants should be declared before functions: > > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.h:80: std::string network_id_; On 2017/05/26 21:38:55, mark a. foltz wrote: > Can you add documentation for these fields? Done.
https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/BUILD.gn (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/BUILD.gn:42: "discovery_network_list_posix.cc", It seems we should put platform specific files into conditionals? https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_info.cpp (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_info.cpp:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. .cc instead of .cpp? Though see mfoltz@'s comment about merging this with into another file. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_info.cpp:19: bool DiscoveryNetworkInfo::operator==(const DiscoveryNetworkInfo& o) const { nit: Please use a more descriptive name for the param, e.g., other https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_info.h (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_info.h:14: DiscoveryNetworkInfo(std::string name, std::string network_id); const std::string& for both parameters. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_list.h (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_list.h:12: std::vector<DiscoveryNetworkInfo> GetDiscoveryNetworkInfoList(); Please provide documentation for this function. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_list_posix.cc (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:34: if (name.size() == 0 || addr->sa_family != AF_PACKET) { nit: Not a big deal, but seems you can check sa_family before creating the string and checking if it is empty? https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:41: if (ll_addr->sll_hatype != ARPHRD_ETHER) { Can you add a comment here that ARPHRD_ETHER is used to represent both ethernet and wifi, or something to that effect? https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_list_unittest.cc (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_list_unittest.cc:23: auto interface_name_set = std::set<std::string>{}; Can this just be std::set<std::string> interface_name_set; ? https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor.cc (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:105: BrowserThread::PostTask( Can you just use PostTaskAndReply here? https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:118: DCHECK_CURRENTLY_ON(BrowserThread::UI); This means the DiscoveryNetworkMonitor constructor (or at least the AddNetworkChangeObserver call) has to be called from the UI thread. Is that necessary true? https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor.h (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.h:56: void Refresh(NetworkRefreshCompleteCallback callback); |callback| could be const ref?
PS9 includes responses to Derek's comments. PTAL, thanks! https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/BUILD.gn (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/BUILD.gn:42: "discovery_network_list_posix.cc", On 2017/05/26 23:49:00, imcheng wrote: > It seems we should put platform specific files into conditionals? Files like *_posix.cc are filtered out by sources_assignment_filter ( https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?q=buildconfi... ). Are you suggesting it should be in a separate if() block for style reasons? https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_info.cpp (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_info.cpp:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/05/26 23:49:00, imcheng wrote: > .cc instead of .cpp? Though see mfoltz@'s comment about merging this with into > another file. Done. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_info.cpp:19: bool DiscoveryNetworkInfo::operator==(const DiscoveryNetworkInfo& o) const { On 2017/05/26 23:49:00, imcheng wrote: > nit: Please use a more descriptive name for the param, e.g., other Done. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_info.h (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_info.h:14: DiscoveryNetworkInfo(std::string name, std::string network_id); On 2017/05/26 23:49:00, imcheng wrote: > const std::string& for both parameters. Done. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_list.h (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_list.h:12: std::vector<DiscoveryNetworkInfo> GetDiscoveryNetworkInfoList(); On 2017/05/26 23:49:01, imcheng wrote: > Please provide documentation for this function. Done. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_list_posix.cc (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:34: if (name.size() == 0 || addr->sa_family != AF_PACKET) { On 2017/05/26 23:49:01, imcheng wrote: > nit: Not a big deal, but seems you can check sa_family before creating the > string and checking if it is empty? Done. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_list_posix.cc:41: if (ll_addr->sll_hatype != ARPHRD_ETHER) { On 2017/05/26 23:49:01, imcheng wrote: > Can you add a comment here that ARPHRD_ETHER is used to represent both ethernet > and wifi, or something to that effect? Done. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_list_unittest.cc (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_list_unittest.cc:23: auto interface_name_set = std::set<std::string>{}; On 2017/05/26 23:49:01, imcheng wrote: > Can this just be std::set<std::string> interface_name_set; ? Done. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor.cc (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:105: BrowserThread::PostTask( On 2017/05/26 23:49:01, imcheng wrote: > Can you just use PostTaskAndReply here? Done. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:118: DCHECK_CURRENTLY_ON(BrowserThread::UI); On 2017/05/26 23:49:01, imcheng wrote: > This means the DiscoveryNetworkMonitor constructor (or at least the > AddNetworkChangeObserver call) has to be called from the UI thread. Is that > necessary true? You're right, that's not necessary. Removed. Similarly, with the PostTaskAndReply change, it's actually not necessary to restrict Refresh to the UI thread either. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor.h (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.h:56: void Refresh(NetworkRefreshCompleteCallback callback); On 2017/05/26 23:49:01, imcheng wrote: > |callback| could be const ref? From //docs/callback.md, base::Callback should be passed by value when ownership is transferred, e.g. passing to PostTask or running directly.
lgtm % comments. In addition to unit tests, it would be good to do some manual testing and document the results to make sure our implementation is processing the data provided by the OS layer correctly. https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... File chrome/browser/media/router/discovery/BUILD.gn (right): https://codereview.chromium.org/2750453002/diff/120001/chrome/browser/media/r... chrome/browser/media/router/discovery/BUILD.gn:42: "discovery_network_list_posix.cc", On 2017/05/30 09:54:29, btolsch wrote: > On 2017/05/26 23:49:00, imcheng wrote: > > It seems we should put platform specific files into conditionals? > > Files like *_posix.cc are filtered out by sources_assignment_filter ( > https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?q=buildconfi... > ). Are you suggesting it should be in a separate if() block for style reasons? Ah ok. I didn't know about source_assignment_filter. It is fine to leave it as is. https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor.cc (right): https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:56: constexpr char DiscoveryNetworkMonitor::kNetworkIdDisconnected[]; Are these necessary since you've already defined them in the .h? https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:103: Observer>::NotificationType::NOTIFY_EXISTING_ONLY)) { Can we initialize network_info_function_ to &GetNetworkInfo here? https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor_unittest.cc (right): https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor_unittest.cc:32: // NetworkChangeNotifier provides singleton behavior while simultaneously Sorry if this has already been discussed -- can we just create a DiscoveryNetworkMonitor instance in this test rather than using the singleton instance?
lgtm % addressing remaining comments by imcheng@. My comments are just nit picking at this point. It would be good to see if we can fix the unit test to not rely on static state. Let me know if you want to dig into the implementation of the NetworkChangeObserver and figure out how to make this happen. https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_list_unittest.cc (right): https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_list_unittest.cc:13: TEST(DiscoveryNetworkListTest, GetDiscoveryNetworkInfoList) { I am debating if this unit test is worth writing. The first part of the test is really sanity checking the output of the syscall and not testing our code. For example if this first part failed how could we fix it? The second part is testing whether the stable sort is correct and there is some value in that. Can the part that does the stable sort be factored out into its own function so it can be fed a list without making the syscall? https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor.cc (right): https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:25: if (network_info_list.size() == 0) { I slightly prefer omitting the { } for one-line if statements. But remain consistent with the style of surrounding code. https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:28: bool has_nonempty_network_id = false; This could be one liner with std::find_if, and slightly more efficient by loop breaking, but I don't feel strongly. (I like that the std:: algorithms denote a specific loop structure, while others find the use of lambda expressions annoying...) https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor.h (right): https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.h:24: // Only one instance of this should be created per browser process. Nit: s/should/will/ since we enforce this through the singleton traits. https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.h:46: static constexpr char kNetworkIdDisconnected[] = "disconnected"; Nit: Adding non-lowercase-ascii characters to these will mean absolutely no chance of conflict with an actual network id. E.g., __disconnected__
Comments addressed. The only remaining static state in the unit tests is from using a function pointer instead of a closure (but this makes the non-test code cleaner). Let me know if you have any comments on the final changes, otherwise I'll be committing soon. https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_list_unittest.cc (right): https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_list_unittest.cc:13: TEST(DiscoveryNetworkListTest, GetDiscoveryNetworkInfoList) { On 2017/06/05 21:28:42, mark a. foltz wrote: > I am debating if this unit test is worth writing. The first part of the test is > really sanity checking the output of the syscall and not testing our code. For > example if this first part failed how could we fix it? > > The second part is testing whether the stable sort is correct and there is some > value in that. Can the part that does the stable sort be factored out into its > own function so it can be fed a list without making the syscall? The first part makes sure that our code isn't returning empty interface names or empty network ids (for which I have loop breaks/continues). If it fails then I missed some condition which will cause our code to return an empty interface name or network id. The slightly strange part about this is that while this test is passing, we can never get a network id of unknown from the monitor. Maybe the empty test should only be on the interface name? The second part tests the stable sort but also that we only get at most one entry per interface (i.e. not like GetNetworkList). For now, I removed the network id emptiness test, factored out the stable sort so it can be tested separately as well, but otherwise left this test alone. https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor.cc (right): https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:25: if (network_info_list.size() == 0) { On 2017/06/05 21:28:43, mark a. foltz wrote: > I slightly prefer omitting the { } for one-line if statements. But remain > consistent with the style of surrounding code. Acknowledged. https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:28: bool has_nonempty_network_id = false; On 2017/06/05 21:28:43, mark a. foltz wrote: > This could be one liner with std::find_if, and slightly more efficient by loop > breaking, but I don't feel strongly. (I like that the std:: algorithms denote a > specific loop structure, while others find the use of lambda expressions > annoying...) Done. https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:56: constexpr char DiscoveryNetworkMonitor::kNetworkIdDisconnected[]; On 2017/05/31 21:19:55, imcheng wrote: > Are these necessary since you've already defined them in the .h? Yes. In short, any odr-use still requires a definition, e.g. line 26 above which is essentially |return std::string(&kNetworkIdDisconnected[0]);|. https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.cc:103: Observer>::NotificationType::NOTIFY_EXISTING_ONLY)) { On 2017/05/31 21:19:55, imcheng wrote: > Can we initialize network_info_function_ to &GetNetworkInfo here? Done. https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor.h (right): https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.h:24: // Only one instance of this should be created per browser process. On 2017/06/05 21:28:43, mark a. foltz wrote: > Nit: s/should/will/ since we enforce this through the singleton traits. Done. https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor.h:46: static constexpr char kNetworkIdDisconnected[] = "disconnected"; On 2017/06/05 21:28:43, mark a. foltz wrote: > Nit: Adding non-lowercase-ascii characters to these will mean absolutely no > chance of conflict with an actual network id. E.g., __disconnected__ Done. https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... File chrome/browser/media/router/discovery/discovery_network_monitor_unittest.cc (right): https://codereview.chromium.org/2750453002/diff/160001/chrome/browser/media/r... chrome/browser/media/router/discovery/discovery_network_monitor_unittest.cc:32: // NetworkChangeNotifier provides singleton behavior while simultaneously On 2017/05/31 21:19:56, imcheng wrote: > Sorry if this has already been discussed -- can we just create a > DiscoveryNetworkMonitor instance in this test rather than using the singleton > instance? Done.
The CQ bit was checked by btolsch@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by btolsch@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by btolsch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, imcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2750453002/#ps200001 (title: "Temporarily fix Windows and Mac compilation")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1496778148967180,
"parent_rev": "de612fd93b37b1b369023c7bccc362720837c31e", "commit_rev":
"2bf972dc59af2654f71f577122e39fed4c0a986e"}
Message was sent while issue was closed.
Description was changed from ========== Add DiscoveryNetworkMonitor implementation This changes adds a singleton class DiscoveryNetworkMonitor which provides general information to services doing device discovery on the local network. BUG=698943 ========== to ========== Add DiscoveryNetworkMonitor implementation This changes adds a singleton class DiscoveryNetworkMonitor which provides general information to services doing device discovery on the local network. BUG=698943 Review-Url: https://codereview.chromium.org/2750453002 Cr-Commit-Position: refs/heads/master@{#477378} Committed: https://chromium.googlesource.com/chromium/src/+/2bf972dc59af2654f71f577122e3... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/2bf972dc59af2654f71f577122e3...
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2750453002/diff/200001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2750453002/diff/200001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:3629: # In-browser discovery is not used by Android for now. Please don't pile these at the bottom. Keep the list sorted. |
