In ShillClientHelper use a{sv} for property dictionaries

chromeos/dbus/shill_client_helper.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/dbus/shill_client_helper.h"
 
 #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"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
 // Class to hold onto a reference to a ShillClientHelper. This calss
 // is owned by callbacks and released once the callback completes.
 // Note: Only success callbacks hold the reference. If an error callback is
@@ skipping 175 matching lines @@
   std::string error_message;
   if (response) {
     // Error message may contain the error message as string.
     dbus::MessageReader reader(response);
     error_name = response->GetErrorName();
     reader.PopString(&error_message);
   }
   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)
     : proxy_(proxy),
       active_refs_(0),
       weak_ptr_factory_(this) {
 }
 
 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) {
   CHECK(released_callback_.is_null());
   released_callback_ = callback;
 }
 
 void ShillClientHelper::AddPropertyChangedObserver(
     ShillPropertyChangedObserver* observer) {
   if (observer_list_.HasObserver(observer))
@@ skipping 164 matching lines @@
                  base::Owned(new RefHolder(weak_ptr_factory_.GetWeakPtr())),
                  callback,
                  error_callback),
       base::Bind(&OnError,
                  error_callback));
 }
 
 // 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: {
       const base::ListValue* list = NULL;
       value.GetAsList(&list);
       dbus::MessageWriter variant_writer(NULL);
       writer->OpenVariant("as", &variant_writer);
       dbus::MessageWriter array_writer(NULL);
       variant_writer.OpenArray("s", &array_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);
       writer->CloseContainer(&variant_writer);
       break;
     }
     case base::Value::TYPE_BOOLEAN:
     case base::Value::TYPE_INTEGER:
     case base::Value::TYPE_DOUBLE:
     case base::Value::TYPE_STRING:
       dbus::AppendBasicTypeValueDataAsVariant(writer, value);
       break;
     default:
-      DLOG(ERROR) << "Unexpected type " << value.GetType();
+      NET_LOG(ERROR) << "Unexpected value type: " << value.GetType();
   }
 
 }
 
 // static
 void ShillClientHelper::AppendServicePropertiesDictionary(
     dbus::MessageWriter* writer,
     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);
 }
 
 void ShillClientHelper::AddRef() {
   ++active_refs_;
 }
 
 void ShillClientHelper::Release() {
   --active_refs_;
   if (active_refs_ == 0 && !released_callback_.is_null())
     base::ResetAndReturn(&released_callback_).Run(this);  // May delete this
 }
 
 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) {
   if (!observer_list_.might_have_observers())
     return;
 
   dbus::MessageReader reader(signal);
   std::string name;
   if (!reader.PopString(&name))
     return;
   scoped_ptr<base::Value> value(dbus::PopDataAsValue(&reader));
   if (!value.get())
     return;
 
   FOR_EACH_OBSERVER(ShillPropertyChangedObserver, observer_list_,
                     OnPropertyChanged(name, *value));
 }
 
 }  // namespace chromeos