NetworkChangeNotifierChromeos: Handle IPConfig property changes on the default network

chromeos/network/shill_property_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/shill_property_handler.h"
 
 #include "base/bind.h"
 #include "base/format_macros.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
@@ skipping 391 matching lines @@
     NetworkDevicePropertyChangedCallback(path, key, value);
   else
     NOTREACHED();
 }
 
 void ShillPropertyHandler::NetworkServicePropertyChangedCallback(
     const std::string& path,
     const std::string& key,
     const base::Value& value) {
   if (key == shill::kIPConfigProperty) {
-    // Handle IPConfig here and call listener_->UpdateNetworkServiceIPAddress
+    // Request the IPConfig for the network and update network properties
     // when the request completes.
     std::string ip_config_path;
     value.GetAsString(&ip_config_path);
     DCHECK(!ip_config_path.empty());
     DBusThreadManager::Get()->GetShillIPConfigClient()->GetProperties(
         dbus::ObjectPath(ip_config_path),
         base::Bind(&ShillPropertyHandler::GetIPConfigCallback,
                    weak_ptr_factory_.GetWeakPtr(), path));
   } else {
     listener_->UpdateNetworkServiceProperty(path, key, value);
   }
 }
 
 void ShillPropertyHandler::GetIPConfigCallback(
     const std::string& service_path,
     DBusMethodCallStatus call_status,
     const base::DictionaryValue& properties)  {
   if (call_status != DBUS_METHOD_CALL_SUCCESS) {
-    LOG(ERROR) << "Failed to get IP properties for: " << service_path;
+    LOG(ERROR) << "Failed to get IP Config properties for: " << service_path;
     return;
   }
-  std::string ip_address;
-  if (!properties.GetStringWithoutPathExpansion(flimflam::kAddressProperty,
-                                                &ip_address)) {
+  const base::Value* ip_address;
+  if (!properties.GetWithoutPathExpansion(flimflam::kAddressProperty,
+                                          &ip_address)) {
     LOG(ERROR) << "Failed to get IP Address property for: " << service_path;
     return;
   }
-  listener_->UpdateNetworkServiceIPAddress(service_path, ip_address);
+  listener_->UpdateNetworkServiceProperty(service_path,
+                                          flimflam::kAddressProperty,

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

Use the complete nested key, like:
 printf('%s.%s', shill::kIPConfigProperty, flimflam::kAddressProperty)

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

I don't see the gain. Is it because you are worried about name conflicts? Using
the complete nested key just means needing StringPrintfs here and in
network_state.cc.

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

Yes, please do not. NetworkState is a convenience class. We use flimflam/shill
strings so that we do not have to redefine a bunch of similar strings, but it is
not meant to be an exact 1:1 mirror of Shill.Service.

That said, if we do add any more IPConfig properties, we should use a dictionary
ip_config_, and set kAddressProperty and kNameServersProperty in that (and a
separate UpdateNetworkServiceIPConfigProperty() method)

[pneubeck (no reviews), 2013/03/29 19:16:52]

This comment was based on the proposed changes to NetworkState::GetProperties
and that NetworkState::PropertyChanged should be the inverse function of
GetProperties.


Still, I don't see the need for this change of keys. Look at the above
NetworkServicePropertyChangedCallback. It simply forwards the key/value pairs.
Later the pairs are filtered in NetworkState. In NetworkState::PropertyChange,
we know that we receive Shill properties.

With this code you break that property. This implementation deviates from the
Shill format and to understand NetworkState::PropertyChange, I have to read
again more code.

On 2013/03/28 19:27:13, stevenjb (chromium) wrote:

[stevenjb, 2013/03/29 19:41:25]

OK, I thought about it some more, and now agree that the keys we use in
NetworkState::PropertyChanged() and NetworkState::GetProperties() should match
Shill, even though we do not need to store members in NetworkState that way. I
was needlessly conflating the two situations.

So, here lets go ahead and use:
  const std::string kIPConfigAddressProperty =
    std::string(shill::kIPConfigProperty) + '.' + flimflam::kAddressProperty;

for the key here and in NetworkState. You can define it as a const in
NetworkState to ensure that they match.

+                                          *ip_address);
+
+  const base::Value* dns_servers = NULL;
+  if (!properties.GetWithoutPathExpansion(flimflam::kNameServersProperty,
+                                          &dns_servers)) {
+    LOG(ERROR) << "Failed to get Name servers property for: " << service_path;
+    return;
+  }
+  listener_->UpdateNetworkServiceProperty(service_path,

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

ditto

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

See my question above.

+                                          flimflam::kNameServersProperty,
+                                          *dns_servers);
 }
 
 void ShillPropertyHandler::NetworkDevicePropertyChangedCallback(
     const std::string& path,
     const std::string& key,
     const base::Value& value) {
   listener_->UpdateDeviceProperty(path, key, value);
 }
 
 }  // namespace internal
 }  // namespace chromeos