dbus: clear array values before reading from variant

PopArrayFromVariant() appends values to the existing value, rather
than clearing first like I expected, so using this without clear()
first means the property value accumulates all values and never
loses them.

BUG=none
TEST=unit test included

Change-Id: Ie392a89190f4ad8570a905f24b2f446e1f2bed81

R=satorux@chromium.org

Review URL: https://chromiumcodereview.appspot.com/9809001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@128108 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 108 insertions, 11 deletions

diff --git a/dbus/property.cc b/dbus/property.cc
index 0d6e7f0..0abe001 100644
--- a/dbus/property.cc
+++ b/dbus/property.cc
@@ -382,6 +382,7 @@ bool Property<std::vector<std::string> >::PopValueFromReader(
   if (!reader->PopVariant(&variant_reader))
     return false;
 
+  value_.clear();
   return variant_reader.PopArrayOfStrings(&value_);
 }
 
@@ -406,6 +407,7 @@ bool Property<std::vector<ObjectPath> >::PopValueFromReader(
   if (!reader->PopVariant(&variant_reader))
     return false;
 
+  value_.clear();
   return variant_reader.PopArrayOfObjectPaths(&value_);
 }
 
diff --git a/dbus/property.h b/dbus/property.h
index d46bd8d..2d2cc82 100644
--- a/dbus/property.h
+++ b/dbus/property.h
@@ -386,9 +386,10 @@ class Property : public PropertyBase {
       callback.Run(response);
   }
 
-  // Updates the cached property value by popping from |reader| which
-  // should be positioned at the property value, generally of variant
-  // type. Implementation provided by specialization.
+  // Updates the cached property value, replacing any previous value
+  // entirely, by popping from |reader| which should be positioned at the
+  // property value, generally of variant type.
+  // Implementation provided by specialization.
   virtual bool PopValueFromReader(MessageReader* reader);
 
   // Appends the passed |value| to |writer|, generally as a variant type.
diff --git a/dbus/property_unittest.cc b/dbus/property_unittest.cc
index effb751..71ce533 100644
--- a/dbus/property_unittest.cc
+++ b/dbus/property_unittest.cc
@@ -167,6 +167,55 @@ TEST_F(PropertyTest, InitialValues) {
   EXPECT_EQ(dbus::ObjectPath("/TestObjectPath"), objects[0]);
 }
 
+TEST_F(PropertyTest, UpdatedValues) {
+  WaitForGetAll();
+
+  // Update the value of the "Name" property, this value should not change.
+  properties_->name.Get(base::Bind(&PropertyTest::PropertyCallback,
+                                   base::Unretained(this),
+                                   "Name"));
+  WaitForCallback("Name");
+  WaitForUpdates(1);
+
+  EXPECT_EQ(properties_->name.value(), "TestService");
+
+  // Update the value of the "Version" property, this value should be changed.
+  properties_->version.Get(base::Bind(&PropertyTest::PropertyCallback,
+                                      base::Unretained(this),
+                                      "Version"));
+  WaitForCallback("Version");
+  WaitForUpdates(1);
+
+  EXPECT_EQ(properties_->version.value(), 20);
+
+  // Update the value of the "Methods" property, this value should not change
+  // and should not grow to contain duplicate entries.
+  properties_->methods.Get(base::Bind(&PropertyTest::PropertyCallback,
+                                      base::Unretained(this),
+                                      "Methods"));
+  WaitForCallback("Methods");
+  WaitForUpdates(1);
+
+  std::vector<std::string> methods = properties_->methods.value();
+  ASSERT_EQ(4U, methods.size());
+  EXPECT_EQ("Echo", methods[0]);
+  EXPECT_EQ("SlowEcho", methods[1]);
+  EXPECT_EQ("AsyncEcho", methods[2]);
+  EXPECT_EQ("BrokenMethod", methods[3]);
+
+  // Update the value of the "Objects" property, this value should not change
+  // and should not grow to contain duplicate entries.
+  properties_->objects.Get(base::Bind(&PropertyTest::PropertyCallback,
+                                      base::Unretained(this),
+                                      "Objects"));
+  WaitForCallback("Objects");
+  WaitForUpdates(1);
+
+  std::vector<dbus::ObjectPath> objects = properties_->objects.value();
+  ASSERT_EQ(1U, objects.size());
+  EXPECT_EQ(dbus::ObjectPath("/TestObjectPath"), objects[0]);
+}
+
 TEST_F(PropertyTest, Get) {
   WaitForGetAll();
 
diff --git a/dbus/test_service.cc b/dbus/test_service.cc
index 443ad9b..986ebd6 100644
--- a/dbus/test_service.cc
+++ b/dbus/test_service.cc
@@ -336,17 +336,62 @@ void TestService::GetProperty(
     return;
   }
 
-  if (name != "Version") {
+  if (name == "Name") {
+    // Return the previous value for the "Name" property:
+    // Variant<"TestService">
+    Response* response = Response::FromMethodCall(method_call);
+    MessageWriter writer(response);
+
+    writer.AppendVariantOfString("TestService");
+
+    response_sender.Run(response);
+  } else if (name == "Version") {
+    // Return a new value for the "Version" property:
+    // Variant<20>
+    Response* response = Response::FromMethodCall(method_call);
+    MessageWriter writer(response);
+
+    writer.AppendVariantOfInt16(20);
+
+    response_sender.Run(response);
+  } else if (name == "Methods") {
+    // Return the previous value for the "Methods" property:
+    // Variant<["Echo", "SlowEcho", "AsyncEcho", "BrokenMethod"]>
+    Response* response = Response::FromMethodCall(method_call);
+    MessageWriter writer(response);
+    MessageWriter variant_writer(NULL);
+    MessageWriter variant_array_writer(NULL);
+
+    writer.OpenVariant("as", &variant_writer);
+    variant_writer.OpenArray("s", &variant_array_writer);
+    variant_array_writer.AppendString("Echo");
+    variant_array_writer.AppendString("SlowEcho");
+    variant_array_writer.AppendString("AsyncEcho");
+    variant_array_writer.AppendString("BrokenMethod");
+    variant_writer.CloseContainer(&variant_array_writer);
+    writer.CloseContainer(&variant_writer);
+
+    response_sender.Run(response);
+  } else if (name == "Objects") {
+    // Return the previous value for the "Objects" property:
+    // Variant<[objectpath:"/TestObjectPath"]>
+    Response* response = Response::FromMethodCall(method_call);
+    MessageWriter writer(response);
+    MessageWriter variant_writer(NULL);
+    MessageWriter variant_array_writer(NULL);
+
+    writer.OpenVariant("ao", &variant_writer);
+    variant_writer.OpenArray("o", &variant_array_writer);
+    variant_array_writer.AppendObjectPath(dbus::ObjectPath("/TestObjectPath"));
+    variant_writer.CloseContainer(&variant_array_writer);
+    writer.CloseContainer(&variant_writer);
+
+    response_sender.Run(response);
+  } else {
+    // Return error.
     response_sender.Run(NULL);
     return;
   }
-
-  Response* response = Response::FromMethodCall(method_call);
-  MessageWriter writer(response);
-
-  writer.AppendVariantOfInt16(20);
-
-  response_sender.Run(response);
 }
 
 void TestService::SetProperty(