WiFiService::GetProperties now correctly populates BSSID and Frequency of connected network.

components/wifi/wifi_service_win.cc

Index: components/wifi/wifi_service_win.cc
diff --git a/components/wifi/wifi_service_win.cc b/components/wifi/wifi_service_win.cc
index 59970715906f72f5b25f1c8822ab7072cf2d788e..b6067e07ba20f6c29a3bca7e690ec9e5dd8a4e93 100644
--- a/components/wifi/wifi_service_win.cc
+++ b/components/wifi/wifi_service_win.cc
@@ -337,18 +337,20 @@ class WiFiServiceImpl : public WiFiService {
                                   std::string* encryption,
                                   std::string* key_type) const;
 
-  // Populate |properties| based on |wlan| and its corresponding bss info from
-  // |wlan_bss_list|.
+  // Populate |properties| based on |wlan|.
   void NetworkPropertiesFromAvailableNetwork(const WLAN_AVAILABLE_NETWORK& wlan,
-                                             const WLAN_BSS_LIST& wlan_bss_list,
                                              NetworkProperties* properties);
 
+  // Populate |properties| based on bss info from |wlan_bss_list|.
+  void NetworkPropertiesFromBssList(const WLAN_BSS_LIST& wlan_bss_list,

[tbarzic, 2014/04/21 18:43:43]

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|.

[mef, 2014/04/21 22:23:19]

Done.

+                                    NetworkProperties* properties);
+
   // Get the list of visible wireless networks.
   DWORD GetVisibleNetworkList(NetworkList* network_list);
 
-  // Find currently connected network if any. Populate |connected_network_guid|
+  // Find currently connected network if any. Populate |connected_properties|
   // on success.
-  DWORD FindConnectedNetwork(std::string* connected_network_guid);
+  DWORD GetConnectedProperties(NetworkProperties* connected_properties);
 
   // Connect to network |network_guid| using previosly stored profile if exists,
   // or just network sid. If |frequency| is not |kFrequencyUnknown| then
