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

components/wifi/wifi_service_win.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/wifi/wifi_service.h"
 
 #include <iphlpapi.h>
 #include <objbase.h>
 #include <wlanapi.h>
 
@@ skipping 319 matching lines @@
   std::string WpaEncryptionFromEncryptionType(
       EncryptionType encryption_type) const;
 
   // Deduce WLANProfile |authEncryption| values from |onc::wifi| security.
   bool AuthEncryptionFromSecurity(const std::string& security,
                                   EncryptionType encryption_type,
                                   std::string* authentication,
                                   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
   // connects only to BSS which uses that frequency and returns
   // |ERROR_NOT_FOUND| if such BSS cannot be found.
   DWORD Connect(const std::string& network_guid, Frequency frequency);
 
   // 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;
 
   // Get DOT11_BSSID_LIST of desired BSSIDs to connect to |ssid| network on
   // given |frequency|.
   DWORD GetDesiredBssList(DOT11_SSID& ssid,
                           Frequency frequency,
                           scoped_ptr<DOT11_BSSID_LIST>* desired_list);
 
@@ skipping 123 matching lines @@
 }
 
 void WiFiServiceImpl::UnInitialize() {
   CloseClientHandle();
 }
 
 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);
 }
 
 void WiFiServiceImpl::GetManagedProperties(
     const std::string& network_guid,
     base::DictionaryValue* managed_properties,
     std::string* error) {
   CheckError(ERROR_CALL_NOT_IMPLEMENTED, kWiFiServiceError, error);
@@ skipping 116 matching lines @@
   DVLOG(1) << "Start Connect: " << network_guid;
   DWORD error_code = EnsureInitialized();
   if (error_code == ERROR_SUCCESS) {
     std::string connected_network_guid;
     error_code = SaveCurrentConnectedNetwork(&connected_network_guid);
     if (error_code == ERROR_SUCCESS) {
       // Check, if the network is already connected on desired frequency.
       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)
         error_code = Connect(network_guid, frequency);
       if (error_code == ERROR_SUCCESS) {
         // Notify that previously connected network has changed.
         NotifyNetworkChanged(connected_network_guid);
         // Start waiting for network connection state change.
         if (!networks_changed_observer_.is_null()) {
           DisableNwCategoryWizard();
@@ skipping 200 matching lines @@
           LOG(ERROR) << "Failed to delete created profile for " << network_guid
                      << " error=" << error_code;
         }
       }
     }
     // Restore automatic network change notifications and stop waiting.
     enable_notify_network_changed_ = true;
     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
     // work around this issue.
     error = ResetDHCP();
     // There is no need to keep created profile as network is connected.
     created_profiles_.RemoveWithoutPathExpansion(network_guid, NULL);
     // Restore previously suppressed notifications.
     enable_notify_network_changed_ = true;
     RestoreNwCategoryWizard();
@@ skipping 28 matching lines @@
        ++it) {
     if (it->guid == network_guid)
       return it;
   }
   return networks.end();
 }
 
 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;
 }
 
 void WiFiServiceImpl::SortNetworks(NetworkList* networks) {
   networks->sort(NetworkProperties::OrderByType);
 }
 
 DWORD WiFiServiceImpl::LoadWlanLibrary() {
@@ skipping 276 matching lines @@
       return onc::wifi::kWEP_PSK;
     case DOT11_AUTH_ALGO_80211_OPEN:
       return onc::wifi::kNone;
     default:
       return onc::wifi::kWPA_EAP;
   }
 }
 
 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.

   } else {
     properties->connection_state = onc::connection_state::kNotConnected;
   }
 
   properties->ssid = SSIDFromWLAN(wlan);
   properties->name = properties->ssid;
   properties->guid = GUIDFromWLAN(wlan);
   properties->type = onc::network_type::kWiFi;
-
-  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 &&
-        0 == memcmp(bss_entry.dot11Ssid.ucSSID,
-                    wlan.dot11Ssid.ucSSID,
-                    bss_entry.dot11Ssid.uSSIDLength)) {
-      properties->frequency = GetNormalizedFrequency(
-          bss_entry.ulChCenterFrequency / 1000);
-      properties->frequency_set.insert(properties->frequency);
-      properties->bssid = NetworkProperties::MacAddressAsString(
-          bss_entry.dot11Bssid);
-    }
-  }
   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 == ssid.uSSIDLength &&
+        0 == memcmp(bss_entry.dot11Ssid.ucSSID,
+                    ssid.ucSSID,
+                    bss_entry.dot11Ssid.uSSIDLength)) {
+      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;
+      }
+    }
+  }
+}
+
 // Get the list of visible wireless networks
 DWORD WiFiServiceImpl::GetVisibleNetworkList(NetworkList* network_list) {
   if (client_ == NULL) {
     NOTREACHED();
     return ERROR_NOINTERFACE;
   }
 
   DWORD error = ERROR_SUCCESS;
   PWLAN_AVAILABLE_NETWORK_LIST available_network_list = NULL;
   PWLAN_BSS_LIST bss_list = NULL;
@@ skipping 18 matching lines @@
                                             NULL,
                                             dot11_BSS_type_any,
                                             FALSE,
                                             NULL,
                                             &bss_list);
     if (error == ERROR_SUCCESS && NULL != bss_list) {
       for (DWORD i = 0; i < available_network_list->dwNumberOfItems; ++i) {
         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
           // |connection_state|, so mark it as |kConnected| if either one is.
           if (network_properties.connection_state ==
               onc::connection_state::kConnected) {
             NetworkList::iterator previous_network_properties =
                 FindNetwork(*network_list, network_properties.guid);
             DCHECK(previous_network_properties != network_list->end());
             previous_network_properties->connection_state =
@@ skipping 10 matching lines @@
   // Clean up.
   if (available_network_list != NULL) {
     WlanFreeMemory_function_(available_network_list);
   }
   if (bss_list != NULL) {
     WlanFreeMemory_function_(bss_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;
   DWORD data_size = 0;
   PWLAN_CONNECTION_ATTRIBUTES wlan_connection_attributes = NULL;
   PWLAN_BSS_LIST bss_list = NULL;
   error = WlanQueryInterface_function_(
       client_,
       &interface_guid_,
       wlan_intf_opcode_current_connection,
       NULL,
       &data_size,
       reinterpret_cast<PVOID*>(&wlan_connection_attributes),
       NULL);
   if (error == ERROR_SUCCESS &&
       wlan_connection_attributes != NULL &&
       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;
 }
 
 WiFiService::Frequency WiFiServiceImpl::GetFrequencyToConnect(
     const std::string& network_guid) const {
   // Check whether desired frequency is set in |connect_properties_|.
   const base::DictionaryValue* properties;
   const base::DictionaryValue* wifi;
   int frequency;
@@ skipping 355 matching lines @@
     NetworkGuidList changed_networks(1, network_guid);
     message_loop_proxy_->PostTask(
         FROM_HERE,
         base::Bind(networks_changed_observer_, changed_networks));
   }
 }
 
 WiFiService* WiFiService::Create() { return new WiFiServiceImpl(); }
 
 }  // namespace wifi