window-service: fix bug when applying properties

Specifically, if in applying a property from a client another
property changed, then client would not be notified.

BUG=none
TEST=covered by test

Change-Id: Ie0f28eb07a7874a9acdcb4fd4f94b2eda03a040b
Reviewed-on: https://chromium-review.googlesource.com/1240197Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#593710}

services/ws/BUILD.gn

@@ -182,6 +182,7 @@ source_set("tests") {
   testonly = true
 
   sources = [
+    "client_root_unittest.cc",
     "drag_drop_delegate_unittest.cc",
     "embedding_unittest.cc",
     "focus_handler_unittest.cc",

services/ws/client_change.cc

@@ -13,8 +13,9 @@ namespace ws {
 
 ClientChange::ClientChange(ClientChangeTracker* tracker,
                            aura::Window* window,
-                           ClientChangeType type)
-    : tracker_(tracker), type_(type) {
+                           ClientChangeType type,
+                           const void* property_key)
+    : tracker_(tracker), type_(type), property_key_(property_key) {
   DCHECK(!tracker_->current_change_);
   tracker_->current_change_ = this;
   if (window)

services/ws/client_change.h

@@ -39,24 +39,33 @@ enum class ClientChangeType {
 // the window.
 class COMPONENT_EXPORT(WINDOW_SERVICE) ClientChange {
  public:
+  // |property_key| is only used for changes of type kProperty.
   ClientChange(ClientChangeTracker* tracker,
                aura::Window* window,
-               ClientChangeType type);
+               ClientChangeType type,
+               const void* property_key = nullptr);
   ~ClientChange();
 
   // The window the changes associated with. Is null if the window has been
   // destroyed during processing.
   aura::Window* window() {
+    return const_cast<aura::Window*>(
+        const_cast<const ClientChange*>(this)->window());
+  }
+
+  const aura::Window* window() const {
     return !window_tracker_.windows().empty() ? window_tracker_.windows()[0]
                                               : nullptr;
   }
 
   ClientChangeType type() const { return type_; }
+  const void* property_key() const { return property_key_; }
 
  private:
   ClientChangeTracker* tracker_;
   aura::WindowTracker window_tracker_;
   const ClientChangeType type_;
+  const void* property_key_;
 
   DISALLOW_COPY_AND_ASSIGN(ClientChange);
 };

services/ws/client_change_tracker.cc

@@ -12,10 +12,26 @@ ClientChangeTracker::ClientChangeTracker() = default;
 
 ClientChangeTracker::~ClientChangeTracker() = default;
 
-bool ClientChangeTracker::IsProcessingChangeForWindow(aura::Window* window,
-                                                      ClientChangeType type) {
+bool ClientChangeTracker::IsProcessingChangeForWindow(
+    aura::Window* window,
+    ClientChangeType type) const {
+  return DoesCurrentChangeEqual(window, type, nullptr);
+}
+
+bool ClientChangeTracker::IsProcessingPropertyChangeForWindow(
+    aura::Window* window,
+    const void* property_key) const {
+  return DoesCurrentChangeEqual(window, ClientChangeType::kProperty,
+                                property_key);
+}
+
+bool ClientChangeTracker::DoesCurrentChangeEqual(
+    aura::Window* window,
+    ClientChangeType type,
+    const void* property_key) const {
   return current_change_ && current_change_->window() == window &&
-         current_change_->type() == type;
+         current_change_->type() == type &&
+         current_change_->property_key() == property_key;
 }
 
 }  // namespace ws

services/ws/client_change_tracker.h

@@ -29,11 +29,18 @@ class COMPONENT_EXPORT(WINDOW_SERVICE) ClientChangeTracker {
   ClientChangeTracker();
   ~ClientChangeTracker();
 
-  bool IsProcessingChangeForWindow(aura::Window* window, ClientChangeType type);
+  bool IsProcessingChangeForWindow(aura::Window* window,
+                                   ClientChangeType type) const;
+  bool IsProcessingPropertyChangeForWindow(aura::Window* window,
+                                           const void* property_key) const;
 
  private:
   friend class ClientChange;
 
+  bool DoesCurrentChangeEqual(aura::Window* window,
+                              ClientChangeType type,
+                              const void* property_key) const;
+
   // Owned by the caller that created the ClientChange. This is set in
   // ClientChange's constructor and reset in the destructor.
   ClientChange* current_change_ = nullptr;

services/ws/client_root.cc

@@ -179,8 +179,8 @@ void ClientRoot::HandleBoundsOrScaleFactorChange(const gfx::Rect& old_bounds) {
 void ClientRoot::OnWindowPropertyChanged(aura::Window* window,
                                          const void* key,
                                          intptr_t old) {
-  if (window_tree_->property_change_tracker_->IsProcessingChangeForWindow(
-          window, ClientChangeType::kProperty)) {
+  if (window_tree_->property_change_tracker_
+          ->IsProcessingPropertyChangeForWindow(window, key)) {
     // Do not send notifications for changes intiated by the client.
     return;
   }

services/ws/client_root_unittest.cc

+// Copyright 2018 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 "services/ws/client_root.h"
+
+#include <string>
+
+#include "services/ws/public/cpp/property_type_converters.h"
+#include "services/ws/public/mojom/window_manager.mojom.h"
+#include "services/ws/window_service.h"
+#include "services/ws/window_service_test_setup.h"
+#include "services/ws/window_tree_test_helper.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/aura/client/aura_constants.h"
+#include "ui/aura/mus/property_converter.h"
+#include "ui/aura/window.h"
+#include "ui/aura/window_observer.h"
+#include "ui/aura/window_tracker.h"
+
+namespace ws {
+namespace {
+
+// WindowObserver that changes a property (|aura::client::kNameKey|) from
+// OnWindowPropertyChanged(). This mirrors ash changing a property when applying
+// a property change from a client.
+class CascadingPropertyTestHelper : public aura::WindowObserver {
+ public:
+  explicit CascadingPropertyTestHelper(aura::Window* window) : window_(window) {
+    window_->AddObserver(this);
+  }
+  ~CascadingPropertyTestHelper() override { window_->RemoveObserver(this); }
+
+  bool did_set_property() const { return did_set_property_; }
+
+  // WindowObserver:
+  void OnWindowPropertyChanged(aura::Window* window,
+                               const void* key,
+                               intptr_t old) override {
+    if (!did_set_property_) {
+      did_set_property_ = true;
+      window->SetProperty(aura::client::kNameKey, new std::string("TEST"));
+    }
+  }
+
+ private:
+  aura::Window* window_;
+  bool did_set_property_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(CascadingPropertyTestHelper);
+};
+
+// Verifies a property change that occurs while servicing a property change from
+// the client results in notifying the client of the new property.
+TEST(ClientRoot, CascadingPropertyChange) {
+  WindowServiceTestSetup setup;
+  aura::Window* top_level =
+      setup.window_tree_test_helper()->NewTopLevelWindow();
+  ASSERT_TRUE(top_level);
+  setup.changes()->clear();
+  CascadingPropertyTestHelper property_helper(top_level);
+
+  // Apply a change from a client.
+  aura::PropertyConverter::PrimitiveType client_value = true;
+  std::vector<uint8_t> client_transport_value =
+      mojo::ConvertTo<std::vector<uint8_t>>(client_value);
+  setup.window_tree_test_helper()->SetWindowProperty(
+      top_level, mojom::WindowManager::kAlwaysOnTop_Property,
+      client_transport_value, 2);
+
+  // CascadingPropertyTestHelper should have gotten the change *and* changed
+  // another property.
+  EXPECT_TRUE(property_helper.did_set_property());
+  ASSERT_FALSE(setup.changes()->empty());
+
+  // The client should be notified of the new value.
+  EXPECT_EQ(CHANGE_TYPE_PROPERTY_CHANGED, (*setup.changes())[0].type);
+  EXPECT_EQ(mojom::WindowManager::kName_Property,
+            (*setup.changes())[0].property_key);
+  setup.changes()->erase(setup.changes()->begin());
+
+  // And the initial change should be acked with completed.
+  EXPECT_EQ("ChangeCompleted id=2 success=true",
+            SingleChangeToDescription(*setup.changes()));
+  EXPECT_TRUE(top_level->GetProperty(aura::client::kAlwaysOnTopKey));
+}
+
+}  // namespace
+}  // namespace ws

services/ws/window_tree.cc

@@ -1010,16 +1010,21 @@ bool WindowTree::SetWindowPropertyImpl(
   }
   aura::PropertyConverter* property_converter =
       window_service_->property_converter();
-  DCHECK(property_converter->IsTransportNameRegistered(name))
-      << "Attempting to set an unregistered property; this is not implemented. "
-      << "property name=" << name;
+  if (!property_converter->IsTransportNameRegistered(name)) {
+    NOTREACHED() << "Attempting to set an unregistered property; this is not "
+                    "implemented. property name="
+                 << name;
+    return false;
+  }
   if (!IsClientCreatedWindow(window) && !IsClientRootWindow(window)) {
     DVLOG(1) << "SetWindowProperty failed (access policy denied change)";
     return false;
   }
 
-  ClientChange change(property_change_tracker_.get(), window,
-                      ClientChangeType::kProperty);
+  ClientChange change(
+      property_change_tracker_.get(), window, ClientChangeType::kProperty,
+      property_converter->GetPropertyKeyFromTransportName(name));
+
   // Special handle the property whose value is a pointer to aura::Window since
   // property converter can't convert the transported value.
   const aura::WindowProperty<aura::Window*>* property =

ui/aura/mus/property_converter.cc

@@ -116,6 +116,51 @@ PropertyConverter::PropertyConverter() {
 
 PropertyConverter::~PropertyConverter() {}
 
+const void* PropertyConverter::GetPropertyKeyFromTransportName(
+    const std::string& transport_name) {
+  for (const auto& primitive_property : primitive_properties_) {
+    if (primitive_property.second.transport_name == transport_name)
+      return primitive_property.first;
+  }
+
+  for (const auto& image_property : image_properties_) {
+    if (image_property.second == transport_name)
+      return image_property.first->name;
+  }
+
+  for (const auto& rect_property : rect_properties_) {
+    if (rect_property.second == transport_name)
+      return rect_property.first->name;
+  }
+
+  for (const auto& size_property : size_properties_) {
+    if (size_property.second == transport_name)
+      return size_property.first->name;
+  }
+
+  for (const auto& string_property : string_properties_) {
+    if (string_property.second == transport_name)
+      return string_property.first->name;
+  }
+
+  for (const auto& string16_property : string16_properties_) {
+    if (string16_property.second == transport_name)
+      return string16_property.first->name;
+  }
+
+  for (const auto& unguessable_token_property : unguessable_token_properties_) {
+    if (unguessable_token_property.second == transport_name)
+      return unguessable_token_property.first->name;
+  }
+
+  for (const auto& window_ptr_property : window_ptr_properties_) {
+    if (window_ptr_property.second == transport_name)
+      return window_ptr_property.first->name;
+  }
+
+  return nullptr;
+}
+
 bool PropertyConverter::IsTransportNameRegistered(
     const std::string& name) const {
   return transport_names_.count(name) > 0;
@@ -334,6 +379,9 @@ void PropertyConverter::SetPropertyFromTransportValue(
     }
   }
 
+  // WARNING: Adding a new map, be sure and update
+  // GetPropertyKeyFromTransportName() as well.
+
   DVLOG(2) << "Unknown mus property name: " << transport_name;
 }
 

ui/aura/mus/property_converter.h

@@ -46,6 +46,10 @@ class AURA_EXPORT PropertyConverter {
   // accept any value.
   static base::RepeatingCallback<bool(int64_t)> CreateAcceptAnyValueCallback();
 
+  // Returns the key for the window property registered against the specified
+  // transport name.
+  const void* GetPropertyKeyFromTransportName(const std::string& name);
+
   // Returns true if RegisterProperty() has been called with the specified
   // transport name.
   bool IsTransportNameRegistered(const std::string& name) const;