Mac OS X-specific implementation of Networking Private API.

components/wifi/wifi_service_mac.mm

+// 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 "base/bind.h"
+#include "base/mac/scoped_nsautorelease_pool.h"
+#include "base/mac/scoped_nsobject.h"
+#include "base/message_loop/message_loop.h"
+#include "components/onc/onc_constants.h"
+
+#import <netinet/in.h>

[Robert Sesek, 2014/01/07 16:19:05]

These headers come below the .h for the .mm and before other project includes.

[mef, 2014/01/08 21:00:03]

Done.

+#import <CoreWLAN/CoreWLAN.h>
+#import <SystemConfiguration/SystemConfiguration.h>
+
+namespace {
+// Declare notification names from the 10.7 SDK.
+const char* kCWSSIDDidChangeNotification_chrome =

[Robert Sesek, 2014/01/07 16:19:05]

kCWSSIDDidChangeNotification is marked as available since 10.6?

[mef, 2014/01/08 21:00:03]

Is it ok to link to CoreWLAN.framework, or should it be loaded dynamically via
dlopen?

[Robert Sesek, 2014/01/09 23:12:45]

I think it's fine to link it directly in the link_settings.

[mef, 2014/01/10 17:01:52]

Done.

+    "com.apple.coreWLAN.notification.ssid";
+}  // namespace
+
+#if !defined(MAC_OS_X_VERSION_10_7) || \
+    MAC_OS_X_VERSION_MAX_ALLOWED < MAC_OS_X_VERSION_10_7
+
+// Local definitions of API added in Mac OS X 10.7
+
+@interface CWInterface (LionAPI)
+- (BOOL)associateToNetwork:(CWNetwork *)network

[Robert Sesek, 2014/01/07 16:19:05]

nit: here and throughout, there's no space between typename and the *; just
|CFNetwork*|

[mef, 2014/01/08 21:00:03]

Done.

+                  password:(NSString *)password
+                     error:(NSError **)error;
+- (NSSet*)scanForNetworksWithName:(NSString *)networkName
+                            error:(NSError **)error;
+@end
+
+enum CWChannelBand {
+   kCWChannelBandUnknown = 0,

[Robert Sesek, 2014/01/07 16:19:05]

nit: only indent 2 spaces

[mef, 2014/01/08 21:00:03]

Done.

+   kCWChannelBand2GHz = 1,
+   kCWChannelBand5GHz = 2,
+};
+
+@interface CWChannel : NSObject
+@property(readonly) CWChannelBand channelBand;
+@end
+
+@interface CWNetwork (LionAPI)
+@property(readonly) CWChannel *wlanChannel;
+@end
+
+#endif  // 10.7
+
+// Observe CoreWLAN notifications and call |WiFiServiceImpl| on worker thread.

[tbarzic, 2014/01/07 20:05:33]

This seems to observe only "connected network ssid" changes, right?
Can you add a comment about this.

[mef, 2014/01/08 21:00:03]

Done.