@@ -359,10 +361,6 @@ class WiFiServiceImpl : public WiFiService {
   // Disconnect from currently connected network if any.
   DWORD Disconnect();
 
-  // Get Frequency of currently connected network |network_guid|. If network is
-  // not connected, then return |kFrequencyUnknown|.
-  Frequency GetConnectedFrequency(const std::string& network_guid);
-
   // Get desired connection freqency if it was set using |SetProperties|.
   // Default to |kFrequencyAny|.
   Frequency GetFrequencyToConnect(const std::string& network_guid) const;
@@ -506,20 +504,28 @@ void WiFiServiceImpl::GetProperties(const std::string& network_guid,
                                     base::DictionaryValue* properties,
                                     std::string* error) {
   DWORD error_code = EnsureInitialized();
+  if (CheckError(error_code, kWiFiServiceError, error))
+    return;
+
+  NetworkProperties connected_properties;
+  error_code = GetConnectedProperties(&connected_properties);
+  if (error_code == ERROR_SUCCESS &&
+      connected_properties.guid == network_guid) {
+    properties->Swap(connected_properties.ToValue(false).get());
+    return;
+  }
+
+  NetworkList network_list;
+  error_code = GetVisibleNetworkList(&network_list);
   if (error_code == ERROR_SUCCESS) {
-    NetworkList network_list;
-    error_code = GetVisibleNetworkList(&network_list);
-    if (error_code == ERROR_SUCCESS && !network_list.empty()) {
-      NetworkList::const_iterator it = FindNetwork(network_list, network_guid);
-      if (it != network_list.end()) {
-        DVLOG(1) << "Get Properties: " << network_guid << ":"
-                   << it->connection_state;
-        properties->Swap(it->ToValue(false).get());
-        return;
-      } else {
-        error_code = ERROR_NOT_FOUND;
-      }
+    NetworkList::const_iterator it = FindNetwork(network_list, network_guid);
+    if (it != network_list.end()) {
+      DVLOG(1) << "Get Properties: " << network_guid << ":"
+                  << it->connection_state;
+      properties->Swap(it->ToValue(false).get());
+      return;
     }
+    error_code = ERROR_NOT_FOUND;
   }
 
   CheckError(error_code, kWiFiServiceError, error);
@@ -656,8 +662,11 @@ void WiFiServiceImpl::StartConnect(const std::string& network_guid,
       bool already_connected = (network_guid == connected_network_guid);
       Frequency frequency = GetFrequencyToConnect(network_guid);
       if (already_connected && frequency != kFrequencyAny) {
-        Frequency connected_frequency = GetConnectedFrequency(network_guid);
-        already_connected = (frequency == connected_frequency);
+        NetworkProperties connected_properties;
+        if (GetConnectedProperties(&connected_properties) == ERROR_SUCCESS) {
+          already_connected = (frequency == connected_properties.frequency) &&

[tbarzic, 2014/04/21 18:43:43]

nit: no need for () 

[mef, 2014/04/21 22:23:19]

Done.

+                              (network_guid == connected_properties.guid);
+        }
       }
       // Connect only if network |network_guid| is not connected already.
       if (!already_connected)
@@ -878,9 +887,9 @@ void WiFiServiceImpl::WaitForNetworkConnect(const std::string& network_guid,
     RestoreNwCategoryWizard();
     return;
   }
-  std::string connected_network_guid;
-  DWORD error = FindConnectedNetwork(&connected_network_guid);
-  if (network_guid == connected_network_guid) {
+  NetworkProperties connected_network_properties;
+  DWORD error = GetConnectedProperties(&connected_network_properties);
+  if (network_guid == connected_network_properties.guid) {
     DVLOG(1) << "WiFi Connected, Reset DHCP: " << network_guid;
     // Even though wireless network is now connected, it may still be unusable,
     // e.g. after Chromecast device reset. Reset DHCP on wireless network to
@@ -929,15 +938,15 @@ WiFiService::NetworkList::iterator WiFiServiceImpl::FindNetwork(
 DWORD WiFiServiceImpl::SaveCurrentConnectedNetwork(
     std::string* connected_network_guid) {
   // Find currently connected network.
-  DWORD error = FindConnectedNetwork(connected_network_guid);
-  if (error == ERROR_SUCCESS && !connected_network_guid->empty()) {
+  NetworkProperties connected_network_properties;
+  DWORD error = GetConnectedProperties(&connected_network_properties);
+  if (error == ERROR_SUCCESS && !connected_network_properties.guid.empty()) {
+    *connected_network_guid = connected_network_properties.guid;
+    SaveTempProfile(*connected_network_guid);
+    std::string profile_xml;
+    error = GetProfile(*connected_network_guid, false, &profile_xml);
     if (error == ERROR_SUCCESS) {
-      SaveTempProfile(*connected_network_guid);
-      std::string profile_xml;
-      error = GetProfile(*connected_network_guid, false, &profile_xml);
-      if (error == ERROR_SUCCESS) {
-        saved_profiles_xml_[*connected_network_guid] = profile_xml;
-      }
+      saved_profiles_xml_[*connected_network_guid] = profile_xml;
     }
   }
   return error;
@@ -1234,7 +1243,6 @@ std::string WiFiServiceImpl::SecurityFromDot11AuthAlg(
 
 void WiFiServiceImpl::NetworkPropertiesFromAvailableNetwork(
     const WLAN_AVAILABLE_NETWORK& wlan,
-    const WLAN_BSS_LIST& wlan_bss_list,
     NetworkProperties* properties) {
   if (wlan.dwFlags & WLAN_AVAILABLE_NETWORK_CONNECTED) {
     properties->connection_state = onc::connection_state::kConnected;

[tbarzic, 2014/04/21 18:43:43]

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)

[mef, 2014/04/21 22:23:19]

I've added TODO. It should also take network connection state into account.

@@ -1246,23 +1254,32 @@ void WiFiServiceImpl::NetworkPropertiesFromAvailableNetwork(
   properties->name = properties->ssid;
   properties->guid = GUIDFromWLAN(wlan);
   properties->type = onc::network_type::kWiFi;
+  properties->security =
+      SecurityFromDot11AuthAlg(wlan.dot11DefaultAuthAlgorithm);
+  properties->signal_strength = wlan.wlanSignalQuality;
+}
 
+void WiFiServiceImpl::NetworkPropertiesFromBssList(
+    const WLAN_BSS_LIST& wlan_bss_list,
+    NetworkProperties* properties) {
+  DOT11_SSID ssid = SSIDFromGUID(properties->guid);

[tbarzic, 2014/04/21 18:43:43]

this can return immediately  if ssid is empty
(or maybe add a DCHECK that ssid is valid)

[mef, 2014/04/21 22:23:19]

Done.

   for (size_t bss = 0; bss < wlan_bss_list.dwNumberOfItems; ++bss) {
     const WLAN_BSS_ENTRY& bss_entry(wlan_bss_list.wlanBssEntries[bss]);
-    if (bss_entry.dot11Ssid.uSSIDLength == wlan.dot11Ssid.uSSIDLength &&
+    if (bss_entry.dot11Ssid.uSSIDLength == ssid.uSSIDLength &&
         0 == memcmp(bss_entry.dot11Ssid.ucSSID,
-                    wlan.dot11Ssid.ucSSID,
+                    ssid.ucSSID,
                     bss_entry.dot11Ssid.uSSIDLength)) {
-      properties->frequency = GetNormalizedFrequency(
-          bss_entry.ulChCenterFrequency / 1000);
-      properties->frequency_set.insert(properties->frequency);
-      properties->bssid = NetworkProperties::MacAddressAsString(
+      std::string bssid = NetworkProperties::MacAddressAsString(
           bss_entry.dot11Bssid);
+      Frequency frequency = GetNormalizedFrequency(
+          bss_entry.ulChCenterFrequency / 1000);
+      properties->frequency_set.insert(frequency);
+      if (properties->bssid.empty() || properties->bssid == bssid) {
+        properties->frequency = frequency;
+        properties->bssid = bssid;
+      }
     }
   }
-  properties->security =
-      SecurityFromDot11AuthAlg(wlan.dot11DefaultAuthAlgorithm);
-  properties->signal_strength = wlan.wlanSignalQuality;
 }
 
 // Get the list of visible wireless networks
@@ -1303,8 +1320,8 @@ DWORD WiFiServiceImpl::GetVisibleNetworkList(NetworkList* network_list) {
         NetworkProperties network_properties;
         NetworkPropertiesFromAvailableNetwork(
             available_network_list->Network[i],
-            *bss_list,
             &network_properties);
+        NetworkPropertiesFromBssList(*bss_list, &network_properties);
         // Check for duplicate network guids.
         if (network_guids.count(network_properties.guid)) {
           // There should be no difference between properties except for
@@ -1335,49 +1352,17 @@ DWORD WiFiServiceImpl::GetVisibleNetworkList(NetworkList* network_list) {
   return error;
 }
 
-// Find currently connected network.
-DWORD WiFiServiceImpl::FindConnectedNetwork(
-    std::string* connected_network_guid) {
+DWORD WiFiServiceImpl::GetConnectedProperties(NetworkProperties* properties) {
   if (client_ == NULL) {
     NOTREACHED();
     return ERROR_NOINTERFACE;
   }
 
-  DWORD error = ERROR_SUCCESS;
-  PWLAN_AVAILABLE_NETWORK_LIST available_network_list = NULL;
-  error = WlanGetAvailableNetworkList_function_(
-      client_, &interface_guid_, 0, NULL, &available_network_list);
-
-  if (error == ERROR_SUCCESS && NULL != available_network_list) {
-    for (DWORD i = 0; i < available_network_list->dwNumberOfItems; ++i) {
-      const WLAN_AVAILABLE_NETWORK& wlan = available_network_list->Network[i];
-      if (wlan.dwFlags & WLAN_AVAILABLE_NETWORK_CONNECTED) {
-        *connected_network_guid = GUIDFromWLAN(wlan);
-        break;
-      }
-    }
-  }
-
-  // Clean up.
-  if (available_network_list != NULL) {
-    WlanFreeMemory_function_(available_network_list);
-  }
-
-  return error;
-}
-
-WiFiService::Frequency WiFiServiceImpl::GetConnectedFrequency(
-    const std::string& network_guid) {
-  if (client_ == NULL) {
-    NOTREACHED();
-    return kFrequencyUnknown;
-  }
-
   // TODO(mef): WlanGetNetworkBssList is not available on XP. If XP support is
   // needed, then different method of getting BSS (e.g. OID query) will have
   // to be used.
   if (WlanGetNetworkBssList_function_ == NULL)
-    return kFrequencyUnknown;
+    return ERROR_NOINTERFACE;
 
   Frequency frequency = kFrequencyUnknown;
   DWORD error = ERROR_SUCCESS;
@@ -1397,40 +1382,39 @@ WiFiService::Frequency WiFiServiceImpl::GetConnectedFrequency(
       wlan_connection_attributes->isState == wlan_interface_state_connected) {

[tbarzic, 2014/04/21 18:43:43]

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)

[mef, 2014/04/21 22:23:19]

Good idea, I think doing it separately will be cleaner. I also added TODO to
take network state into account.

     WLAN_ASSOCIATION_ATTRIBUTES& connected_wlan =
         wlan_connection_attributes->wlanAssociationAttributes;
-    // Try to find connected frequency based on bss.
-    if (GUIDFromSSID(connected_wlan.dot11Ssid) == network_guid &&
-        WlanGetNetworkBssList_function_ != NULL) {
-      error = WlanGetNetworkBssList_function_(client_,
-                                              &interface_guid_,
-                                              &connected_wlan.dot11Ssid,
-                                              connected_wlan.dot11BssType,
-                                              FALSE,
-                                              NULL,
-                                              &bss_list);
-      if (error == ERROR_SUCCESS && NULL != bss_list) {
-        // Go through bss_list and find matching BSSID.
-        for (size_t bss = 0; bss < bss_list->dwNumberOfItems; ++bss) {
-          const WLAN_BSS_ENTRY& bss_entry(bss_list->wlanBssEntries[bss]);
-          if (0 == memcmp(bss_entry.dot11Bssid,
-                          connected_wlan.dot11Bssid,
-                          sizeof(bss_entry.dot11Bssid))) {
-            frequency = GetNormalizedFrequency(
-                bss_entry.ulChCenterFrequency / 1000);
-            break;
-          }
-        }
-      }
+
+    properties->connection_state = onc::connection_state::kConnected;
+    properties->ssid = GUIDFromSSID(connected_wlan.dot11Ssid);
+    properties->name = properties->ssid;
+    properties->guid = GUIDFromSSID(connected_wlan.dot11Ssid);
+    properties->type = onc::network_type::kWiFi;
+    properties->bssid = NetworkProperties::MacAddressAsString(
+        connected_wlan.dot11Bssid);
+    properties->security = SecurityFromDot11AuthAlg(
+        wlan_connection_attributes->wlanSecurityAttributes.dot11AuthAlgorithm);
+    properties->signal_strength = connected_wlan.wlanSignalQuality;
+
+    // TODO(mef): WlanGetNetworkBssList is not available on XP. If XP support is
+    // needed, then different method of getting BSS (e.g. OID query) will have
+    // to be used.
+    error = WlanGetNetworkBssList_function_(client_,
+                                            &interface_guid_,
+                                            &connected_wlan.dot11Ssid,
+                                            connected_wlan.dot11BssType,
+                                            FALSE,
+                                            NULL,
+                                            &bss_list);
+    if (error == ERROR_SUCCESS && NULL != bss_list) {
+      NetworkPropertiesFromBssList(*bss_list, properties);
     }
   }
 
   // Clean up.
-  if (wlan_connection_attributes != NULL) {
+  if (wlan_connection_attributes != NULL)
     WlanFreeMemory_function_(wlan_connection_attributes);
-  }
 
-  if (bss_list != NULL) {
+  if (bss_list != NULL)
     WlanFreeMemory_function_(bss_list);
-  }
 
   return frequency;
 }