7 files changed, 317 insertions, 81 deletions

diff --git a/chromeos/dbus/nfc_client_helpers.cc b/chromeos/dbus/nfc_client_helpers.cc
index b99502b..1469d26 100644
--- a/chromeos/dbus/nfc_client_helpers.cc
+++ b/chromeos/dbus/nfc_client_helpers.cc
@@ -34,55 +34,6 @@ void OnError(const ErrorCallback& error_callback,
   error_callback.Run(error_name, error_message);
 }
 
-void AppendValueDataAsVariant(dbus::MessageWriter* writer,
-                              const base::Value& value) {
-  switch (value.GetType()) {
-    case base::Value::TYPE_DICTIONARY: {
-      const base::DictionaryValue* dictionary = NULL;
-      value.GetAsDictionary(&dictionary);
-      dbus::MessageWriter variant_writer(NULL);
-      dbus::MessageWriter array_writer(NULL);
-      writer->OpenVariant("a{sv}", &variant_writer);
-      variant_writer.OpenArray("{sv}", &array_writer);
-      for (base::DictionaryValue::Iterator iter(*dictionary);
-           !iter.IsAtEnd(); iter.Advance()) {
-        dbus::MessageWriter entry_writer(NULL);
-        array_writer.OpenDictEntry(&entry_writer);
-        entry_writer.AppendString(iter.key());
-        AppendValueDataAsVariant(&entry_writer, iter.value());
-        array_writer.CloseContainer(&entry_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);
-      dbus::MessageWriter array_writer(NULL);
-      writer->OpenVariant("av", &variant_writer);
-      variant_writer.OpenArray("v", &array_writer);
-      for (base::ListValue::const_iterator iter = list->begin();
-           iter != list->end(); ++iter) {
-        const base::Value* value = *iter;
-        AppendValueDataAsVariant(&array_writer, *value);
-      }
-      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();
-  }
-}
-
 DBusObjectMap::DBusObjectMap(const std::string& service_name,
                              Delegate* delegate,
                              dbus::Bus* bus)
diff --git a/chromeos/dbus/nfc_client_helpers.h b/chromeos/dbus/nfc_client_helpers.h
index be38a72..d50db07 100644
--- a/chromeos/dbus/nfc_client_helpers.h
+++ b/chromeos/dbus/nfc_client_helpers.h
@@ -45,12 +45,6 @@ CHROMEOS_EXPORT void OnSuccess(const base::Closure& callback,
 CHROMEOS_EXPORT void OnError(const ErrorCallback& error_callback,
                              dbus::ErrorResponse* response);
 
-// Appends any value (basic types and nested types) represented by |value| to
-// the writer |writer| as a variant type.
-// TODO(armansito): Consider moving this to dbus/values_util.h"
-CHROMEOS_EXPORT void AppendValueDataAsVariant(dbus::MessageWriter* writer,
-                                              const base::Value& value);
-
 // DBusObjectMap is a simple data structure that facilitates keeping track of
 // D-Bus object proxies and properties. It maintains a mapping from object
 // paths to object proxy - property structure pairs.
diff --git a/chromeos/dbus/nfc_device_client.cc b/chromeos/dbus/nfc_device_client.cc
index e9833c9..a1976c8 100644
--- a/chromeos/dbus/nfc_device_client.cc
+++ b/chromeos/dbus/nfc_device_client.cc
@@ -11,6 +11,7 @@
 #include "chromeos/dbus/nfc_adapter_client.h"
 #include "dbus/bus.h"
 #include "dbus/message.h"
+#include "dbus/values_util.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 using chromeos::nfc_client_helpers::DBusObjectMap;
@@ -108,18 +109,7 @@ class NfcDeviceClientImpl : public NfcDeviceClient,
     dbus::MethodCall method_call(nfc_device::kNfcDeviceInterface,
                                  nfc_device::kPush);
     dbus::MessageWriter writer(&method_call);
-    dbus::MessageWriter array_writer(NULL);
-    dbus::MessageWriter dict_entry_writer(NULL);
-    writer.OpenArray("{sv}", &array_writer);
-    for (base::DictionaryValue::Iterator iter(attributes);
-         !iter.IsAtEnd(); iter.Advance()) {
-      array_writer.OpenDictEntry(&dict_entry_writer);
-      dict_entry_writer.AppendString(iter.key());
-      nfc_client_helpers::AppendValueDataAsVariant(&dict_entry_writer,
-                                                   iter.value());
-      array_writer.CloseContainer(&dict_entry_writer);
-    }
-    writer.CloseContainer(&array_writer);
+    dbus::AppendValueData(&writer, attributes);
 
     object_proxy->CallMethodWithErrorCallback(
         &method_call,
diff --git a/chromeos/dbus/nfc_tag_client.cc b/chromeos/dbus/nfc_tag_client.cc
index 6c439fa..eae3ef9 100644
--- a/chromeos/dbus/nfc_tag_client.cc
+++ b/chromeos/dbus/nfc_tag_client.cc
@@ -11,6 +11,7 @@
 #include "chromeos/dbus/nfc_adapter_client.h"
 #include "dbus/bus.h"
 #include "dbus/message.h"
+#include "dbus/values_util.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 using chromeos::nfc_client_helpers::DBusObjectMap;
@@ -109,18 +110,7 @@ class NfcTagClientImpl : public NfcTagClient,
     // Create the arguments.
     dbus::MethodCall method_call(nfc_tag::kNfcTagInterface, nfc_tag::kWrite);
     dbus::MessageWriter writer(&method_call);
-    dbus::MessageWriter array_writer(NULL);
-    dbus::MessageWriter dict_entry_writer(NULL);
-    writer.OpenArray("{sv}", &array_writer);
-    for (base::DictionaryValue::Iterator iter(attributes);
-         !iter.IsAtEnd(); iter.Advance()) {
-      array_writer.OpenDictEntry(&dict_entry_writer);
-      dict_entry_writer.AppendString(iter.key());
-      nfc_client_helpers::AppendValueDataAsVariant(&dict_entry_writer,
-                                                   iter.value());
-      array_writer.CloseContainer(&dict_entry_writer);
-    }
-    writer.CloseContainer(&array_writer);
+    dbus::AppendValueData(&writer, attributes);
 
     object_proxy->CallMethodWithErrorCallback(
         &method_call,
diff --git a/dbus/values_util.cc b/dbus/values_util.cc
index 7574f39..c27c272 100644
--- a/dbus/values_util.cc
+++ b/dbus/values_util.cc
@@ -77,6 +77,8 @@ std::string GetTypeSignature(const base::Value& value) {
       return "ay";
     case base::Value::TYPE_DICTIONARY:
       return "a{sv}";
+    case base::Value::TYPE_LIST:
+      return "av";
     default:
       DLOG(ERROR) << "Unexpected type " << value.GetType();
       return std::string();
@@ -253,4 +255,53 @@ void AppendBasicTypeValueDataAsVariant(MessageWriter* writer,
   writer->CloseContainer(&sub_writer);
 }
 
+void AppendValueData(MessageWriter* writer, const base::Value& value) {
+  switch (value.GetType()) {
+    case base::Value::TYPE_DICTIONARY: {
+      const base::DictionaryValue* dictionary = NULL;
+      value.GetAsDictionary(&dictionary);
+      dbus::MessageWriter array_writer(NULL);
+      writer->OpenArray("{sv}", &array_writer);
+      for (base::DictionaryValue::Iterator iter(*dictionary);
+           !iter.IsAtEnd(); iter.Advance()) {
+        dbus::MessageWriter dict_entry_writer(NULL);
+        array_writer.OpenDictEntry(&dict_entry_writer);
+        dict_entry_writer.AppendString(iter.key());
+        AppendValueDataAsVariant(&dict_entry_writer, iter.value());
+        array_writer.CloseContainer(&dict_entry_writer);
+      }
+      writer->CloseContainer(&array_writer);
+      break;
+    }
+    case base::Value::TYPE_LIST: {
+      const base::ListValue* list = NULL;
+      value.GetAsList(&list);
+      dbus::MessageWriter array_writer(NULL);
+      writer->OpenArray("v", &array_writer);
+      for (base::ListValue::const_iterator iter = list->begin();
+           iter != list->end(); ++iter) {
+        const base::Value* value = *iter;
+        AppendValueDataAsVariant(&array_writer, *value);
+      }
+      writer->CloseContainer(&array_writer);
+      break;
+    }
+    case base::Value::TYPE_BOOLEAN:
+    case base::Value::TYPE_INTEGER:
+    case base::Value::TYPE_DOUBLE:
+    case base::Value::TYPE_STRING:
+      AppendBasicTypeValueData(writer, value);
+      break;
+    default:
+      DLOG(ERROR) << "Unexpected type: " << value.GetType();
+  }
+}
+
+void AppendValueDataAsVariant(MessageWriter* writer, const base::Value& value) {
+  MessageWriter variant_writer(NULL);
+  writer->OpenVariant(GetTypeSignature(value), &variant_writer);
+  AppendValueData(&variant_writer, value);
+  writer->CloseContainer(&variant_writer);
+}
+
 }  // namespace dbus
diff --git a/dbus/values_util.h b/dbus/values_util.h
index e34c7a0..9ece9b9 100644
--- a/dbus/values_util.h
+++ b/dbus/values_util.h
@@ -22,15 +22,37 @@ class MessageWriter;
 // double.  Non-string diciontary keys are converted to strings.
 CHROME_DBUS_EXPORT base::Value* PopDataAsValue(MessageReader* reader);
 
-// Appends a basic type value to |writer|.
+// Appends a basic type value to |writer|. Basic types are BOOLEAN, INTEGER,
+// DOUBLE, and STRING. Use this function for values that are known to be basic
+// types and to handle basic type members of collections that should not
+// have type "a{sv}" or "av". Otherwise, use AppendValueData.
 CHROME_DBUS_EXPORT void AppendBasicTypeValueData(MessageWriter* writer,
                                                  const base::Value& value);
 
-// Appends a basic type value to |writer| as a variant.
+// Appends a basic type value to |writer| as a variant. Basic types are BOOLEAN,
+// INTEGER, DOUBLE, and STRING. Use this function for values that are known to
+// be basic types and to handle basic type members of collections that should
+// not have type "a{sv}" or "av". Otherwise, use AppendValueDataAsVariant.
 CHROME_DBUS_EXPORT void AppendBasicTypeValueDataAsVariant(
     MessageWriter* writer,
     const base::Value& value);
 
+// Appends a value to |writer|. Value can be a basic type, as well as a
+// collection type, such as dictionary or list. Collections will be recursively
+// written as variant containers, i.e. dictionaries will be written with type
+// a{sv} and lists with type av. Any sub-dictionaries or sub-lists will also
+// have these types.
+CHROME_DBUS_EXPORT void AppendValueData(MessageWriter* writer,
+                                        const base::Value& value);
+
+// Appends a value to |writer| as a variant. Value can be a basic type, as well
+// as a collection type, such as dictionary or list. Collections will be
+// recursively written as variant containers, i.e. dictionaries will be written
+// with type a{sv} and lists with type av. Any sub-dictionaries or sub-lists
+// will also have these types.
+CHROME_DBUS_EXPORT void AppendValueDataAsVariant(MessageWriter* writer,
+                                                 const base::Value& value);
+
 }  // namespace dbus
 
 #endif  // DBUS_VALUES_UTIL_H_
diff --git a/dbus/values_util_unittest.cc b/dbus/values_util_unittest.cc
index 2de12d7..336d771 100644
--- a/dbus/values_util_unittest.cc
+++ b/dbus/values_util_unittest.cc
@@ -449,4 +449,242 @@ TEST(ValuesUtilTest, AppendBasicTypesAsVariant) {
   EXPECT_TRUE(value->Equals(&kStringValue));
 }
 
+TEST(ValuesUtilTest, AppendValueDataBasicTypes) {
+  const base::FundamentalValue kBoolValue(false);
+  const base::FundamentalValue kIntegerValue(42);
+  const base::FundamentalValue kDoubleValue(4.2);
+  const base::StringValue kStringValue("string");
+
+  scoped_ptr<Response> response(Response::CreateEmpty());
+  MessageWriter writer(response.get());
+  AppendValueData(&writer, kBoolValue);
+  AppendValueData(&writer, kIntegerValue);
+  AppendValueData(&writer, kDoubleValue);
+  AppendValueData(&writer, kStringValue);
+
+  MessageReader reader(response.get());
+  scoped_ptr<base::Value> value;
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&kBoolValue));
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&kIntegerValue));
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&kDoubleValue));
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&kStringValue));
+}
+
+TEST(ValuesUtilTest, AppendValueDataAsVariantBasicTypes) {
+  const base::FundamentalValue kBoolValue(false);
+  const base::FundamentalValue kIntegerValue(42);
+  const base::FundamentalValue kDoubleValue(4.2);
+  const base::StringValue kStringValue("string");
+
+  scoped_ptr<Response> response(Response::CreateEmpty());
+  MessageWriter writer(response.get());
+  AppendValueDataAsVariant(&writer, kBoolValue);
+  AppendValueDataAsVariant(&writer, kIntegerValue);
+  AppendValueDataAsVariant(&writer, kDoubleValue);
+  AppendValueDataAsVariant(&writer, kStringValue);
+
+  MessageReader reader(response.get());
+  scoped_ptr<base::Value> value;
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&kBoolValue));
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&kIntegerValue));
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&kDoubleValue));
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&kStringValue));
+}
+
+TEST(ValuesUtilTest, AppendDictionary) {
+  // Set up the input dictionary.
+  const std::string kKey1 = "one";
+  const std::string kKey2 = "two";
+  const std::string kKey3 = "three";
+  const std::string kKey4 = "four";
+  const std::string kKey5 = "five";
+  const std::string kKey6 = "six";
+
+  const bool kBoolValue = true;
+  const int32 kInt32Value = -45;
+  const double kDoubleValue = 4.9;
+  const std::string kStringValue = "fifty";
+
+  base::ListValue* list_value = new base::ListValue();
+  list_value->AppendBoolean(kBoolValue);
+  list_value->AppendInteger(kInt32Value);
+
+  base::DictionaryValue* dictionary_value = new base::DictionaryValue();
+  dictionary_value->SetBoolean(kKey1, kBoolValue);
+  dictionary_value->SetInteger(kKey2, kDoubleValue);
+
+  base::DictionaryValue test_dictionary;
+  test_dictionary.SetBoolean(kKey1, kBoolValue);
+  test_dictionary.SetInteger(kKey2, kInt32Value);
+  test_dictionary.SetDouble(kKey3, kDoubleValue);
+  test_dictionary.SetString(kKey4, kStringValue);
+  test_dictionary.Set(kKey5, list_value);  // takes ownership
+  test_dictionary.Set(kKey6, dictionary_value);  // takes ownership
+
+  scoped_ptr<Response> response(Response::CreateEmpty());
+  MessageWriter writer(response.get());
+  AppendValueData(&writer, test_dictionary);
+  base::FundamentalValue int_value(kInt32Value);
+  AppendValueData(&writer, int_value);
+
+  // Read the data.
+  MessageReader reader(response.get());
+  scoped_ptr<base::Value> value;
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&test_dictionary));
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&int_value));
+}
+
+TEST(ValuesUtilTest, AppendDictionaryAsVariant) {
+  // Set up the input dictionary.
+  const std::string kKey1 = "one";
+  const std::string kKey2 = "two";
+  const std::string kKey3 = "three";
+  const std::string kKey4 = "four";
+  const std::string kKey5 = "five";
+  const std::string kKey6 = "six";
+
+  const bool kBoolValue = true;
+  const int32 kInt32Value = -45;
+  const double kDoubleValue = 4.9;
+  const std::string kStringValue = "fifty";
+
+  base::ListValue* list_value = new base::ListValue();
+  list_value->AppendBoolean(kBoolValue);
+  list_value->AppendInteger(kInt32Value);
+
+  base::DictionaryValue* dictionary_value = new base::DictionaryValue();
+  dictionary_value->SetBoolean(kKey1, kBoolValue);
+  dictionary_value->SetInteger(kKey2, kDoubleValue);
+
+  base::DictionaryValue test_dictionary;
+  test_dictionary.SetBoolean(kKey1, kBoolValue);
+  test_dictionary.SetInteger(kKey2, kInt32Value);
+  test_dictionary.SetDouble(kKey3, kDoubleValue);
+  test_dictionary.SetString(kKey4, kStringValue);
+  test_dictionary.Set(kKey5, list_value);  // takes ownership
+  test_dictionary.Set(kKey6, dictionary_value);  // takes ownership
+
+  scoped_ptr<Response> response(Response::CreateEmpty());
+  MessageWriter writer(response.get());
+  AppendValueDataAsVariant(&writer, test_dictionary);
+  base::FundamentalValue int_value(kInt32Value);
+  AppendValueData(&writer, int_value);
+
+  // Read the data.
+  MessageReader reader(response.get());
+  scoped_ptr<base::Value> value;
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&test_dictionary));
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&int_value));
+}
+
+TEST(ValuesUtilTest, AppendList) {
+  // Set up the input list.
+  const std::string kKey1 = "one";
+  const std::string kKey2 = "two";
+
+  const bool kBoolValue = true;
+  const int32 kInt32Value = -45;
+  const double kDoubleValue = 4.9;
+  const std::string kStringValue = "fifty";
+
+  base::ListValue* list_value = new base::ListValue();
+  list_value->AppendBoolean(kBoolValue);
+  list_value->AppendInteger(kInt32Value);
+
+  base::DictionaryValue* dictionary_value = new base::DictionaryValue();
+  dictionary_value->SetBoolean(kKey1, kBoolValue);
+  dictionary_value->SetInteger(kKey2, kDoubleValue);
+
+  base::ListValue test_list;
+  test_list.AppendBoolean(kBoolValue);
+  test_list.AppendInteger(kInt32Value);
+  test_list.AppendDouble(kDoubleValue);
+  test_list.AppendString(kStringValue);
+  test_list.Append(list_value);  // takes ownership
+  test_list.Append(dictionary_value);  // takes ownership
+
+  scoped_ptr<Response> response(Response::CreateEmpty());
+  MessageWriter writer(response.get());
+  AppendValueData(&writer, test_list);
+  base::FundamentalValue int_value(kInt32Value);
+  AppendValueData(&writer, int_value);
+
+  // Read the data.
+  MessageReader reader(response.get());
+  scoped_ptr<base::Value> value;
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&test_list));
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&int_value));
+}
+
+TEST(ValuesUtilTest, AppendListAsVariant) {
+  // Set up the input list.
+  const std::string kKey1 = "one";
+  const std::string kKey2 = "two";
+
+  const bool kBoolValue = true;
+  const int32 kInt32Value = -45;
+  const double kDoubleValue = 4.9;
+  const std::string kStringValue = "fifty";
+
+  base::ListValue* list_value = new base::ListValue();
+  list_value->AppendBoolean(kBoolValue);
+  list_value->AppendInteger(kInt32Value);
+
+  base::DictionaryValue* dictionary_value = new base::DictionaryValue();
+  dictionary_value->SetBoolean(kKey1, kBoolValue);
+  dictionary_value->SetInteger(kKey2, kDoubleValue);
+
+  base::ListValue test_list;
+  test_list.AppendBoolean(kBoolValue);
+  test_list.AppendInteger(kInt32Value);
+  test_list.AppendDouble(kDoubleValue);
+  test_list.AppendString(kStringValue);
+  test_list.Append(list_value);  // takes ownership
+  test_list.Append(dictionary_value);  // takes ownership
+
+  scoped_ptr<Response> response(Response::CreateEmpty());
+  MessageWriter writer(response.get());
+  AppendValueDataAsVariant(&writer, test_list);
+  base::FundamentalValue int_value(kInt32Value);
+  AppendValueData(&writer, int_value);
+
+  // Read the data.
+  MessageReader reader(response.get());
+  scoped_ptr<base::Value> value;
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&test_list));
+  value.reset(PopDataAsValue(&reader));
+  ASSERT_TRUE(value.get() != NULL);
+  EXPECT_TRUE(value->Equals(&int_value));
+}
+
 }  // namespace dbus