In ShillClientHelper use a{sv} for property dictionaries

chromeos/dbus/shill_client_helper.cc

Index: chromeos/dbus/shill_client_helper.cc
diff --git a/chromeos/dbus/shill_client_helper.cc b/chromeos/dbus/shill_client_helper.cc
index c9271809a87735326f6fe8250bd88d186f4e6cbd..622304a2eca83daa0692bf501b4f85006c13b5ec 100644
--- a/chromeos/dbus/shill_client_helper.cc
+++ b/chromeos/dbus/shill_client_helper.cc
@@ -7,6 +7,7 @@
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/values.h"
+#include "chromeos/device_event_log.h"
 #include "dbus/message.h"
 #include "dbus/object_proxy.h"
 #include "dbus/values_util.h"
@@ -202,6 +203,28 @@ void OnError(const ShillClientHelper::ErrorCallback& error_callback,
   error_callback.Run(error_name, error_message);
 }
 
+void WriteStringDictionary(const base::DictionaryValue& dictionary,
+                           dbus::MessageWriter* writer) {
+  dbus::MessageWriter variant_writer(NULL);
+  writer->OpenVariant("a{ss}", &variant_writer);
+  dbus::MessageWriter array_writer(NULL);
+  variant_writer.OpenArray("{ss}", &array_writer);
+  for (base::DictionaryValue::Iterator it(dictionary); !it.IsAtEnd();
+       it.Advance()) {
+    dbus::MessageWriter entry_writer(NULL);
+    array_writer.OpenDictEntry(&entry_writer);
+    entry_writer.AppendString(it.key());
+    const base::Value& value = it.value();
+    std::string value_string;
+    if (!value.GetAsString(&value_string))
+      NET_LOG(ERROR) << "Dictionary value not a string: " << it.key();
+    entry_writer.AppendString(value_string);
+    array_writer.CloseContainer(&entry_writer);
+  }
+  variant_writer.CloseContainer(&array_writer);
+  writer->CloseContainer(&variant_writer);
+}

[stevenjb, 2015/01/22 23:14:31]

The above is copy/pasted from the original code (lines 405-421)

+
 }  // namespace
 
 ShillClientHelper::ShillClientHelper(dbus::ObjectProxy* proxy)
