Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
245313002:
WiFiService::GetProperties now correctly populates BSSID and Frequency of connected network. (Closed)
|
Created:
6 years, 8 months ago by mef Modified:
6 years, 8 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionWiFiService::GetProperties now correctly populates BSSID and Frequency of connected network.
BUG=364907
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265640
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address codereview comments. #
Total comments: 6
Patch Set 3 : Address codereview comments. #
Total comments: 6
Messages
Total messages: 11 (0 generated)
Hi guys, please take a look.
https://codereview.chromium.org/245313002/diff/1/components/wifi/wifi_service... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/245313002/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:345: void NetworkPropertiesFromBssList(const WLAN_BSS_LIST& wlan_bss_list, suggestions for naming (I understood the current name more in sense 'Create network properties from bss list'): MergeNetworkPropertiesFromBssList UpdateNetworkPropertiesFromBssList Also, I'd mention in the method comment that properties should have guid set, and maybe that the bssid property will not be overwritten if set in |properties|. https://codereview.chromium.org/245313002/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:667: already_connected = (frequency == connected_properties.frequency) && nit: no need for () https://codereview.chromium.org/245313002/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:1248: properties->connection_state = onc::connection_state::kConnected; how does this relate to wlan_connection_attributes->isState from WlanQueryInterface. It would be nice for the connection states in getVisibleNwtworks and getProperties results to be consistent (though, the cast extension _currently_ does not rely on connection states reported by getVisibleNetworks) https://codereview.chromium.org/245313002/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:1265: DOT11_SSID ssid = SSIDFromGUID(properties->guid); this can return immediately if ssid is empty (or maybe add a DCHECK that ssid is valid) https://codereview.chromium.org/245313002/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:1382: wlan_connection_attributes->isState == wlan_interface_state_connected) { can we set connection_state to Connecting for wlan_interface_state_associating wlan_interface_state_discovering wlan_interface_state_authenticating Though, this would be useful only if the state changes during Connect call (i.e. after Connect returns, GetProperties will return 'NotConnected' connection state only if the connection failed). (if it makes sense, I'm fine with doing this separately)
https://codereview.chromium.org/245313002/diff/1/components/wifi/wifi_service... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/245313002/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:345: void NetworkPropertiesFromBssList(const WLAN_BSS_LIST& wlan_bss_list, On 2014/04/21 18:43:43, tbarzic wrote: > suggestions for naming (I understood the current name more in sense 'Create > network properties from bss list'): > MergeNetworkPropertiesFromBssList > UpdateNetworkPropertiesFromBssList > > Also, I'd mention in the method comment that properties should have guid set, > and maybe that the bssid property will not be overwritten if set in > |properties|. Done. https://codereview.chromium.org/245313002/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:667: already_connected = (frequency == connected_properties.frequency) && On 2014/04/21 18:43:43, tbarzic wrote: > nit: no need for () Done. https://codereview.chromium.org/245313002/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:1248: properties->connection_state = onc::connection_state::kConnected; On 2014/04/21 18:43:43, tbarzic wrote: > how does this relate to wlan_connection_attributes->isState from > WlanQueryInterface. > It would be nice for the connection states in getVisibleNwtworks and > getProperties results to be consistent (though, the cast extension _currently_ > does not rely on connection states reported by getVisibleNetworks) I've added TODO. It should also take network connection state into account. https://codereview.chromium.org/245313002/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:1265: DOT11_SSID ssid = SSIDFromGUID(properties->guid); On 2014/04/21 18:43:43, tbarzic wrote: > this can return immediately if ssid is empty > (or maybe add a DCHECK that ssid is valid) Done. https://codereview.chromium.org/245313002/diff/1/components/wifi/wifi_service... components/wifi/wifi_service_win.cc:1382: wlan_connection_attributes->isState == wlan_interface_state_connected) { On 2014/04/21 18:43:43, tbarzic wrote: > can we set connection_state to Connecting for > wlan_interface_state_associating > wlan_interface_state_discovering > wlan_interface_state_authenticating > > Though, this would be useful only if the state changes during Connect call (i.e. > after Connect returns, GetProperties will return 'NotConnected' connection state > only if the connection failed). > > (if it makes sense, I'm fine with doing this separately) Good idea, I think doing it separately will be cleaner. I also added TODO to take network state into account.
https://codereview.chromium.org/245313002/diff/20001/components/wifi/wifi_ser... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/245313002/diff/20001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:899: if (network_guid == connected_network_properties.guid) { && connected_network_properties.connection_state == kConnected? https://codereview.chromium.org/245313002/diff/20001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:908: enable_notify_network_changed_ = true; is enable_notify_network_changed_ still needed? https://codereview.chromium.org/245313002/diff/20001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:1357: if (network_guids.count(network_properties.guid)) { potential future cleanup: you could collect set of available_networks to use prior to creating network properties. that way you wouldn't have to use FindNetwork bellow, and you'd aviod going through bss_list multiple times for duplicated networks
https://codereview.chromium.org/245313002/diff/20001/components/wifi/wifi_ser... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/245313002/diff/20001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:899: if (network_guid == connected_network_properties.guid) { On 2014/04/22 04:59:10, tbarzic wrote: > && connected_network_properties.connection_state == kConnected? Done. https://codereview.chromium.org/245313002/diff/20001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:908: enable_notify_network_changed_ = true; On 2014/04/22 04:59:10, tbarzic wrote: > is enable_notify_network_changed_ still needed? Yes, until |GetConnectedProperties| takes network state into account. We don't want preliminary kConnected notifications. https://codereview.chromium.org/245313002/diff/20001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:1357: if (network_guids.count(network_properties.guid)) { On 2014/04/22 04:59:10, tbarzic wrote: > potential future cleanup: > you could collect set of available_networks to use prior to creating network > properties. that way you wouldn't have to use FindNetwork bellow, and you'd > aviod going through bss_list multiple times for duplicated networks sgtm.
lgtm
The CQ bit was checked by mef@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/245313002/40001
https://codereview.chromium.org/245313002/diff/40001/components/wifi/wifi_ser... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/245313002/diff/40001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:675: network_guid == connected_properties.guid; Don't we need to verify that we are actually connected? https://codereview.chromium.org/245313002/diff/40001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:952: if (error == ERROR_SUCCESS && !connected_network_properties.guid.empty()) { Don't we need to verify the connection state? https://codereview.chromium.org/245313002/diff/40001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:1454: return frequency; return error;
Message was sent while issue was closed.
Change committed as 265640
Message was sent while issue was closed.
Thanks, Antonio! I've addressed your comments in a follow-up CL https://codereview.chromium.org/249683002 https://codereview.chromium.org/245313002/diff/40001/components/wifi/wifi_ser... File components/wifi/wifi_service_win.cc (right): https://codereview.chromium.org/245313002/diff/40001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:675: network_guid == connected_properties.guid; On 2014/04/23 13:54:17, afontan wrote: > Don't we need to verify that we are actually connected? Done. https://codereview.chromium.org/245313002/diff/40001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:952: if (error == ERROR_SUCCESS && !connected_network_properties.guid.empty()) { On 2014/04/23 13:54:17, afontan wrote: > Don't we need to verify the connection state? Done. https://codereview.chromium.org/245313002/diff/40001/components/wifi/wifi_ser... components/wifi/wifi_service_win.cc:1454: return frequency; On 2014/04/23 13:54:17, afontan wrote: > return error; Done. |
