NetworkChangeNotifierChromeos: Handle IPConfig property changes on the default network

chromeos/network/network_state.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.h"
 
 #include "base/values.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
+namespace {
+
+// Helper to convert a ListValue of strings into a comma separated string.
+bool ConvertListValueToString(const base::ListValue& string_list,

[pneubeck (no reviews), 2013/03/28 10:37:54]

You can use this one (move it to base/value_conversions.*):

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/net...

[gauravsh, 2013/03/28 18:48:54]

I don't think this is general and universal enough to belong in base (and highly
doubt the base OWNERS will accept it either).

[stevenjb, 2013/03/28 19:27:13]

I tend to agree that this shouldn't be generalized. Getting back a std::vector<>
of strings (which is what the referenced function does) and converting that to a
comma-separated list wouldn't be any simpler than this.

Also, since cros_network_functions.cc is going away, I'm fine with duplicating
this. If we need it elsewhere in code we intend to maintain, we can add it to a
utility module at that time.

+                              std::string* result) {
+  for (size_t i = 0; i < string_list.GetSize(); ++i) {
+    std::string name_server;
+    if (!string_list.GetString(i, &name_server))
+      return false;
+    if (!result->empty())
+      *result += ",";
+    result->append(name_server);
+  }
+  return true;
+}
+
+}  // namespace
+
 namespace chromeos {
 
 NetworkState::NetworkState(const std::string& path)
     : ManagedState(MANAGED_TYPE_NETWORK, path),
       signal_strength_(0) {
 }
 
 NetworkState::~NetworkState() {
 }
 
 bool NetworkState::PropertyChanged(const std::string& key,
                                    const base::Value& value) {
   // Keep care that these properties are the same as in |GetProperties|.
   if (ManagedStatePropertyChanged(key, value))
     return true;
   if (key == flimflam::kSignalStrengthProperty) {
     return GetIntegerValue(key, value, &signal_strength_);
   } else if (key == flimflam::kStateProperty) {
     return GetStringValue(key, value, &connection_state_);
   } else if (key == flimflam::kErrorProperty) {
     return GetStringValue(key, value, &error_);
+  } else if (key == flimflam::kAddressProperty) {
+    return GetStringValue(key, value, &ip_address_);
+  } else if (key == flimflam::kNameServersProperty) {
+    const base::ListValue* dns_list;
+    if (value.GetAsList(&dns_list) &&
+        ConvertListValueToString(*dns_list, &dns_servers_))
+      return true;
   } else if (key == flimflam::kActivationStateProperty) {
     return GetStringValue(key, value, &activation_state_);
   } else if (key == flimflam::kRoamingStateProperty) {
     return GetStringValue(key, value, &roaming_);
   } else if (key == flimflam::kSecurityProperty) {
     return GetStringValue(key, value, &security_);
   } else if (key == flimflam::kNetworkTechnologyProperty) {
     return GetStringValue(key, value, &technology_);
   } else if (key == flimflam::kDeviceProperty) {
     return GetStringValue(key, value, &device_path_);
@@ skipping 10 matching lines @@
 void NetworkState::GetProperties(base::DictionaryValue* dictionary) const {
   // Keep care that these properties are the same as in |PropertyChanged|.
   dictionary->SetStringWithoutPathExpansion(flimflam::kNameProperty, name());
   dictionary->SetStringWithoutPathExpansion(flimflam::kTypeProperty, type());
   dictionary->SetIntegerWithoutPathExpansion(flimflam::kSignalStrengthProperty,
                                              signal_strength());
   dictionary->SetStringWithoutPathExpansion(flimflam::kStateProperty,
                                             connection_state());
   dictionary->SetStringWithoutPathExpansion(flimflam::kErrorProperty,
                                             error());
+  dictionary->SetStringWithoutPathExpansion(flimflam::kAddressProperty,

[pneubeck (no reviews), 2013/03/28 10:37:54]

Create a nested IPConfig (shill::kIPConfigProperty) dictionary and store the
ipaddress and nameservers in there.

[gauravsh, 2013/03/28 18:48:54]

Wouldn't that be confusing - this isn't the complete set of IP Config
properties. What is the intended purpose of GetProperties?

[stevenjb, 2013/03/28 19:27:13]

For now I think that keeping them separate is fine. If we add additional
IPConfig properties I agree with Philipp; it might be better to more closely
match the Shill model, and to limit the number of top level NetworkState
properties.

+                                            ip_address());
+  dictionary->SetStringWithoutPathExpansion(flimflam::kNameServersProperty,
+                                            dns_servers());
   dictionary->SetStringWithoutPathExpansion(flimflam::kActivationStateProperty,
                                             activation_state());
   dictionary->SetStringWithoutPathExpansion(flimflam::kRoamingStateProperty,
                                             roaming());
   dictionary->SetStringWithoutPathExpansion(flimflam::kSecurityProperty,
                                             security());
   dictionary->SetStringWithoutPathExpansion(
       flimflam::kNetworkTechnologyProperty,
       technology());
   dictionary->SetStringWithoutPathExpansion(flimflam::kDeviceProperty,
@@ skipping 22 matching lines @@
 }
 
 // static
 bool NetworkState::StateIsConnecting(const std::string& connection_state) {
   return (connection_state == flimflam::kStateAssociation ||
           connection_state == flimflam::kStateConfiguration ||
           connection_state == flimflam::kStateCarrier);
 }
 
 }  // namespace chromeos