Add chromeos::NetworkStateManager

chromeos/network/network_state_handler.cc

+// Copyright (c) 2012 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 "chromeos/network/network_state_handler.h"
+
+#include <algorithm>

[pneubeck (no reviews), 2012/10/25 14:42:17]

unused?

[stevenjb, 2012/10/25 22:05:22]

Removed

+
+#include "base/bind.h"
+#include "base/stl_util.h"
+#include "base/string_util.h"
+#include "base/values.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/shill_device_client.h"
+#include "chromeos/dbus/shill_ipconfig_client.h"
+#include "chromeos/dbus/shill_manager_client.h"
+#include "chromeos/dbus/shill_service_client.h"
+#include "chromeos/network/device_state.h"
+#include "chromeos/network/managed_state.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler_observer.h"
+#include "chromeos/network/network_service_observer.h"
+#include "dbus/object_path.h"
+
+namespace {
+
+void ErrorCallbackFunction(const std::string& error_name,
+                           const std::string& error_message) {
+  // TODO(stevenjb): Add error logging.
+  LOG(ERROR) << "Shill Error: " << error_name << " : " << error_message;
+}
+
+const base::ListValue* GetListValue(const std::string& key,
+                                    const base::Value& value) {
+  const base::ListValue* vlist = NULL;
+  if (!value.GetAsList(&vlist)) {
+    LOG(ERROR) << "Error parsing key as list: " << key;
+    return NULL;
+  }
+  return vlist;
+}
+
+}
+
+namespace chromeos {
+
+NetworkStateHandler::NetworkStateHandler()
+    : shill_manager_(DBusThreadManager::Get()->GetShillManagerClient()),
+      weak_ptr_factory_(this) {
+}
+
+NetworkStateHandler::~NetworkStateHandler() {
+  STLDeleteContainerPointers(network_list_.begin(), network_list_.end());
+  // Delete network service observers.
+  STLDeleteContainerPairSecondPointers(
+      observed_networks_.begin(), observed_networks_.end());
+  CHECK(shill_manager_ == DBusThreadManager::Get()->GetShillManagerClient());
+  shill_manager_->RemovePropertyChangedObserver(this);
+}
+
+void NetworkStateHandler::Init() {
+  shill_manager_->GetProperties(
+      base::Bind(&NetworkStateHandler::ManagerPropertiesCallback,
+                 weak_ptr_factory_.GetWeakPtr()));
+  shill_manager_->AddPropertyChangedObserver(this);
+}
+
+void NetworkStateHandler::AddObserver(NetworkStateHandlerObserver* observer) {
+  observers_.AddObserver(observer);
+}
+
+void NetworkStateHandler::RemoveObserver(
+    NetworkStateHandlerObserver* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+bool NetworkStateHandler::TechnologyAvailable(
+    const std::string& technology) const {
+  return available_technologies_.find(technology) !=
+      available_technologies_.end();
+}
+
+bool NetworkStateHandler::TechnologyEnabled(
+    const std::string& technology) const {
+  return enabled_technologies_.find(technology) !=
+      enabled_technologies_.end();
+}
+
+void NetworkStateHandler::SetTechnologyEnabled(const std::string& technology,
+                                               bool enabled) {
+  if (enabled) {
+    shill_manager_->EnableTechnology(technology,
+                                     base::Bind(&base::DoNothing),
+                                     base::Bind(&ErrorCallbackFunction));
+  } else {
+    shill_manager_->DisableTechnology(technology,
+                                      base::Bind(&base::DoNothing),
+                                      base::Bind(&ErrorCallbackFunction));
+  }
+}
+
+const DeviceState* NetworkStateHandler::GetDeviceState(
+    const std::string& path) const {
+  return const_cast<NetworkStateHandler*>(this)->GetModifiableDeviceState(path);

[pneubeck (no reviews), 2012/10/25 14:42:17]

I understand that for public functions. But why would that be a problem here?
The  GetModifiable*State methods are private methods and not part of the public
interface or to derived classes (which we don't expect).
The const_cast is really not necessary:

const DeviceState* GetDeviceState(
    const std::string& path) const {
  return GetModifiableDeviceState(path);
}

DeviceState* GetModifiableDeviceState(
     const std::string& path) const {
  for (DeviceStateList::const_iterator iter = ...)
    ...
}

[stevenjb, 2012/10/25 22:05:22]

Yeah, I see your point. It seems a bit awkward, but so is const_cast. I don't
feel strongly either way, so I'll change it. I added a comment in the header for
GetModifiable*.

On 2012/10/25 14:42:17, pneubeck wrote:
GetModifiableDeviceState() 

+}
+
+const DeviceState* NetworkStateHandler::GetDeviceStateByType(
+    const std::string& type) const {
+  for (DeviceStateList::const_iterator iter = device_list_.begin();
+       iter != device_list_.end(); ++iter) {
+    const DeviceState* device = *iter;
+    if (device->type() == type)
+      return device;
+  }
+  return NULL;
+}
+
+const NetworkState* NetworkStateHandler::GetNetworkState(
+    const std::string& path) const {
+  return const_cast<NetworkStateHandler*>(this)->GetModifiableNetworkState(
+      path);
+}
+
+const NetworkState* NetworkStateHandler::ActiveNetwork() const {
+  if (network_list_.empty())
+    return NULL;
+  const NetworkState* network = network_list_.front();
+  if (!network->IsConnectedState())
+    return NULL;
+  return network;
+}
+
+const NetworkState* NetworkStateHandler::ConnectedNetworkByType(
+    const std::string& type) const {
+  for (NetworkStateList::const_iterator iter = network_list_.begin();
+       iter != network_list_.end(); ++iter) {
+    const NetworkState* network = *iter;
+    if (!network->IsConnectedState())
+      break;  // Connected networks are listed first.
+    if (network->type() == type)
+      return network;
+  }
+  return NULL;
+}
+
+const NetworkState* NetworkStateHandler::ConnectingNetworkByType(
+    const std::string& type) const {
+  for (NetworkStateList::const_iterator iter = network_list_.begin();
+       iter != network_list_.end(); ++iter) {
+    const NetworkState* network = *iter;
+    if (network->IsConnectedState())
+      continue;
+    if (!network->IsConnectingState())
+      break;  // Connected and connecting networks are listed first.
+    if (network->type() == type ||
+        (type.empty() && type != flimflam::kTypeEthernet)) {
+      return network;
+    }
+  }
+  return NULL;
+}
+
+std::string NetworkStateHandler::HardwareAddress(
+    const std::string& type) const {
+  std::string result;
+  const NetworkState* network = ConnectedNetworkByType(type);
+  if (network) {
+    const DeviceState* device = GetDeviceState(network->device_path());
+    if (device)
+      result = device->mac_address();
+  }
+  StringToUpperASCII(&result);
+  return result;
+}
+
+std::string NetworkStateHandler::HardwareAddressFormatted(
+    const std::string& type) const {
+  std::string address = HardwareAddress(type);
+  if (address.size() % 2 != 0)
+    return address;
+  std::string result;
+  for (size_t i = 0; i < address.size(); ++i) {
+    if ((i != 0) && (i % 2 == 0))
+      result.push_back(':');
+    result.push_back(address[i]);
+  }
+  return result;
+}
+
+const NetworkStateHandler::NetworkStateList&
+NetworkStateHandler::GetNetworkList() const {
+  RequestScan();

[pneubeck (no reviews), 2012/10/25 14:42:17]

Why not expose RequestScan publicly and remove it here? The list you return here
is not updated by the scan on return time. So the caller has to observe
NetworkList changes in anyway.
The current implementation only provokes unintentional scan requests.

[stevenjb, 2012/10/25 22:05:22]

We decidedly to explicitly not expose RequestScan. Eventually this is something
that will be triggered implicitly in Shill and the RequestScan will be a no-op.

The assumption is that if the UI wants the list of networks, it wants to know
their latest state also, i.e. the scan is not unintentional.

+  return network_list_;
+}
+
+void NetworkStateHandler::OnPropertyChanged(const std::string& key,
+                                            const base::Value& value) {
+  if (ManagerPropertyChanged(key, value))
+    FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                      NetworkManagerChanged(key));
+}
+
+//------------------------------------------------------------------------------
+// Private methods
+
+DeviceState* NetworkStateHandler::GetModifiableDeviceState(
+    const std::string& path) {
+  for (DeviceStateList::iterator iter = device_list_.begin();
+       iter != device_list_.end(); ++iter) {
+    DeviceState* device = *iter;
+    if (device->path() == path)
+      return device;
+  }
+  return NULL;
+}
+
+NetworkState* NetworkStateHandler::GetModifiableNetworkState(
+    const std::string& path) {
+  for (NetworkStateList::iterator iter = network_list_.begin();
+       iter != network_list_.end(); ++iter) {
+    NetworkState* network = *iter;
+    if (network->path() == path)
+      return network;
+  }
+  return NULL;
+}
+
+void NetworkStateHandler::ManagerPropertiesCallback(
+    DBusMethodCallStatus call_status,
+    const base::DictionaryValue& properties) {
+  if (call_status != DBUS_METHOD_CALL_SUCCESS) {
+    LOG(ERROR) << "Failed to get Manager properties:" << call_status;
+    return;
+  }
+  bool notify = false;
+  for (base::DictionaryValue::key_iterator iter = properties.begin_keys();

[pneubeck (no reviews), 2012/10/25 14:42:17]

You could use DictionaryValue::Iterator in this case.
Improves readability as GetWithoutPathExpansion and the if can be removed.

Applies to most cases in this file.

[stevenjb, 2012/10/25 22:05:22]

I'd never seen that before. Nice.
Done.

+       iter != properties.end_keys(); ++iter) {
+    const std::string& key = *iter;
+    const base::Value* value;
+    bool res = properties.GetWithoutPathExpansion(key, &value);
+    if (res)
+      notify |= ManagerPropertyChanged(key, *value);
+    else
+      LOG(ERROR) << "Error getting value for key: " << key;
+  }
+  if (notify) {
+    FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                      NetworkManagerChanged(std::string()));
+  }
+}
+
+bool NetworkStateHandler::ManagerPropertyChanged(const std::string& key,
+                                                 const base::Value& value) {
+  bool notify_manager_changed = false;
+  if (key == flimflam::kServicesProperty) {
+    const base::ListValue* vlist = GetListValue(key, value);
+    if (vlist) {
+      UpdateManagedList(ManagedState::MANAGED_TYPE_NETWORK, *vlist);
+      // Do not send an UpdateManagerChanged notification to observers.
+      // An UpdateNetworkList notification will be sent when the service list
+      // updates have completed.
+    }
+  } else if (key == flimflam::kDevicesProperty) {
+    const ListValue* vlist = GetListValue(key, value);
+    if (vlist) {
+      UpdateManagedList(ManagedState::MANAGED_TYPE_DEVICE, *vlist);
+      notify_manager_changed = true;
+    }
+  } else if (key == flimflam::kAvailableTechnologiesProperty) {
+    const base::ListValue* vlist = GetListValue(key, value);
+    if (vlist ) {
+      UpdateAvailableTechnologies(*vlist);
+      notify_manager_changed = true;
+    }
+  } else if (key == flimflam::kEnabledTechnologiesProperty) {
+    const base::ListValue* vlist = GetListValue(key, value);
+    if (vlist) {
+      UpdateEnabledTechnologies(*vlist);
+      notify_manager_changed = true;
+    }
+  }
+  return notify_manager_changed;
+}
+
+NetworkStateHandler::ManagedStateList* NetworkStateHandler::GetManagedList(
+    ManagedState::ManagedType type) {
+  // Use reinterpret_cast here to avoid copying the list to a list of identical

[pneubeck (no reviews), 2012/10/25 14:42:17]

Copying a vector<*> of about 10 entries should be very very fast, the one alloc
will take the most time. Why should that be a problem? How often should this
function be called? Do we have any measurements? Although, I assume that Dbus
will make problems much earlier.
Anyways, in the template version there is also no copy necessary.
This is neither a downcast nor a upcast. list<A> and list<B> (A != B) are in no
subtype relation.
As list<A> and list<B> don't share the same code, are you sure that the compiler
even compiles both code instantiations of list<T> equally?

And even if the cast would work, the resulting list is modifiable. So the
compiler doesn't complain if you insert the wrong elements into the list, e.g
you can add a NetworkState to the DeviceState list. That is what I meant with
losing type-safety.

To summarize: this cast is unchecked and by definition incorrect and because of
that, operations on the resulting list are unsafe.

About the template version. Why should the debugging be a pain in that case?

[stevenjb, 2012/10/25 22:05:22]

I have had difficulty stepping through templated functions in the past. It's one
of the reasons I prefer to avoid them. I also find them a pain in the ass to
read.

<A> and <B> are pointers, and we're downcasting the pointers. There is no
typesafe way to do this in C++, but we *know* that network_list_ and
device_list_ contain pointers to ManagedState, so treating them as lists to
baseclass pointers is safe.

So, yes, in theory, there are plenty of things the compiler can't check for.
However, this is a private method, and misuse of it would be, well, a bug.

If you really have a problem with this, I'll do it another way, but not with
function templates. Maybe with a templated wrapper class for the lists, I'll
have to think on it.

+  // base class pointers.
+  switch(type) {
+    case ManagedState::MANAGED_TYPE_NETWORK:
+      return reinterpret_cast<ManagedStateList*>(&network_list_);
+    case ManagedState::MANAGED_TYPE_DEVICE:
+      return reinterpret_cast<ManagedStateList*>(&device_list_);
+  }
+  return NULL;
+}
+
+void NetworkStateHandler::UpdateManagedList(ManagedState::ManagedType type,
+                                            const base::ListValue& entries) {
+  ManagedStateList* managed_list = GetManagedList(type);
+  VLOG(2) << "UpdateManagedList: " << type;
+  // Create a map of existing entries.
+  std::map<std::string, ManagedState*> managed_map;
+  for (ManagedStateList::iterator iter = managed_list->begin();
+       iter != managed_list->end(); ++iter) {
+    ManagedState* managed = *iter;
+    managed_map[managed->path()] = managed;
+  }
+  // Clear the list (pointers are owned by managed_map).
+  managed_list->clear();
+  // Updates managed_list and request updates for new entries.
+  for (base::ListValue::const_iterator iter = entries.begin();
+       iter != entries.end(); ++iter) {
+    std::string path;
+    (*iter)->GetAsString(&path);
+    if (path.empty())
+      continue;
+    std::map<std::string, ManagedState*>::iterator found =
+        managed_map.find(path);
+    bool request_properties = false;
+    if (found == managed_map.end()) {
+      request_properties = true;
+      managed_list->push_back(ManagedState::Create(type, path));
+    } else {
+      ManagedState* managed = found->second;
+      managed_list->push_back(managed);
+      managed_map.erase(found);
+    }
+    if (request_properties) {
+      ++pending_updates_[type];
+      RequestProperties(type, path);
+    }
+  }
+  // Delete any remaning entries in managed_map.
+  STLDeleteContainerPairSecondPointers(managed_map.begin(), managed_map.end());
+}
+
+void NetworkStateHandler::UpdateObservedNetworkServices() {
+  // Watch any connected or connecting networks.
+  NetworkServiceObserverMap new_observed;
+  for (NetworkStateList::const_iterator iter = network_list_.begin();
+       iter != network_list_.end(); ++iter) {
+    const NetworkState* network = *iter;
+    if (!network->IsConnectedState() && !network->IsConnectingState())

[pneubeck (no reviews), 2012/10/25 14:42:17]

Doesn't this also mean, that we cannot observe e.g. the signal strength of a
idle network anymore?

I'm still not getting the point of the "not observing idle networks". If a
network is not connected, it actually shouldn't change a lot and thus we will
not get many state updates.

[stevenjb, 2012/10/25 22:05:22]

Shill does not update the strength or other properties of idle networks, so
observing them is just extra overhead, except for the case where they come out
of idle, which would have been a bug as described in the email thread. Keep in
mind that in a busy area (like my condo), there can be O(100) idle networks. 

This will switch to observing members of the watch list (all networks for now),
and we will rely on Shill to pair this down to "interesting" networks. I will
put that in a separate patch.

+      break;  // Connected and connecting networks are listed first.
+    const std::string& path = network->path();
+    NetworkServiceObserverMap::iterator iter = observed_networks_.find(path);
+    if (iter != observed_networks_.end()) {
+      new_observed[path] = observed_networks_[path];
+    } else {
+      new_observed[path] = new NetworkServiceObserver(
+          path, base::Bind(&NetworkStateHandler::NetworkServicePropertyChanged,
+                           weak_ptr_factory_.GetWeakPtr()));
+    }
+    observed_networks_.erase(path);
+  }
+  VLOG(2) << "UpdateObservedNetworkServices, new observed: "
+          << new_observed.size();
+  // Delete network service observers still in observed_networks_.
+  STLDeleteContainerPairSecondPointers(
+      observed_networks_.begin(), observed_networks_.end());
+  observed_networks_.swap(new_observed);
+  // Notify observers that the list of networks has changed.
+  FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                    NetworkListChanged(network_list_));
+  // Notify observers if the active network has changed.
+  const NetworkState* new_active_network =
+      network_list_.empty() ? NULL : network_list_.front();
+  if (new_active_network->path() != active_network_path_) {
+    active_network_path_ = new_active_network->path();
+    FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                      ActiveNetworkChanged(new_active_network));
+  }
+}
+
+void NetworkStateHandler::UpdateAvailableTechnologies(
+    const base::ListValue& technologies) {
+  available_technologies_.clear();
+  for (base::ListValue::const_iterator iter = technologies.begin();
+       iter != technologies.end(); ++iter) {
+    std::string technology;
+    (*iter)->GetAsString(&technology);
+    if (technology.empty())
+      continue;
+    available_technologies_.insert(technology);
+  }
+}
+
+void NetworkStateHandler::UpdateEnabledTechnologies(
+    const base::ListValue& technologies) {
+  enabled_technologies_.clear();
+  for (base::ListValue::const_iterator iter = technologies.begin();
+       iter != technologies.end(); ++iter) {
+    std::string technology;
+    (*iter)->GetAsString(&technology);
+    if (technology.empty())
+      continue;
+    enabled_technologies_.insert(technology);
+  }
+}
+
+void NetworkStateHandler::RequestProperties(ManagedState::ManagedType type,
+                                            const std::string& path) {
+  switch(type) {
+    case ManagedState::MANAGED_TYPE_NETWORK:
+      DBusThreadManager::Get()->GetShillServiceClient()->GetProperties(
+          dbus::ObjectPath(path),
+          base::Bind(&NetworkStateHandler::GetPropertiesCallback,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     type, path));
+      break;
+    case ManagedState::MANAGED_TYPE_DEVICE:
+      DBusThreadManager::Get()->GetShillDeviceClient()->GetProperties(
+          dbus::ObjectPath(path),
+          base::Bind(&NetworkStateHandler::GetPropertiesCallback,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     type, path));
+      break;
+  }
+}
+
+void NetworkStateHandler::GetPropertiesCallback(
+    ManagedState::ManagedType type,
+    const std::string& path,
+    DBusMethodCallStatus call_status,
+    const base::DictionaryValue& properties) {
+  VLOG(2) << "GetPropertiesCallback: " << type << " : " << path;
+  --pending_updates_[type];
+  if (call_status != DBUS_METHOD_CALL_SUCCESS) {
+    LOG(ERROR) << "Failed to get properties for: " << path
+               << ": " << call_status;
+    return;
+  }
+  bool found = false;
+  ManagedStateList* list = GetManagedList(type);
+  for (ManagedStateList::iterator iter = list->begin();

[pneubeck (no reviews), 2012/10/25 14:42:17]

Here, you could use
ManagedState* managed = GetModifiable*State(path);

[stevenjb, 2012/10/25 22:05:22]

I've been trying to keep Type specific code out of these (i.e. that would
require an if() to call the right method. Let me think on a better way to handle
the common abstraction that will also work for Get*State.

+       iter != list->end(); ++iter) {
+    ManagedState* managed = *iter;
+    if (managed->path() == path) {
+      ParseProperties(managed, properties);
+      found = true;
+      break;
+    }
+  }
+  if (!found)
+    LOG(ERROR) << "GetPropertiesCallback: " << path << " Not found!";
+  // Notify observers only when all updates for that type have completed.
+  if (type == ManagedState::MANAGED_TYPE_NETWORK &&
+      pending_updates_[type] == 0) {
+    UpdateObservedNetworkServices();
+  }
+}
+
+void NetworkStateHandler::ParseProperties(
+    ManagedState* managed,
+    const base::DictionaryValue& properties) {
+  for (base::DictionaryValue::key_iterator iter = properties.begin_keys();

[pneubeck (no reviews), 2012/10/25 14:42:17]

DictionaryValue::Iterator

[stevenjb, 2012/10/25 22:05:22]

Done.

+       iter != properties.end_keys(); ++iter) {
+    bool success = false;
+    const std::string& key = *iter;
+    // Handle IPConfig here since it requires additional complexity that is
+    // better handled here than in NetworkService::PropertyChanged (e.g.

[pneubeck (no reviews), 2012/10/25 14:42:17]

NetworkState::PropertyChanged?

[stevenjb, 2012/10/25 22:05:22]

Done.

+    // observer notification).
+    if (key == shill::kIPConfigProperty) {
+      DCHECK(managed->managed_type() == ManagedState::MANAGED_TYPE_NETWORK);
+      std::string ip_config_path;
+      if (properties.GetStringWithoutPathExpansion(key, &ip_config_path)) {
+        DBusThreadManager::Get()->GetShillIPConfigClient()->GetProperties(

[pneubeck (no reviews), 2012/10/25 14:42:17]

This means, that the state can even be inconsistent!
Some properties are already updated and ipconfig may not be.
Please mention that in the class comment.

We should get Shill to send us ipconfig as part of the property dictionary.

Actually, the current shill interface allows only to observe property changes
one-by-one. So, properties that are not encapsulated in a dictionary will
potentially be "inconsistent" (one value already new, the other still old).

We should get Shill to support observing a dictionary of property changes,
similar to a transaction.

[stevenjb, 2012/10/25 22:05:22]

That's always true; shill will send us single property updates. I'll add that to
the header comments.
Agreed. I think there may be a TODO for that already.
When we implement NetworkConfigurationHandler, we will want to gather all state
(i.e. wait for IPConfigs to complete) before returning the complete dictionary
in a callback, so that the result will be consistent (at the time the request
was made).

For most of the UI code however, we want to know what the current known state
is, and receive notifications when that changes.

That said, I realized I was sending the wrong notification when we receive the
IP address.

+            dbus::ObjectPath(ip_config_path),
+            base::Bind(&NetworkStateHandler::GetIPConfigCallback,
+                       weak_ptr_factory_.GetWeakPtr(),
+                       managed->path()));
+        success = true;
+      }
+    } else {
+      const base::Value* value;
+      if (properties.GetWithoutPathExpansion(key, &value))
+        success = managed->PropertyChanged(key, *value);
+    }
+    if (!success)
+      LOG(ERROR) << "Error getting value for key: " << key;
+  }
+}
+
+void NetworkStateHandler::NetworkServicePropertyChanged(
+    const std::string& path,
+    const std::string& key,
+    const base::Value& value) {
+  NetworkState* network = GetModifiableNetworkState(path);
+  if (!network)
+    return;
+  if (network->PropertyChanged(key, value)) {
+    FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                      NetworkServicePropertyChanged(network, key));
+    if (network == network_list_.front() && key == flimflam::kStateProperty) {
+      FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                        ActiveNetworkStateChanged(network));
+    }
+  }
+}
+
+void NetworkStateHandler::GetIPConfigCallback(
+    const std::string& path,

[pneubeck (no reviews), 2012/10/25 14:42:17]

this could be both ipconfig path or service path, please give it an unambiguous
name.

[stevenjb, 2012/10/25 22:05:22]

Done.

+    DBusMethodCallStatus call_status,
+    const base::DictionaryValue& properties)  {
+  if (call_status != DBUS_METHOD_CALL_SUCCESS) {
+    LOG(ERROR) << "Failed to get IP properties for: " << path;
+    return;
+  }
+  bool success = false;
+  for (base::DictionaryValue::key_iterator iter = properties.begin_keys();

[pneubeck (no reviews), 2012/10/25 14:42:17]

DictionaryValue::Iterator

[stevenjb, 2012/10/25 22:05:22]

Actually, no need to iterate, this can just be
properties.GetStringWithoutPathExpansion.

+       iter != properties.end_keys(); ++iter) {
+    const std::string& key = *iter;
+    if (key == flimflam::kAddressProperty) {
+      std::string ip_config;
+      properties.GetStringWithoutPathExpansion(key, &ip_config);
+      NetworkState* network = GetModifiableNetworkState(path);
+      if (network) {
+        network->set_ip_config(ip_config);
+        success = true;
+      }
+      break;
+    }
+  }
+  if (success) {
+    FOR_EACH_OBSERVER(NetworkStateHandlerObserver, observers_,
+                      NetworkManagerChanged(shill::kIPConfigProperty));
+  }
+}
+
+void NetworkStateHandler::RequestScan() const {
+  if (TechnologyEnabled(flimflam::kTypeWifi)) {
+    shill_manager_->RequestScan(flimflam::kTypeWifi,

[pneubeck (no reviews), 2012/10/25 14:42:17]

According to manager-api.txt you can simply use "" instead of a particular
technology:

void RequestScan(string type)
  Request a scan for the specified technology. If
  type is the string "" then a scan request is
  made for each technology.

[stevenjb, 2012/10/25 22:05:22]

Ah, that's new. This was copied from NetworkLibrary. Thanks.

+                                base::Bind(&base::DoNothing),
+                                base::Bind(&ErrorCallbackFunction));
+  }
+  if (TechnologyEnabled(flimflam::kTypeWimax)) {
+    shill_manager_->RequestScan(flimflam::kTypeWimax,
+                                base::Bind(&base::DoNothing),
+                                base::Bind(&ErrorCallbackFunction));
+  }
+}
+
+}  // namespace chromeos