Provide IPConfigs in networkingPrivate.GetProperties

chromeos/network/network_configuration_handler.cc

Index: chromeos/network/network_configuration_handler.cc
diff --git a/chromeos/network/network_configuration_handler.cc b/chromeos/network/network_configuration_handler.cc
index 9b2efc9231cecb59dff689dc1c41cba927a9bc04..1a6e1fbfa8e817361cfbd9d8a994caeed7802382 100644
--- a/chromeos/network/network_configuration_handler.cc
+++ b/chromeos/network/network_configuration_handler.cc
@@ -20,6 +20,7 @@
 #include "chromeos/dbus/shill_manager_client.h"
 #include "chromeos/dbus/shill_profile_client.h"
 #include "chromeos/dbus/shill_service_client.h"
+#include "chromeos/network/device_state.h"
 #include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/shill_property_util.h"
@@ -47,33 +48,6 @@ void InvokeErrorCallback(const std::string& service_path,
       error_callback, service_path, error_name, error_msg);
 }
 
-void GetPropertiesCallback(
-    const network_handler::DictionaryResultCallback& callback,
-    const network_handler::ErrorCallback& error_callback,
-    const std::string& service_path,
-    DBusMethodCallStatus call_status,
-    const base::DictionaryValue& properties) {
-  if (call_status != DBUS_METHOD_CALL_SUCCESS) {
-    // Because network services are added and removed frequently, we will see
-    // failures regularly, so don't log these.
-    network_handler::RunErrorCallback(error_callback,
-                                      service_path,
-                                      network_handler::kDBusFailedError,
-                                      network_handler::kDBusFailedErrorMessage);
-    return;
-  }
-  if (callback.is_null())
-    return;
-
-  // Get the correct name from WifiHex if necessary.
-  scoped_ptr<base::DictionaryValue> properties_copy(properties.DeepCopy());
-  std::string name =
-      shill_property_util::GetNameFromProperties(service_path, properties);
-  if (!name.empty())
-    properties_copy->SetStringWithoutPathExpansion(shill::kNameProperty, name);
-  callback.Run(service_path, *properties_copy.get());
-}
-
 void SetNetworkProfileErrorCallback(
     const std::string& service_path,
     const std::string& profile_path,
@@ -208,11 +182,11 @@ class NetworkConfigurationHandler::ProfileEntryDeleter
 void NetworkConfigurationHandler::GetProperties(
     const std::string& service_path,
     const network_handler::DictionaryResultCallback& callback,
-    const network_handler::ErrorCallback& error_callback) const {
+    const network_handler::ErrorCallback& error_callback) {
   DBusThreadManager::Get()->GetShillServiceClient()->GetProperties(
       dbus::ObjectPath(service_path),
-      base::Bind(&GetPropertiesCallback,
-                 callback, error_callback, service_path));
+      base::Bind(&NetworkConfigurationHandler::GetPropertiesCallback,
+                 AsWeakPtr(), callback, error_callback, service_path));
 }
 
 void NetworkConfigurationHandler::SetProperties(
@@ -369,6 +343,53 @@ void NetworkConfigurationHandler::ProfileEntryDeleterCompleted(
   profile_entry_deleters_.erase(iter);
 }
 
+void NetworkConfigurationHandler::GetPropertiesCallback(
+    const network_handler::DictionaryResultCallback& callback,
+    const network_handler::ErrorCallback& error_callback,
+    const std::string& service_path,
+    DBusMethodCallStatus call_status,
+    const base::DictionaryValue& properties) {
+  if (call_status != DBUS_METHOD_CALL_SUCCESS) {
+    // Because network services are added and removed frequently, we will see
+    // failures regularly, so don't log these.
+    network_handler::RunErrorCallback(error_callback,
+                                      service_path,
+                                      network_handler::kDBusFailedError,
+                                      network_handler::kDBusFailedErrorMessage);
+    return;
+  }
+  if (callback.is_null())
+    return;
+
+  // Get the correct name from WifiHex if necessary.
+  scoped_ptr<base::DictionaryValue> properties_copy(properties.DeepCopy());
+  std::string name =
+      shill_property_util::GetNameFromProperties(service_path, properties);
+  if (!name.empty())
+    properties_copy->SetStringWithoutPathExpansion(shill::kNameProperty, name);
+
+  // Get the IPConfig properties from the device and store them in "IPConfigs"
+  // (plural) in the properties dictionary.
+  std::string device;
+  properties_copy->GetStringWithoutPathExpansion(
+      shill::kDeviceProperty, &device);
+  const DeviceState* device_state =
+      network_state_handler_->GetDeviceState(device);

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

Just some thought, not saying that it's really a problem:

If you get the ipconfigs from StateHandler's cache, might this lead to timing
issues?
Namely, if the properties received here are more up-to-date than the device
state, e.g. before and after connecting. This might lead to
ConnectionState=connected but no available ipconfigs.

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

So, I thought about this some, and realized a few things:
1) We shouldn't do this here, we should do it in ManagedNCH.
2) We should only do this when State == Connected
3) Because Shill handles all requests in-order, the only edge case would occur
if the service changed state to Connected in the same frame that GetProperties
was requested. Otherwise Chrome would have already received the IPConfigs
changed signal and requested the IPConfigs, which would have been processed by
the time we got to this code. However, there will always be an edge case because
State may have changed to !Connected by the time we get here, in which case a
fresh request would produce the wrong result. Thus, I think we should stick with
the edge case that is less work :)

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

sg

+  if (device_state) {
+    // Convert IPConfig dictionary to ListValue
+    base::ListValue* ip_configs = new base::ListValue;
+    for (base::DictionaryValue::Iterator iter(device_state->ip_configs());
+         !iter.IsAtEnd(); iter.Advance()) {
+      ip_configs->Append(iter.value().DeepCopy());
+    }
+    properties_copy->SetWithoutPathExpansion(
+        shill::kIPConfigsProperty, ip_configs);

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

do we want to document that Shill doesn't use this property for Services but
only for Devices?

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

Done. (elaborated comment above)

+  } else {
+    NET_LOG_ERROR("GetPropertiesCallback: no device: " + device, service_path);
+  }
+  callback.Run(service_path, *properties_copy.get());

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

.get() shouldn't be necessary

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

Done.

+}
+
 void NetworkConfigurationHandler::SetPropertiesSuccessCallback(
     const std::string& service_path,
     const base::Closure& callback) {