Implement invalidation of Dbus properties

BUG=481247
R=armansito@chromium.org

Review URL: https://codereview.chromium.org/1086363004

Cr-Commit-Position: refs/heads/master@{#329142}

5 files changed, 142 insertions, 9 deletions

diff --git a/dbus/property.cc b/dbus/property.cc
index 0e912f3..66b86c0 100644
--- a/dbus/property.cc
+++ b/dbus/property.cc
@@ -21,10 +21,10 @@ namespace dbus {
 void PropertyBase::Init(PropertySet* property_set, const std::string& name) {
   DCHECK(!property_set_);
   property_set_ = property_set;
+  is_valid_ = false;
   name_ = name;
 }
 
-
 //
 // PropertySet implementation.
 //
@@ -78,10 +78,10 @@ void PropertySet::ChangedReceived(Signal* signal) {
                  << "expected dictionary: " << signal->ToString();
   }
 
-  // TODO(keybuk): dbus properties api has invalidated properties array
-  // on the end, we don't handle this right now because I don't know of
-  // any service that sends it - or what they expect us to do with it.
-  // Add later when we need it.
+  if (!InvalidatePropertiesFromReader(&reader)) {
+    LOG(WARNING) << "Property changed signal has wrong parameters: "
+                 << "expected array to invalidate: " << signal->ToString();
+  }
 }
 
 void PropertySet::ChangedConnected(const std::string& interface_name,
@@ -115,8 +115,15 @@ void PropertySet::OnGet(PropertyBase* property, GetCallback callback,
   }
 
   MessageReader reader(response);
-  if (property->PopValueFromReader(&reader))
+  if (property->PopValueFromReader(&reader)) {
+    property->set_valid(true);
     NotifyPropertyChanged(property->name());
+  } else {
+    if (property->is_valid()) {
+      property->set_valid(false);
+      NotifyPropertyChanged(property->name());
+    }
+  }
 
   if (!callback.is_null())
     callback.Run(response);
@@ -199,13 +206,42 @@ bool PropertySet::UpdatePropertyFromReader(MessageReader* reader) {
 
   PropertyBase* property = it->second;
   if (property->PopValueFromReader(reader)) {
+    property->set_valid(true);
     NotifyPropertyChanged(name);
     return true;
   } else {
+    if (property->is_valid()) {
+      property->set_valid(false);
+      NotifyPropertyChanged(property->name());
+    }
     return false;
   }
 }
 
+bool PropertySet::InvalidatePropertiesFromReader(MessageReader* reader) {
+  DCHECK(reader);
+  MessageReader array_reader(NULL);
+  if (!reader->PopArray(&array_reader))
+    return false;
+
+  while (array_reader.HasMoreData()) {
+    std::string name;
+    if (!array_reader.PopString(&name))
+      return false;
+
+    PropertiesMap::iterator it = properties_map_.find(name);
+    if (it == properties_map_.end())
+      continue;
+
+    PropertyBase* property = it->second;
+    if (property->is_valid()) {
+      property->set_valid(false);
+      NotifyPropertyChanged(property->name());
+    }
+  }
+
+  return true;
+}
 
 void PropertySet::NotifyPropertyChanged(const std::string& name) {
   if (!property_changed_callback_.is_null())
diff --git a/dbus/property.h b/dbus/property.h
index 419da53..321f4dbd 100644
--- a/dbus/property.h
+++ b/dbus/property.h
@@ -134,7 +134,7 @@ class PropertySet;
 // used by PropertySet.
 class PropertyBase {
  public:
-  PropertyBase() : property_set_(NULL) {}
+  PropertyBase() : property_set_(nullptr), is_valid_(false) {}
 
   // Initializes the |property_set| and property |name| so that method
   // calls may be made from this class. This method is called by
@@ -156,6 +156,12 @@ class PropertyBase {
   //   }
   const std::string& name() const { return name_; }
 
+  // Returns true if property is valid, false otherwise.
+  bool is_valid() const { return is_valid_; }
+
+  // Allows to mark Property as valid or invalid.
+  void set_valid(bool is_valid) { is_valid_ = is_valid; }
+
   // Method used by PropertySet to retrieve the value from a MessageReader,
   // no knowledge of the contained type is required, this method returns
   // true if its expected type was found, false if not.
@@ -181,6 +187,8 @@ class PropertyBase {
   // no ownership is taken and |property_set_| must outlive this class.
   PropertySet* property_set_;
 
+  bool is_valid_;
+
   // Name of the property.
   std::string name_;
 
@@ -302,6 +310,10 @@ class CHROME_DBUS_EXPORT PropertySet {
   }
 
  private:
+  // Invalidates properties by reading an array of names, from
+  // |message_reader|. Returns false if message is in incorrect format.
+  bool InvalidatePropertiesFromReader(MessageReader* reader);
+
   // Pointer to object proxy for making method calls, no ownership is taken
   // so this must outlive this class.
   ObjectProxy* object_proxy_;
diff --git a/dbus/property_unittest.cc b/dbus/property_unittest.cc
index c7df1ce..5208f84 100644
--- a/dbus/property_unittest.cc
+++ b/dbus/property_unittest.cc
@@ -27,8 +27,7 @@ namespace dbus {
 // Property<>.
 class PropertyTest : public testing::Test {
  public:
-  PropertyTest() {
-  }
+  PropertyTest() {}
 
   struct Properties : public PropertySet {
     Property<std::string> name;
@@ -110,6 +109,10 @@ class PropertyTest : public testing::Test {
     run_loop_->Quit();
   }
 
+  // Generic method callback, that might be used together with
+  // WaitForMethodCallback to test wether method was succesfully called.
+  void MethodCallback(Response* response) { run_loop_->Quit(); }
+
  protected:
   // Called when a property value is updated.
   void OnPropertyChanged(const std::string& name) {
@@ -135,6 +138,12 @@ class PropertyTest : public testing::Test {
     WaitForUpdates(kExpectedSignalUpdates);
   }
 
+  // Waits until MethodCallback is called.
+  void WaitForMethodCallback() {
+    run_loop_.reset(new base::RunLoop);
+    run_loop_->Run();
+  }
+
   // Waits for the callback. |id| is the string bound to the callback when
   // the method call is made that identifies it and distinguishes from any
   // other; you can set this to whatever you wish.
@@ -159,9 +168,14 @@ class PropertyTest : public testing::Test {
 };
 
 TEST_F(PropertyTest, InitialValues) {
+  EXPECT_FALSE(properties_->name.is_valid());
+  EXPECT_FALSE(properties_->version.is_valid());
+
   WaitForGetAll();
 
+  EXPECT_TRUE(properties_->name.is_valid());
   EXPECT_EQ("TestService", properties_->name.value());
+  EXPECT_TRUE(properties_->version.is_valid());
   EXPECT_EQ(10, properties_->version.value());
 
   std::vector<std::string> methods = properties_->methods.value();
@@ -278,6 +292,38 @@ TEST_F(PropertyTest, Set) {
   EXPECT_EQ("NewService", properties_->name.value());
 }
 
+TEST_F(PropertyTest, Invalidate) {
+  WaitForGetAll();
+
+  EXPECT_TRUE(properties_->name.is_valid());
+
+  // Invalidate name.
+  MethodCall method_call("org.chromium.TestInterface", "PerformAction");
+  MessageWriter writer(&method_call);
+  writer.AppendString("InvalidateProperty");
+  writer.AppendObjectPath(ObjectPath("/org/chromium/TestService"));
+  object_proxy_->CallMethod(
+      &method_call, ObjectProxy::TIMEOUT_USE_DEFAULT,
+      base::Bind(&PropertyTest::MethodCallback, base::Unretained(this)));
+  WaitForMethodCallback();
+
+  // TestService sends a property update.
+  WaitForUpdates(1);
+
+  EXPECT_FALSE(properties_->name.is_valid());
+
+  // Set name to something valid.
+  properties_->name.Set("NewService",
+                        base::Bind(&PropertyTest::PropertyCallback,
+                                   base::Unretained(this), "Set"));
+  WaitForCallback("Set");
+
+  // TestService sends a property update.
+  WaitForUpdates(1);
+
+  EXPECT_TRUE(properties_->name.is_valid());
+}
+
 TEST(PropertyTestStatic, ReadWriteStringMap) {
   scoped_ptr<Response> message(Response::CreateEmpty());
   MessageWriter writer(message.get());
diff --git a/dbus/test_service.cc b/dbus/test_service.cc
index 0170192..2c14869 100644
--- a/dbus/test_service.cc
+++ b/dbus/test_service.cc
@@ -498,6 +498,8 @@ void TestService::PerformAction(
                                 base::Unretained(this),
                                 method_call, response_sender));
     return;
+  } else if (action == "InvalidateProperty") {
+    SendPropertyInvalidatedSignal();
   }
 
   scoped_ptr<Response> response = Response::FromMethodCall(method_call);
@@ -705,6 +707,37 @@ void TestService::SendPropertyChangedSignalInternal(const std::string& name) {
   array_writer.CloseContainer(&dict_entry_writer);
   writer.CloseContainer(&array_writer);
 
+  MessageWriter invalidated_array_writer(NULL);
+
+  writer.OpenArray("s", &invalidated_array_writer);
+  writer.CloseContainer(&invalidated_array_writer);
+
+  exported_object_->SendSignal(&signal);
+}
+
+void TestService::SendPropertyInvalidatedSignal() {
+  message_loop()->PostTask(
+      FROM_HERE, base::Bind(&TestService::SendPropertyInvalidatedSignalInternal,
+                            base::Unretained(this)));
+}
+
+void TestService::SendPropertyInvalidatedSignalInternal() {
+  Signal signal(kPropertiesInterface, kPropertiesChanged);
+  MessageWriter writer(&signal);
+  writer.AppendString("org.chromium.TestInterface");
+
+  MessageWriter array_writer(NULL);
+  MessageWriter dict_entry_writer(NULL);
+
+  writer.OpenArray("{sv}", &array_writer);
+  writer.CloseContainer(&array_writer);
+
+  MessageWriter invalidated_array_writer(NULL);
+
+  writer.OpenArray("s", &invalidated_array_writer);
+  invalidated_array_writer.AppendString("Name");
+  writer.CloseContainer(&invalidated_array_writer);
+
   exported_object_->SendSignal(&signal);
 }
 
diff --git a/dbus/test_service.h b/dbus/test_service.h
index 5f215d3..de4aa72 100644
--- a/dbus/test_service.h
+++ b/dbus/test_service.h
@@ -167,6 +167,12 @@ class TestService : public base::Thread {
   // Helper function for SendPropertyChangedSignal().
   void SendPropertyChangedSignalInternal(const std::string& name);
 
+  // Sends a property invalidated signal for the name property.
+  void SendPropertyInvalidatedSignal();
+
+  // Helper function for SendPropertyInvalidatedSignal().
+  void SendPropertyInvalidatedSignalInternal();
+
   // Helper function for RequestOwnership().
   void RequestOwnershipInternal(base::Callback<void(bool)> callback);