+@interface WLANNotificationObserver : NSObject {
+  // Task runner for worker tasks.
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  // Notification handler.
+  base::Closure notification_handler_;
+}
+@end
+
+@implementation WLANNotificationObserver
+
+// Upon receiving |notification| post |notification_handler_| to |task_runner_|.
+- (void)handleInterfaceNotification:(NSNotification*)notification {
+  DCHECK(task_runner_);
+  DCHECK(!notification_handler_.is_null());
+  task_runner_->PostTask(FROM_HERE, notification_handler_);
+}
+
+// Start listening for WLAN notifications.
+- (void)startListen:(base::Closure) notification_handler
+    usingTaskRunner:(scoped_refptr<base::SequencedTaskRunner>) task_runner {
+  notification_handler_ = notification_handler;
+  task_runner_ = task_runner;
+  [[NSNotificationCenter defaultCenter]

[Robert Sesek, 2014/01/07 16:19:05]

You can remove this entire ObjC class if you use the block-based
NSNotificationCenter API.

[mef, 2014/01/08 21:00:03]

Done.

+      addObserver:self
+      selector:@selector(handleInterfaceNotification:)
+      name:[NSString stringWithUTF8String:kCWSSIDDidChangeNotification_chrome]
+      object:nil];
+}
+
+// Stop listening for WLAN notifications.
+- (void)stopListen
+{
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
+}
+
+@end
+
+namespace wifi {
+
+// Implementation of WiFiService for Mac OS X.
+class WiFiServiceImpl : public WiFiService {

[Robert Sesek, 2014/01/07 16:19:05]

Why not call this WiFiServiceMac ?

[mef, 2014/01/08 21:00:03]

Done.

+ public:
+  WiFiServiceImpl();
+  virtual ~WiFiServiceImpl();
+
+  // WiFiService interface implementation.
+  virtual void Initialize(
+      scoped_refptr<base::SequencedTaskRunner> task_runner) OVERRIDE;
+
+  virtual void UnInitialize() OVERRIDE;
+
+  virtual void GetProperties(const std::string& network_guid,
+                             base::DictionaryValue* properties,
+                             std::string* error) OVERRIDE;
+
+  virtual void GetManagedProperties(const std::string& network_guid,
+                                    base::DictionaryValue* managed_properties,
+                                    std::string* error) OVERRIDE;
+
+  virtual void GetState(const std::string& network_guid,
+                        base::DictionaryValue* properties,
+                        std::string* error) OVERRIDE;
+
+  virtual void SetProperties(const std::string& network_guid,
+                             scoped_ptr<base::DictionaryValue> properties,
+                             std::string* error) OVERRIDE;
+
+  virtual void CreateNetwork(bool shared,
+                             scoped_ptr<base::DictionaryValue> properties,
+                             std::string* network_guid,
+                             std::string* error) OVERRIDE;
+
+  virtual void GetVisibleNetworks(const std::string& network_type,
+                                  base::ListValue* network_list) OVERRIDE;
+
+  virtual void RequestNetworkScan() OVERRIDE;
+
+  virtual void StartConnect(const std::string& network_guid,
+                            std::string* error) OVERRIDE;
+
+  virtual void StartDisconnect(const std::string& network_guid,
+                               std::string* error) OVERRIDE;
+
+  virtual void SetEventObservers(
+      scoped_refptr<base::MessageLoopProxy> message_loop_proxy,
+      const NetworkGuidListCallback& networks_changed_observer,
+      const NetworkGuidListCallback& network_list_changed_observer) OVERRIDE;
+
+ private:
+  // Check |nsError| and if is not |nil|, then store |error_name|
+  // into |error|.
+  bool CheckError(NSError* nsError,

[Robert Sesek, 2014/01/07 16:19:05]

naming: In C++, variables do not use camelCase.

[mef, 2014/01/08 21:00:03]

Done.

+                  const char* error_name,
+                  std::string* error) const;
+
+  // Get |ssid| from unique |network_guid|.
+  NSString* SSIDFromGUID(const std::string& network_guid) const {

[Robert Sesek, 2014/01/07 16:19:05]

I think you can remove this and the method below entirely if you use
base/strings/sys_string_conversions.h.

[mef, 2014/01/08 21:00:03]

I'd like to have explicit SSID / GUID convertors in case if we decide that GUID
shouldn't be equal to SSID.

+    return [NSString stringWithUTF8String:network_guid.c_str()];
+  }
+
+  // Get unique |network_guid| string based on |ssid|.
+  std::string GUIDFromSSID(NSString* ssid) const {
+    if (ssid == nil)
+      return std::string();
+    return std::string([ssid UTF8String]);
+  }
+
+  // Populate |properties| from |network|.
+  void NetworkPropertiesFromCWNetwork(const CWNetwork* network,
+                                      NetworkProperties* properties) const;
+
+  // Convert |CWSecurityMode| into onc::wifi::k{WPA|WEP}* security constant.
+  std::string SecurityFromCWSecurityMode(CWSecurityMode security) const;
+
+  // Wait up to |kMaxAttempts| with |kAttemptDelayMs| delay for connection
+  // to network with |network_guid|. Notify that |NetworkChanged| upon success.
+  void WaitForNetworkConnect(const std::string& network_guid, int attempt);
+
+  // Get the list of visible wireless networks. If |network_guid| is not empty,
+  // then only return that network.
+  NSError* GetVisibleNetworkList(const std::string& network_guid,
+                                 NetworkList* network_list);
+
+  // Sort networks, so connected/connecting is up front, then by type:
+  // Ethernet, WiFi, Cellular, VPN

[tbarzic, 2014/01/07 20:05:33]

missing . at the end of the comment

[mef, 2014/01/08 21:00:03]

Done.

+  static void SortNetworks(NetworkList* networks);
+
+  // Handle notification from |WLANListener|;

[tbarzic, 2014/01/07 20:05:33]

Hm, I can't see anything named |WLANListener| here. Did you mean
|wlan_observer_|

[mef, 2014/01/08 21:00:03]

Done.

+  void OnListenerNotification();
+
+  // Notify |network_list_changed_observer_| that list of visible networks has
+  // changed to |networks|.
+  void NotifyNetworkListChanged(const NetworkList& networks);
+
+  // Notify |networks_changed_observer_| that network |network_guid| status has
+  // changed.
+  void NotifyNetworkChanged(const std::string& network_guid);
+
+  // CoreWLAN.Framework bundle.
+  base::scoped_nsobject<NSBundle> bundle_;
+  // Default interface.
+  base::scoped_nsobject<CWInterface> interface_;
+  // WLAN Notifications observer.
+  base::scoped_nsobject<WLANNotificationObserver> wlan_observer_;
+
+  // Observer to get notified when network(s) have changed (e.g. connect).
+  NetworkGuidListCallback networks_changed_observer_;
+  // Observer to get notified when network list has changed (scan complete).
+  NetworkGuidListCallback network_list_changed_observer_;
+  // MessageLoopProxy to post events on UI thread.
+  scoped_refptr<base::MessageLoopProxy> message_loop_proxy_;
+  // Task runner for worker tasks.
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  // Cache of network list collected by GetVisibleNetworks.
+  NetworkList networks_;
+  // Temporary storage of network properties indexed by |network_guid|.
+  base::DictionaryValue network_properties_;
+  // If |false|, then |networks_changed_observer_| is not notified.
+  bool enable_notify_network_changed_;
+  // Number of attempts to check that network has connected successfully.
+  static const int kMaxAttempts = 200;

[tbarzic, 2014/01/07 20:05:33]

the name could be more descriptive (e.g. kMaxConnectionAttempts)

[mef, 2014/01/08 21:00:03]

Done.

+  // Delay between attempts to check that network has connected successfully.
+  static const int kAttemptDelayMs = 100;
+};
+
+WiFiServiceImpl::WiFiServiceImpl() : enable_notify_network_changed_(true) {
+}
+
+WiFiServiceImpl::~WiFiServiceImpl() {
+}
+
+void WiFiServiceImpl::Initialize(
+  scoped_refptr<base::SequencedTaskRunner> task_runner) {
+  // As the WLAN api binding runs on its own thread, we need to provide our own

[Robert Sesek, 2014/01/07 16:19:05]

I don't understand this. What's backing this SequencedTaskRunner and why isn't
it providing the autorelease pool automatically (for reference,
SequencedWorkerPool does)?

[mef, 2014/01/08 21:00:03]

I'm not sure I understand the question, could you elaborate? I've copied this
comment from
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ge...

[Robert Sesek, 2014/01/09 23:12:45]

I don't think you need to supply your own autorelease pool at all in this CL,
since the thread running the MessageLoop should be doing that for you. Why did
you add this in the first place? Were you seeing console errors being printed
around objects being leaked because no pool was in place? Unless you're running
your own base::Thread, there should be an autorelease pool around the main event
loop automatically.

[mef, 2014/01/10 17:01:52]

FYI: WiFiService calls are posted from UI thread to sequenced task runner on
worker pool, so they run on random worker threads. I didn't observe any errors,
but adding local autorelease pool seemed to make sense. I'll be happy to remove
it though if its not necessary. Please advise. 

[Robert Sesek, 2014/01/10 18:59:51]

As I wrote above, it is not necessary. Worker threads create the autorelease
pool around the main MessageLoop.

https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq...

[mef, 2014/01/10 20:26:16]

Done.

+  // auto release pool. It's simplest to do this as an automatic variable in
+  // each method that needs it, to ensure the scoping is correct and does not
+  // interfere with any other code using autorelease pools on the thread.
+  base::mac::ScopedNSAutoreleasePool auto_pool;
+
+  task_runner_.swap(task_runner);
+
+  bundle_.reset([[NSBundle alloc]
+      initWithPath:@"/System/Library/Frameworks/CoreWLAN.framework"]);

[Robert Sesek, 2014/01/07 16:19:05]

Why do you dynamically load CoreWLAN?

[mef, 2014/01/08 21:00:03]

To mimic geolocation code, but I guess we can actually link it directly as we
don't support 10.5 any more, right?

[Robert Sesek, 2014/01/09 23:12:45]

Yes. I think so.

[mef, 2014/01/10 17:01:52]

Done.

+  if (!bundle_) {
+    DVLOG(1) << "Failed to load the CoreWLAN framework bundle";
+    return;
+  }
+
+  Class cw_interface_class = [bundle_ classNamed:@"CWInterface"];
+  interface_.reset([[cw_interface_class interface] retain]);
+  if (!bundle_) {
+    DVLOG(1) << "Failed to initialize default interface";
+    return;
+  }
+
+  WLANNotificationObserver* observer = [[WLANNotificationObserver alloc] init];

[Robert Sesek, 2014/01/07 16:19:05]

Put this in the scoper to start with, rather than .reset()ing below.

[mef, 2014/01/08 21:00:03]

Done.

+  [observer startListen:base::Bind(&WiFiServiceImpl::OnListenerNotification,
+                                   base::Unretained(this))
+        usingTaskRunner:task_runner_];
+  wlan_observer_.reset(observer);
+}
+
+void WiFiServiceImpl::UnInitialize() {
+  [wlan_observer_ stopListen];
+}
+
+void WiFiServiceImpl::GetProperties(const std::string& network_guid,
+                                    base::DictionaryValue* properties,
+                                    std::string* error) {
+  base::mac::ScopedNSAutoreleasePool auto_pool;
+
+  if (networks_.empty()) {
+    DVLOG(1) << "GetProperties";
+    NSError* nsError = GetVisibleNetworkList(std::string(), &networks_);

[Robert Sesek, 2014/01/07 16:19:05]

naming: nsError

[mef, 2014/01/08 21:00:03]

Done.

+    if (CheckError(nsError, "Error.GetProperties", error))
+      return;
+  }
+
+  for (WiFiService::NetworkList::iterator it = networks_.begin();
+       it != networks_.end();
+       ++it) {
+    if (it->guid == network_guid) {
+      bool is_connected = network_guid == GUIDFromSSID([interface_ ssid]);
+      it->connection_state =
+          is_connected ? onc::connection_state::kConnected :
+                         onc::connection_state::kNotConnected;
+      scoped_ptr<base::DictionaryValue> network(it->ToValue(false));
+      properties->Swap(network.get());
+      DVLOG(1) << *properties;
+      return;
+    }
+  }
+
+  *error = "Error.NotFound";
+}
+
+void WiFiServiceImpl::GetManagedProperties(
+    const std::string& network_guid,
+    base::DictionaryValue* managed_properties,
+    std::string* error) {
+  *error = "Error.NotImplemented";

[Robert Sesek, 2014/01/07 16:19:05]

Are these going to be implemented?

[mef, 2014/01/08 21:00:03]

At some point, not in current release.

+}
+
+void WiFiServiceImpl::GetState(const std::string& network_guid,
+                               base::DictionaryValue* properties,
+                               std::string* error) {
+  *error = "Error.NotImplemented";
+}
+
+void WiFiServiceImpl::SetProperties(
+    const std::string& network_guid,
+    scoped_ptr<base::DictionaryValue> properties,
+    std::string* error) {
+  base::mac::ScopedNSAutoreleasePool auto_pool;
+  network_properties_.SetWithoutPathExpansion(network_guid,
+                                              properties.release());
+}
+
+void WiFiServiceImpl::CreateNetwork(
+    bool shared,
+    scoped_ptr<base::DictionaryValue> properties,
+    std::string* network_guid,
+    std::string* error) {
+  *error = "Error.NotImplemented";
+}
+
+void WiFiServiceImpl::GetVisibleNetworks(const std::string& network_type,
+                                         base::ListValue* network_list) {
+  if (!network_type.empty() &&
+      network_type != onc::network_type::kAllTypes &&
+      network_type != onc::network_type::kWiFi) {
+    return;
+  }
+
+  DVLOG(1) << __FUNCTION__;

[Robert Sesek, 2014/01/07 16:19:05]

Remove unnecessary logging; here and throughout.

[mef, 2014/01/08 21:00:03]

Why?

[Robert Sesek, 2014/01/09 23:12:45]

It needlessly clutters the code.

+
+  base::mac::ScopedNSAutoreleasePool auto_pool;
+
+  GetVisibleNetworkList(std::string(), &networks_);
+
+  SortNetworks(&networks_);
+  for (WiFiService::NetworkList::const_iterator it = networks_.begin();
+       it != networks_.end();
+       ++it) {
+    scoped_ptr<base::DictionaryValue> network(it->ToValue(true));
+    network_list->Append(network.release());
+  }
+}
+
+void WiFiServiceImpl::RequestNetworkScan() {
+  base::mac::ScopedNSAutoreleasePool auto_pool;
+  NetworkList networks;
+  DVLOG(1) << __FUNCTION__;
+
+  NSError* nsError = GetVisibleNetworkList(std::string(), &networks);

[Robert Sesek, 2014/01/07 16:19:05]

naming: nsError. Here and throughout.

[mef, 2014/01/08 21:00:03]

Done.

+  if (nsError == nil && !networks.empty()) {
+    NotifyNetworkListChanged(networks);
+  }
+}
+
+void WiFiServiceImpl::StartConnect(const std::string& network_guid,
+                                   std::string* error) {
+  base::mac::ScopedNSAutoreleasePool auto_pool;
+  NSError* nsError = nil;
+
+  DVLOG(1) << "*** StartConnect: " << network_guid;
+  NSSet* networks = [interface_
+      scanForNetworksWithName:SSIDFromGUID(network_guid)
+                        error:&nsError];
+
+  if (CheckError(nsError, "Error.scanForNetworksWithName", error))

[Robert Sesek, 2014/01/07 16:19:05]

Should these errors be made into constants?

[mef, 2014/01/08 21:00:03]

Done.

+    return;
+
+  CWNetwork* network = [networks anyObject];
+  if (network == nil) {
+    *error = "Error.NotFound";
+    return;
+  }
+  // Remember previously connected network.
+  std::string connected_network_guid = GUIDFromSSID([interface_ ssid]);
+  // Check, whether desired network is already connected.
+  if (network_guid == connected_network_guid) {
+    NotifyNetworkChanged(connected_network_guid);
+    return;
+  }
+  // Check whether WiFi Password is set in |network_properties_|
+  base::DictionaryValue* properties;
+  base::DictionaryValue* wifi;
+  std::string passphrase;
+  NSString* nsPassword = nil;

[Robert Sesek, 2014/01/07 16:19:05]

naming

[mef, 2014/01/08 21:00:03]

Done.

+  if (network_properties_.GetDictionaryWithoutPathExpansion(network_guid,
+                                                            &properties) &&
+      properties->GetDictionary(onc::network_type::kWiFi, &wifi) &&
+      wifi->GetString(onc::wifi::kPassphrase, &passphrase)) {
+    nsPassword = [NSString stringWithUTF8String:passphrase.c_str()];

[Robert Sesek, 2014/01/07 16:19:05]

base/strings/sys_string_conversions.h

[mef, 2014/01/08 21:00:03]

Done.

+  }
+
+  // Disable automatic network change notifications as they get fired
+  // when network is just connected, but not yet accessible (doesn't
+  // have valid IP address).

[tbarzic, 2014/01/07 20:05:33]

This seems wrong..
what if another network gets connected while we're waiting for this one to
become accessible?

Also, the network might change by means other than this API, and this suggests
that in these they would erroneously be reported as "Connected". Can we just set
the network status to "Connecting" if it's 'connected' but not 'accessible'?

[mef, 2014/01/08 21:00:03]

I think I should change 'GetProperties' to check reachability of network
[interface_ ssid], and report state as 'Connecting' if network is not reachable
yet. Does this make sense?

[tbarzic, 2014/01/08 23:06:23]

sounds good

[mef, 2014/01/10 20:26:16]

Done.

+  enable_notify_network_changed_ = false;
+  // Number of attempts to associate to network.
+  static const int kMaxAssociationAttempts = 3;
+  // Try to associate to network several times if timeout or PMK error occurs.
+  for (int i = 0; i < kMaxAssociationAttempts; ++i) {
+    // Nil out the PMK to prevent stale data from causing invalid PMK error
+    // (CoreWLANTypes -3924).
+    [interface_ setPairwiseMasterKey:nil error:&nsError];
+    if ([interface_ associateToNetwork:network

[Robert Sesek, 2014/01/07 16:19:05]

What if this is running on 10.6? This method is only declared on 10.7+.

It seems that this method used to be |-
(BOOL)associateToNetwork:(CWNetwork*)network
parameters:(NSDictionary*)parameters error:(NSError**)error| in 10.6

[mef, 2014/01/08 21:00:03]

Couple of followup questions:
- What is suggested way of dealing with API differences? 
- Where can I find a 10.6 API documentation?

[Robert Sesek, 2014/01/09 23:12:45]

This is the first time I've ever seen Apple actually _remove_ deprecated methods
from the API headers, which is really weird. Typically you do this using:

if ([obj respondsToSelector:@selector(methodName:withArg:arg2:)] {
  [obj methodName:…];
}

I don't know how to dig up old API documentation, but I can provide you a link
to the old 10.6 SDK where you can look at the header comments.

[mef, 2014/01/10 17:01:52]

Done.

+                              password:nsPassword
+                                 error:&nsError]) {
+      // Notify that previously connected network has changed.
+      NotifyNetworkChanged(connected_network_guid);
+
+      // Start waiting for network connection state change.

[tbarzic, 2014/01/07 20:05:33]

Add a comment that WaiForNetworkConnect is async and that it'll reset
enable_notify_network_changed_

[mef, 2014/01/08 21:00:03]

Done.

+      if (!networks_changed_observer_.is_null()) {
+        WaitForNetworkConnect(network_guid, 0);
+        return;
+      }
+    } else {
+      long error_code = [nsError code];

[Robert Sesek, 2014/01/07 16:19:05]

long -> NSInteger

[mef, 2014/01/08 21:00:03]

Done.

+      if (error_code != kCWTimeoutErr && error_code != kCWInvalidPMKErr) {
+        break;
+      }
+    }
+  }
+  enable_notify_network_changed_ = true;
+  CheckError(nsError, "Error.associateToNetwork", error);
+}
+
+void WiFiServiceImpl::StartDisconnect(const std::string& network_guid,
+                                      std::string* error) {
+  base::mac::ScopedNSAutoreleasePool auto_pool;
+  DVLOG(1) << "*** StartDisconnect: " << network_guid;
+
+  if (network_guid == GUIDFromSSID([interface_ ssid])) {
+    [interface_ disassociate];
+    NotifyNetworkChanged(network_guid);

[tbarzic, 2014/01/07 20:05:33]

Shouldn't this be reported from |OnListenerNotification|

[mef, 2014/01/08 21:00:03]

Done.

+  } else {
+    *error = "not-connected";

[Robert Sesek, 2014/01/07 16:19:05]

Why doesn't this error start with |Error.|?

[mef, 2014/01/08 21:00:03]

Done.

+  }
+}
+
+void WiFiServiceImpl::SetEventObservers(
+    scoped_refptr<base::MessageLoopProxy> message_loop_proxy,
+    const NetworkGuidListCallback& networks_changed_observer,
+    const NetworkGuidListCallback& network_list_changed_observer) {
+  base::mac::ScopedNSAutoreleasePool auto_pool;
+  message_loop_proxy_.swap(message_loop_proxy);
+  networks_changed_observer_ = networks_changed_observer;
+  network_list_changed_observer_ = network_list_changed_observer;
+}
+
+void WiFiServiceImpl::WaitForNetworkConnect(const std::string& network_guid,
+                                            int attempt) {
+  // If network didn't get connected in |kMaxAttempts|, then restore automatic
+  // network change notifications and stop waiting.
+  if (attempt > kMaxAttempts) {
+    DLOG(ERROR) << kMaxAttempts << " attempts exceeded waiting for connect to "
+                << network_guid;
+    // Restore previously suppressed notifications.
+    enable_notify_network_changed_ = true;
+    return;
+  }
+
+  // Check whether WiFi network is reachable.
+  struct sockaddr_in localWiFiAddress;

[Robert Sesek, 2014/01/07 16:19:05]

naming

[mef, 2014/01/08 21:00:03]

Done.

+  bzero(&localWiFiAddress, sizeof(localWiFiAddress));
+  localWiFiAddress.sin_len = sizeof(localWiFiAddress);
+  localWiFiAddress.sin_family = AF_INET;
+  localWiFiAddress.sin_addr.s_addr = htonl(IN_LINKLOCALNETNUM);
+
+  SCNetworkReachabilityRef reachability =

[Robert Sesek, 2014/01/07 16:19:05]

Use a ScopedCFTypeRef.

[mef, 2014/01/08 21:00:03]

Done.

+      SCNetworkReachabilityCreateWithAddress(
+          kCFAllocatorDefault,
+          (const struct sockaddr*)&localWiFiAddress);

[Robert Sesek, 2014/01/07 16:19:05]

C-style casts are banned.

[mef, 2014/01/08 21:00:03]

Done.

+  SCNetworkReachabilityFlags flags = 0u;
+  if (SCNetworkReachabilityGetFlags(reachability, &flags) &&
+      (flags & kSCNetworkReachabilityFlagsReachable) &&
+      (flags & kSCNetworkReachabilityFlagsIsDirect)) {
+    DVLOG(1) << "WiFi Connected, Reachable: " << network_guid;
+    // Restore previously suppressed notifications.
+    enable_notify_network_changed_ = true;
+    NotifyNetworkChanged(network_guid);
+  } else {
+    DVLOG(1) << "Attempt:"  << attempt << ", reachability:" << flags;
+    // Continue waiting for network connection state change.
+    task_runner_->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&WiFiServiceImpl::WaitForNetworkConnect,
+                   base::Unretained(this),
+                   network_guid,
+                   ++attempt),
+        base::TimeDelta::FromMilliseconds(kAttemptDelayMs));
+  }
+
+  if (reachability)
+    CFRelease(reachability);
+}
+
+
+NSError* WiFiServiceImpl::GetVisibleNetworkList(const std::string& network_guid,
+                                                NetworkList* network_list) {
+
+  NSError* nsError = nil;
+  NSString* networkName = nil;

[Robert Sesek, 2014/01/07 16:19:05]

naming

[mef, 2014/01/08 21:00:03]

Done.

+
+  DVLOG(1) << "<<< GetVisibleNetworkList: " << network_guid;
+
+  if (!network_guid.empty())
+    networkName = SSIDFromGUID(network_guid);
+
+  NSSet* networks = [interface_ scanForNetworksWithName:networkName
+                                                  error:&nsError];
+  if (nsError != nil)
+    return nsError;
+
+  std::map<std::string, NetworkProperties*> network_properties_map;
+
+  CWNetwork* network;
+  // There is one |network| per BSS in |networks|, so go through the set and
+  // combine them, paying attention to supported frequencies.
+  for(network in networks) {

[Robert Sesek, 2014/01/07 16:19:05]

nit: space before (

[mef, 2014/01/08 21:00:03]

Done.

+    NetworkProperties network_properties;
+    NetworkPropertiesFromCWNetwork(network, &network_properties);
+
+    if (network_properties_map.find(network_properties.guid) ==
+        network_properties_map.end()) {
+      network_list->push_back(network_properties);
+      network_properties_map[network_properties.guid] = &network_list->back();
+    } else {
+      NetworkProperties* existing = network_properties_map.at(
+          network_properties.guid);
+      existing->frequency_set.insert(*network_properties.frequency_set.begin());
+    }
+  }
+  DVLOG(1) << ">>> GetVisibleNetworkList: " << network_guid;
+
+  return nil;
+}
+
+bool WiFiServiceImpl::CheckError(NSError* nsError,
+                                 const char* error_name,
+                                 std::string* error) const {
+  if (nsError != nil) {
+    DLOG(ERROR) << "*** Error:" << error_name << ":" << [nsError code];
+    *error = error_name;
+    return true;
+  }
+  return false;
+}
+
+void WiFiServiceImpl::NetworkPropertiesFromCWNetwork(
+    const CWNetwork* network,
+    NetworkProperties* properties) const {
+
+  if (NSOrderedSame == [[network ssid] compare:[interface_ ssid]])

[Robert Sesek, 2014/01/07 16:19:05]

Preference is for |expr == NSOrderedSame|

http://dev.chromium.org/developers/coding-style#Code_Formatting

[mef, 2014/01/08 21:00:03]

Done.

+    properties->connection_state = onc::connection_state::kConnected;
+  else
+    properties->connection_state = onc::connection_state::kNotConnected;
+
+  properties->ssid = [[network ssid] UTF8String];

[Robert Sesek, 2014/01/07 16:19:05]

sys_string_conversions.h. This looks like you're assigning a weak C-string
pointer to something that will outlive this function.

[mef, 2014/01/08 21:00:03]

|ssid| and other properties are std::string.

[Robert Sesek, 2014/01/09 23:12:45]

Yes, but I had to look that up. It looks like you're assigning to a const char*.

[mef, 2014/01/10 17:01:52]

Any suggestions on making it more apparent?

[Robert Sesek, 2014/01/10 18:59:51]

Yes, the string conversion function to make it explicit for operator=(const
std::string&)

[mef, 2014/01/10 20:26:16]

Done. Use SysNSStringToUTF8.

+  properties->name = properties->ssid;
+  properties->guid = GUIDFromSSID([network ssid]);
+  properties->type = onc::network_type::kWiFi;
+
+  properties->bssid = [[network bssid] UTF8String];
+  if ([[network wlanChannel] channelBand] == kCWChannelBand2GHz)

[Robert Sesek, 2014/01/07 16:19:05]

Again, what if this is running on a 10.6 machine?

[mef, 2014/01/08 21:00:03]

Is there some other way to get this info on 10.6 machine?
I especially need bssid, but band would also be nice to have.

[Robert Sesek, 2014/01/09 23:12:45]

That I do not know, unfortunately :|.

[mef, 2014/01/10 17:01:52]

It seems that native ChromeCast App only supports 10.7+, so it should be ok to
just fail gracefully.

[Robert Sesek, 2014/01/10 18:59:51]

Chrome needs to run on 10.6. If you call this as is, it will crash on every 10.6
machine.

[mef, 2014/01/10 20:26:16]

Done. Added respondsToSelector:@selector(associateToNetwork:password:error:) to
WiFiServiceMac::Initialize, and release interface_ if it is not supported.

+    properties->frequency = kFrequency2400;
+  else
+    properties->frequency = kFrequency5000;
+  properties->frequency_set.insert(properties->frequency);
+  properties->security = SecurityFromCWSecurityMode(
+      static_cast<CWSecurityMode>([[network securityMode] intValue]));
+
+  properties->signal_strength = [[network rssi] intValue];
+}
+
+std::string WiFiServiceImpl::SecurityFromCWSecurityMode(
+    CWSecurityMode security) const {
+  switch (security) {
+    case kCWSecurityModeWPA_Enterprise:
+    case kCWSecurityModeWPA2_Enterprise:
+      return onc::wifi::kWPA_EAP;
+    case kCWSecurityModeWPA_PSK:
+    case kCWSecurityModeWPA2_PSK:
+      return onc::wifi::kWPA_PSK;
+    case kCWSecurityModeWEP:
+      return onc::wifi::kWEP_PSK;
+    case kCWSecurityModeOpen:
+      return onc::wifi::kNone;
+    // TODO(mef): Figure out correct mapping.
+    case kCWSecurityModeWPS:
+    case kCWSecurityModeDynamicWEP:
+      return onc::wifi::kWPA_EAP;
+  }
+  return onc::wifi::kWPA_EAP;
+}
+
+void WiFiServiceImpl::SortNetworks(NetworkList* networks) {

[Robert Sesek, 2014/01/07 16:19:05]

This is called exactly once. Why does it need its own method?

[mef, 2014/01/08 21:00:03]

Done.

+  networks->sort(NetworkProperties::OrderByType);
+}
+
+

[Robert Sesek, 2014/01/07 16:19:05]

nit: double blank line

[mef, 2014/01/08 21:00:03]

Done.

+void WiFiServiceImpl::OnListenerNotification() {
+  std::string connected_network_guid = GUIDFromSSID([interface_ ssid]);
+  DVLOG(1) << " *** Got Notification: " << connected_network_guid;
+  if (connected_network_guid.empty()) {
+    // Find previously connected network and notify that it is disconnected.

[tbarzic, 2014/01/07 20:05:33]

When a connected network changes from A to B, will there always be a
notification with empty interface_ ssid?
If not, we may want to do this even if |connected_network_guid| is not empty.

[mef, 2014/01/08 21:00:03]

In my testing it appears to always get notification for disconnect, so
[interface_ ssid] is empty first.

[tbarzic, 2014/01/08 23:06:23]

You don't seem too sure about this :)
Can you add a comment?

[mef, 2014/01/10 17:01:52]

Done.

+    for (WiFiService::NetworkList::iterator it = networks_.begin();
+         it != networks_.end();
+         ++it) {
+      if (it->connection_state == onc::connection_state::kConnected) {
+        it->connection_state = onc::connection_state::kNotConnected;
+        NotifyNetworkChanged(it->guid);
+      }
+    }
+  } else {
+    NotifyNetworkChanged(connected_network_guid);
+  }
+}
+
+void WiFiServiceImpl::NotifyNetworkListChanged(const NetworkList& networks) {
+  if (network_list_changed_observer_.is_null())

[Robert Sesek, 2014/01/07 16:19:05]

Why listen for network change events if nothing is going to use that data?

[mef, 2014/01/08 21:00:03]

Because caller could've done |RequestNetworkScan| without setting
|network_list_changed_observer_|.

[tbarzic, 2014/01/08 23:06:23]

I agree that there is no real need to observe network changes if there are no
listeners.

[mef, 2014/01/10 20:26:16]

Done.

+    return;
+
+  NetworkGuidList current_networks;
+  for (NetworkList::const_iterator it = networks.begin();
+       it != networks.end();
+       ++it) {
+    current_networks.push_back(it->guid);
+  }
+
+  message_loop_proxy_->PostTask(
+      FROM_HERE,
+      base::Bind(network_list_changed_observer_, current_networks));
+}
+
+void WiFiServiceImpl::NotifyNetworkChanged(const std::string& network_guid) {
+  if (!enable_notify_network_changed_ || networks_changed_observer_.is_null())

[Robert Sesek, 2014/01/07 16:19:05]

Same question.

[mef, 2014/01/08 21:00:03]

Done. I've moved 'NSNotificationCenter addObserverForName' into
|WiFiServiceMac::SetEventObservers|.

+    return;
+
+  DVLOG(1) << "NotifyNetworkChanged: " << network_guid;
+  NetworkGuidList changed_networks(1, network_guid);
+  message_loop_proxy_->PostTask(
+      FROM_HERE,
+      base::Bind(networks_changed_observer_, changed_networks));
+}
+

[Robert Sesek, 2014/01/07 16:19:05]

"// static"

[mef, 2014/01/08 21:00:03]

Done.

+WiFiService* WiFiService::Create() { return new WiFiServiceImpl(); }
+
+}  // namespace wifi