@@ -211,8 +234,8 @@ ShillClientHelper::ShillClientHelper(dbus::ObjectProxy* proxy)
 }
 
 ShillClientHelper::~ShillClientHelper() {
-  LOG_IF(ERROR, observer_list_.might_have_observers())
-      << "ShillClientHelper destroyed with active observers";
+  if (observer_list_.might_have_observers())
+    NET_LOG(ERROR) << "ShillClientHelper destroyed with active observers";
 }
 
 void ShillClientHelper::SetReleasedCallback(ReleasedCallback callback) {
@@ -397,31 +420,28 @@ void ShillClientHelper::CallListValueMethodWithErrorCallback(
 // static
 void ShillClientHelper::AppendValueDataAsVariant(dbus::MessageWriter* writer,
                                                  const base::Value& value) {
+  AppendValueDataAsVariantEx(writer, value,
+                             false /* use_string_dictionaries */);

[stevenjb, 2015/01/22 23:14:31]

Note: this changes the default behavior, but this is only used for Shill, and
the only writable Shill dictionaries are Cellular.APN (which this is not used
for directly) and StaticIPConfig (which this is also not used for directly but
which uses the new 'default' behavior).

[hashimoto, 2015/01/23 02:53:48]

Isn't it possible to change Shill to accept a{sv} as Cellular.APN to use this
format exclusively?
Automatically switching the message format depending on the key name looks
smart, but might be confusing to understand for newcomers.

[stevenjb, 2015/01/23 18:49:20]

We're looking into that but it's going to be a pretty messy change on the Shill
side and we need an intermediate solution. (Also, Shill would have to support
both versions in the interrum which may be a hughe pain). I'll add a TODO to the
comment.

+}
+
+// static
+void ShillClientHelper::AppendValueDataAsVariantEx(

[hashimoto, 2015/01/23 02:53:48]

"Ex" suffix here doesn't look pretty.
How about adding the use_string_dictionaries argument directly to
AppendValueDataAsVariant(), instead of having 2 methods?
The argument can be replaced with an enum to make the user code readable.

[stevenjb, 2015/01/23 18:49:21]

I don't think it makes sense to change the signature just to support an
implementation change. I removed the new call from the header and renamed the
internal one instead.

+    dbus::MessageWriter* writer,
+    const base::Value& value,
+    bool use_string_dictionaries) {
   // Support basic types and string-to-string dictionary.
   switch (value.GetType()) {
     case base::Value::TYPE_DICTIONARY: {
       const base::DictionaryValue* dictionary = NULL;
       value.GetAsDictionary(&dictionary);
-      dbus::MessageWriter variant_writer(NULL);
-      writer->OpenVariant("a{ss}", &variant_writer);
-      dbus::MessageWriter array_writer(NULL);
-      variant_writer.OpenArray("{ss}", &array_writer);
-      for (base::DictionaryValue::Iterator it(*dictionary);
-           !it.IsAtEnd();
-           it.Advance()) {
-        dbus::MessageWriter entry_writer(NULL);
-        array_writer.OpenDictEntry(&entry_writer);
-        entry_writer.AppendString(it.key());
-        const base::Value& value = it.value();
-        std::string value_string;
-        DLOG_IF(ERROR, value.GetType() != base::Value::TYPE_STRING)
-            << "Unexpected type " << value.GetType();
-        value.GetAsString(&value_string);
-        entry_writer.AppendString(value_string);
-        array_writer.CloseContainer(&entry_writer);
+      if (use_string_dictionaries) {
+        WriteStringDictionary(*dictionary, writer);
+      } else {
+        dbus::MessageWriter variant_writer(NULL);
+        writer->OpenVariant("a{sv}", &variant_writer);
+        AppendServicePropertiesDictionary(&variant_writer, *dictionary);
+        writer->CloseContainer(&variant_writer);
       }
-      variant_writer.CloseContainer(&array_writer);
-      writer->CloseContainer(&variant_writer);
       break;
     }
     case base::Value::TYPE_LIST: {
@@ -434,10 +454,9 @@ void ShillClientHelper::AppendValueDataAsVariant(dbus::MessageWriter* writer,
       for (base::ListValue::const_iterator it = list->begin();
            it != list->end(); ++it) {
         const base::Value& value = **it;
-        LOG_IF(ERROR, value.GetType() != base::Value::TYPE_STRING)
-            << "Unexpected type " << value.GetType();
         std::string value_string;
-        value.GetAsString(&value_string);
+        if (!value.GetAsString(&value_string))
+          NET_LOG(ERROR) << "List value not a string: " << value;
         array_writer.AppendString(value_string);
       }
       variant_writer.CloseContainer(&array_writer);
@@ -451,7 +470,7 @@ void ShillClientHelper::AppendValueDataAsVariant(dbus::MessageWriter* writer,
       dbus::AppendBasicTypeValueDataAsVariant(writer, value);
       break;
     default:
-      DLOG(ERROR) << "Unexpected type " << value.GetType();
+      NET_LOG(ERROR) << "Unexpected value type: " << value.GetType();
   }
 
 }
@@ -462,13 +481,16 @@ void ShillClientHelper::AppendServicePropertiesDictionary(
     const base::DictionaryValue& dictionary) {
   dbus::MessageWriter array_writer(NULL);
   writer->OpenArray("{sv}", &array_writer);
-  for (base::DictionaryValue::Iterator it(dictionary);
-       !it.IsAtEnd();
+  for (base::DictionaryValue::Iterator it(dictionary); !it.IsAtEnd();
        it.Advance()) {
     dbus::MessageWriter entry_writer(NULL);
     array_writer.OpenDictEntry(&entry_writer);
     entry_writer.AppendString(it.key());
-    ShillClientHelper::AppendValueDataAsVariant(&entry_writer, it.value());
+    // Shill expects Cellular.APN to be a string dictionary, a{ss}. All other
+    // properties use a varient dictionary, a{sv}.
+    bool use_string_dictionaries = it.key() == shill::kCellularApnProperty;
+    ShillClientHelper::AppendValueDataAsVariantEx(&entry_writer, it.value(),
+                                                  use_string_dictionaries);
     array_writer.CloseContainer(&entry_writer);
   }
   writer->CloseContainer(&array_writer);
@@ -487,8 +509,8 @@ void ShillClientHelper::Release() {
 void ShillClientHelper::OnSignalConnected(const std::string& interface,
                                           const std::string& signal,
                                           bool success) {
-  LOG_IF(ERROR, !success) << "Connect to " << interface << " " << signal
-                          << " failed.";
+  if (!success)
+    NET_LOG(ERROR) << "Connect to " << interface << " " << signal << " failed.";
 }
 
 void ShillClientHelper::OnPropertyChanged(dbus::Signal* signal) {