Provide IPConfigs in networkingPrivate.GetProperties

chromeos/network/shill_property_handler.cc

Index: chromeos/network/shill_property_handler.cc
diff --git a/chromeos/network/shill_property_handler.cc b/chromeos/network/shill_property_handler.cc
index 8ae24fbb83c1fccca81a2ca9b653b3f2d13e0bad..0cdea179158aa5a61d4f42982db43ad7df119ccc 100644
--- a/chromeos/network/shill_property_handler.cc
+++ b/chromeos/network/shill_property_handler.cc
@@ -489,16 +489,17 @@ void ShillPropertyHandler::GetPropertiesCallback(
     }
   }
   listener_->UpdateManagedStateProperties(type, path, properties);
-  // Request IPConfig parameters for networks.
-  if (type == ManagedState::MANAGED_TYPE_NETWORK &&
-      properties.HasKey(shill::kIPConfigProperty)) {
-    std::string ip_config_path;
-    if (properties.GetString(shill::kIPConfigProperty, &ip_config_path)) {
-      DBusThreadManager::Get()->GetShillIPConfigClient()->GetProperties(
-          dbus::ObjectPath(ip_config_path),
-          base::Bind(&ShillPropertyHandler::GetIPConfigCallback,
-                     AsWeakPtr(), path));
-    }
+
+  if (type == ManagedState::MANAGED_TYPE_NETWORK) {

[pneubeck (no reviews), 2014/05/05 21:09:28]

you could drop the type conditions (type == ManagedState....) and directly look
for kIPConfig*Property.

[stevenjb, 2014/05/06 01:15:11]

I would rather have the explicit logic.

+    // Request IPConfig properties.
+    const base::Value* value;

[pneubeck (no reviews), 2014/05/05 21:09:28]

initialize with NULL

[stevenjb, 2014/05/06 01:15:11]

Not necessary.

+    if (properties.Get(shill::kIPConfigProperty, &value))

[pneubeck (no reviews), 2014/05/05 21:09:28]

here and below, Get -> GetWithoutPathExpansion

[stevenjb, 2014/05/06 01:15:11]

Done.

+      RequestIPConfig(type, path, *value);
+  } else if (type == ManagedState::MANAGED_TYPE_DEVICE) {
+    // Clear and request IPConfig properties for each entry in IPConfigs.
+    const base::Value* value;

[pneubeck (no reviews), 2014/05/05 21:09:28]

initialize with NULL

[stevenjb, 2014/05/06 01:15:11]

Not necessary.

+    if (properties.Get(shill::kIPConfigsProperty, &value))
+      RequestIPConfigsList(type, path, *value);
   }
 
   // Notify the listener only when all updates for that type have completed.
@@ -518,70 +519,69 @@ void ShillPropertyHandler::PropertyChangedCallback(
     const std::string& path,
     const std::string& key,
     const base::Value& value) {
+  if (type == ManagedState::MANAGED_TYPE_NETWORK &&

[pneubeck (no reviews), 2014/05/05 21:09:28]

again, checking for |type| not really necessary

[stevenjb, 2014/05/06 01:15:11]

I'd rather be explicit.

+      key == shill::kIPConfigProperty) {
+    RequestIPConfig(type, path, value);
+  } else if (type == ManagedState::MANAGED_TYPE_DEVICE &&
+      key == shill::kIPConfigsProperty) {
+    RequestIPConfigsList(type, path, value);
+  }
+
   if (type == ManagedState::MANAGED_TYPE_NETWORK)
-    NetworkServicePropertyChangedCallback(path, key, value);
+    listener_->UpdateNetworkServiceProperty(path, key, value);
   else if (type == ManagedState::MANAGED_TYPE_DEVICE)
-    NetworkDevicePropertyChangedCallback(path, key, value);
+    listener_->UpdateDeviceProperty(path, key, value);
   else
     NOTREACHED();
 }
 
-void ShillPropertyHandler::NetworkServicePropertyChangedCallback(
+void ShillPropertyHandler::RequestIPConfig(
+    ManagedState::ManagedType type,
+    const std::string& path,
+    const base::Value& value) {
+  std::string ip_config_path;
+  value.GetAsString(&ip_config_path);
+  if (ip_config_path.empty())
+    return;
+  DBusThreadManager::Get()->GetShillIPConfigClient()->GetProperties(
+      dbus::ObjectPath(ip_config_path),
+      base::Bind(&ShillPropertyHandler::GetIPConfigCallback,
+                 AsWeakPtr(), type, path, ip_config_path));
+}
+
+void ShillPropertyHandler::RequestIPConfigsList(
+    ManagedState::ManagedType type,
     const std::string& path,
-    const std::string& key,
     const base::Value& value) {
-  if (key == shill::kIPConfigProperty) {
-    // Request the IPConfig for the network and update network properties
-    // when the request completes.
+  const base::ListValue* ip_configs;

[pneubeck (no reviews), 2014/05/05 21:09:28]

initialize with NULL

[stevenjb, 2014/05/06 01:15:11]

Unneeded

+  if (!value.GetAsList(&ip_configs))
+    return;
+  for (size_t i = 0; i < ip_configs->GetSize(); i++) {

[pneubeck (no reviews), 2014/05/05 21:09:28]

better use base::ListValue::const_iterator

[stevenjb, 2014/05/06 01:15:11]

Done.

     std::string ip_config_path;
-    value.GetAsString(&ip_config_path);
-    DCHECK(!ip_config_path.empty());
+    if (!ip_configs->GetString(i, &ip_config_path) || ip_config_path.empty()) {

[pneubeck (no reviews), 2014/05/05 21:09:28]

should call RequestIPConfig

[stevenjb, 2014/05/06 01:15:11]

I don't follow.

[pneubeck (no reviews), 2014/05/06 07:31:17]

for (base::ListValue::const_iterator iter...)
  RequestIPConfig(type, path, **iter);

to keep the NET_LOG_ERROR, you could move it to RequestIPConfig

[stevenjb, 2014/05/06 16:56:19]

Oh, I see. I guess that's a little more clear. Done.

+      NET_LOG_ERROR("Missing or empty IPConfig in list", path);
+      continue;
+    }
     DBusThreadManager::Get()->GetShillIPConfigClient()->GetProperties(
         dbus::ObjectPath(ip_config_path),
         base::Bind(&ShillPropertyHandler::GetIPConfigCallback,
-                   AsWeakPtr(), path));
-  } else {
-    listener_->UpdateNetworkServiceProperty(path, key, value);
+                   AsWeakPtr(), type, path, ip_config_path));
   }
 }
 
 void ShillPropertyHandler::GetIPConfigCallback(
-    const std::string& service_path,
+    ManagedState::ManagedType type,
+    const std::string& path,
+    const std::string& ip_config_path,
     DBusMethodCallStatus call_status,
     const base::DictionaryValue& properties)  {
   if (call_status != DBUS_METHOD_CALL_SUCCESS) {
     NET_LOG_ERROR("Failed to get IP Config properties",
-                  base::StringPrintf("%s: %d",
-                                     service_path.c_str(), call_status));
-    return;
-  }
-  UpdateIPConfigProperty(service_path, properties, shill::kAddressProperty);
-  UpdateIPConfigProperty(service_path, properties, shill::kNameServersProperty);
-  UpdateIPConfigProperty(service_path, properties, shill::kPrefixlenProperty);
-  UpdateIPConfigProperty(service_path, properties, shill::kGatewayProperty);
-  UpdateIPConfigProperty(service_path, properties,
-                         shill::kWebProxyAutoDiscoveryUrlProperty);
-}
-
-void ShillPropertyHandler::UpdateIPConfigProperty(
-    const std::string& service_path,
-    const base::DictionaryValue& properties,
-    const char* property) {
-  const base::Value* value;
-  if (!properties.GetWithoutPathExpansion(property, &value)) {
-    LOG(ERROR) << "Failed to get IPConfig property: " << property
-               << ", for: " << service_path;
+                  base::StringPrintf("%s: %d", path.c_str(), call_status));
     return;
   }
-  listener_->UpdateNetworkServiceProperty(
-      service_path, NetworkState::IPConfigProperty(property), *value);
-}
-
-void ShillPropertyHandler::NetworkDevicePropertyChangedCallback(
-    const std::string& path,
-    const std::string& key,
-    const base::Value& value) {
-  listener_->UpdateDeviceProperty(path, key, value);
+  NET_LOG_EVENT("IP Config properties received", path);
+  listener_->UpdateIPConfigProperites(type, path, ip_config_path, properties);
 }
 
 }  // namespace internal