ws: Add property support for WindowToWindowData and client notifications

Add window properties to ws2 WindowData structs, like the original ws.
Add a helper function, PropertyConverter::GetTransportProperties.

Have ClientRoot notify clients on property changes, like bounds changes.
Add support for tracking client-initiated property changes.
(this only works for client roots? what about child windows?)

Encode test tracker property transport values as hex.
(so a false bool value is reported as "000..000" instead of "")

Add tests for WindowToWindowData and server-initiated property changes.

Bug: 837695
Change-Id: I2a8e4ce7eeca0b8300cca9e20d668c178d77ee7e
Reviewed-on: https://chromium-review.googlesource.com/1058118
Commit-Queue: Michael Wasserman <msw@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558547}

services/ui/ws2/client_change.h

@@ -27,6 +27,8 @@ class ClientChangeTracker;
 enum class ClientChangeType {
   // Used for WindowTree::SetWindowBounds().
   kBounds,
+  // Used for WindowTree::SetWindowProperty().
+  kProperty,
 };
 
 // ClientChange represents an incoming request from a WindowTreeClient. For

services/ui/ws2/client_root.cc

@@ -7,8 +7,10 @@
 #include "services/ui/ws2/client_change.h"
 #include "services/ui/ws2/client_change_tracker.h"
 #include "services/ui/ws2/client_window.h"
+#include "services/ui/ws2/window_service.h"
 #include "services/ui/ws2/window_service_client.h"
 #include "ui/aura/mus/client_surface_embedder.h"
+#include "ui/aura/mus/property_converter.h"
 #include "ui/aura/window.h"
 #include "ui/compositor/dip_util.h"
 
@@ -65,6 +67,29 @@ void ClientRoot::UpdatePrimarySurfaceId() {
       viz::SurfaceId(window_->GetFrameSinkId(), GetLocalSurfaceId()));
 }
 
+void ClientRoot::OnWindowPropertyChanged(aura::Window* window,
+                                         const void* key,
+                                         intptr_t old) {
+  if (window_service_client_->property_change_tracker_
+          ->IsProcessingChangeForWindow(window, ClientChangeType::kProperty)) {
+    // Do not send notifications for changes intiated by the client.
+    return;
+  }
+  std::string transport_name;
+  std::unique_ptr<std::vector<uint8_t>> transport_value;
+  if (window_service_client_->window_service()
+          ->property_converter()
+          ->ConvertPropertyForTransport(window, key, &transport_name,
+                                        &transport_value)) {
+    base::Optional<std::vector<uint8_t>> transport_value_mojo;
+    if (transport_value)
+      transport_value_mojo.emplace(std::move(*transport_value));
+    window_service_client_->window_tree_client_->OnWindowSharedPropertyChanged(
+        window_service_client_->TransportIdForWindow(window), transport_name,
+        transport_value_mojo);
+  }
+}
+
 void ClientRoot::OnWindowBoundsChanged(aura::Window* window,
                                        const gfx::Rect& old_bounds,
                                        const gfx::Rect& new_bounds,
@@ -74,8 +99,7 @@ void ClientRoot::OnWindowBoundsChanged(aura::Window* window,
   base::Optional<viz::LocalSurfaceId> surface_id = local_surface_id_;
   if (window_service_client_->property_change_tracker_
           ->IsProcessingChangeForWindow(window, ClientChangeType::kBounds)) {
-    // The expectation is the client is not notified of changes the client
-    // initiated.
+    // Do not send notifications for changes intiated by the client.
     return;
   }
   window_service_client_->window_tree_client_->OnWindowBoundsChanged(

services/ui/ws2/client_root.h

@@ -50,6 +50,9 @@ class COMPONENT_EXPORT(WINDOW_SERVICE) ClientRoot
   void UpdatePrimarySurfaceId();
 
   // aura::WindowObserver:
+  void OnWindowPropertyChanged(aura::Window* window,
+                               const void* key,
+                               intptr_t old) override;
   void OnWindowBoundsChanged(aura::Window* window,
                              const gfx::Rect& old_bounds,
                              const gfx::Rect& new_bounds,

services/ui/ws2/test_change_tracker.cc

@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "mojo/public/cpp/bindings/map.h"
@@ -422,7 +423,7 @@ void TestChangeTracker::OnWindowSharedPropertyChanged(
   if (!data)
     change.property_value = "NULL";
   else
-    change.property_value.assign(data->begin(), data->end());
+    change.property_value = base::HexEncode(data->data(), data->size());
   AddChange(change);
 }
 

services/ui/ws2/window_service_client.cc

@@ -340,8 +340,8 @@ mojom::WindowDataPtr WindowServiceClient::WindowToWindowData(
       transient_parent ? TransportIdForWindow(transient_parent)
                        : kInvalidTransportId;
   client_window->bounds = window->bounds();
-
-  // TODO(crbug.com/837695): Include the window properties.
+  client_window->properties =
+      window_service_->property_converter()->GetTransportProperties(window);
   client_window->visible = window->TargetVisibility();
   return client_window;
 }
@@ -506,16 +506,19 @@ bool WindowServiceClient::SetWindowPropertyImpl(
   }
   DCHECK(window_service_->property_converter()->IsTransportNameRegistered(name))
       << "Attempting to set an unrgistered property; this is not implemented.";
-  if (IsClientCreatedWindow(window) || IsClientRootWindow(window)) {
-    std::unique_ptr<std::vector<uint8_t>> data;
-    if (value.has_value())
-      data = std::make_unique<std::vector<uint8_t>>(value.value());
-    window_service_->property_converter()->SetPropertyFromTransportValue(
-        window, name, data.get());
-    return true;
+  if (!IsClientCreatedWindow(window) && !IsClientRootWindow(window)) {
+    DVLOG(1) << "SetWindowProperty failed (access policy denied change)";
+    return false;
   }
-  DVLOG(1) << "SetWindowProperty failed (access policy denied change)";
-  return false;
+
+  ClientChange change(property_change_tracker_.get(), window,
+                      ClientChangeType::kProperty);
+  std::unique_ptr<std::vector<uint8_t>> data;
+  if (value.has_value())
+    data = std::make_unique<std::vector<uint8_t>>(value.value());
+  window_service_->property_converter()->SetPropertyFromTransportValue(
+      window, name, data.get());
+  return true;
 }
 
 bool WindowServiceClient::SetWindowOpacityImpl(const ClientWindowId& window_id,

services/ui/ws2/window_service_client.h

@@ -73,6 +73,8 @@ class COMPONENT_EXPORT(WINDOW_SERVICE) WindowServiceClient
   void SendPointerWatcherEventToClient(int64_t display_id,
                                        std::unique_ptr<Event> event);
 
+  WindowService* window_service() { return window_service_; }
+
  private:
   friend class ClientRoot;
   friend class WindowServiceClientTestHelper;

services/ui/ws2/window_service_client_test_helper.cc

@@ -19,6 +19,11 @@ mojom::WindowTree* WindowServiceClientTestHelper::window_tree() {
   return static_cast<mojom::WindowTree*>(window_service_client_);
 }
 
+mojom::WindowDataPtr WindowServiceClientTestHelper::WindowToWindowData(
+    aura::Window* window) {
+  return window_service_client_->WindowToWindowData(window);
+}
+
 aura::Window* WindowServiceClientTestHelper::NewWindow(
     Id transport_window_id,
     base::flat_map<std::string, std::vector<uint8_t>> properties) {
@@ -47,13 +52,6 @@ void WindowServiceClientTestHelper::SetWindowBounds(aura::Window* window,
       local_surface_id);
 }
 
-void WindowServiceClientTestHelper::SetEventTargetingPolicy(
-    aura::Window* window,
-    mojom::EventTargetingPolicy policy) {
-  window_service_client_->SetEventTargetingPolicy(
-      window_service_client_->TransportIdForWindow(window), policy);
-}
-
 void WindowServiceClientTestHelper::SetWindowProperty(
     aura::Window* window,
     const std::string& name,
@@ -64,5 +62,12 @@ void WindowServiceClientTestHelper::SetWindowProperty(
       value);
 }
 
+void WindowServiceClientTestHelper::SetEventTargetingPolicy(
+    aura::Window* window,
+    mojom::EventTargetingPolicy policy) {
+  window_service_client_->SetEventTargetingPolicy(
+      window_service_client_->TransportIdForWindow(window), policy);
+}
+
 }  // namespace ws2
 }  // namespace ui

services/ui/ws2/window_service_client_test_helper.h

@@ -10,6 +10,7 @@
 
 #include "base/containers/flat_map.h"
 #include "base/macros.h"
+#include "services/ui/public/interfaces/window_tree_constants.mojom.h"
 #include "services/ui/ws2/ids.h"
 
 namespace aura {
@@ -41,6 +42,8 @@ class WindowServiceClientTestHelper {
 
   mojom::WindowTree* window_tree();
 
+  mojom::WindowDataPtr WindowToWindowData(aura::Window* window);
+
   aura::Window* NewWindow(
       Id transport_window_id,
       base::flat_map<std::string, std::vector<uint8_t>> properties = {});
@@ -50,12 +53,12 @@ class WindowServiceClientTestHelper {
   void SetWindowBounds(aura::Window* window,
                        const gfx::Rect& bounds,
                        uint32_t change_id = 1);
-  void SetEventTargetingPolicy(aura::Window* window,
-                               mojom::EventTargetingPolicy policy);
   void SetWindowProperty(aura::Window* window,
                          const std::string& name,
                          const std::vector<uint8_t>& value,
                          uint32_t change_id = 1);
+  void SetEventTargetingPolicy(aura::Window* window,
+                               mojom::EventTargetingPolicy policy);
 
  private:
   WindowServiceClient* window_service_client_;

services/ui/ws2/window_service_client_unittest.cc

@@ -118,8 +118,8 @@ class TestLayoutManager : public aura::LayoutManager {
 TEST(WindowServiceClientTest, NewWindow) {
   WindowServiceTestHelper helper;
   EXPECT_TRUE(helper.changes()->empty());
-  aura::Window* top_level = helper.helper()->NewWindow(1);
-  ASSERT_TRUE(top_level);
+  aura::Window* window = helper.helper()->NewWindow(1);
+  ASSERT_TRUE(window);
   EXPECT_EQ("ChangeCompleted id=1 sucess=true",
             SingleChangeToDescription(*helper.changes()));
   helper.changes()->clear();
@@ -130,12 +130,12 @@ TEST(WindowServiceClientTest, NewWindowWithProperties) {
   EXPECT_TRUE(helper.changes()->empty());
   aura::PropertyConverter::PrimitiveType value = true;
   std::vector<uint8_t> transport = mojo::ConvertTo<std::vector<uint8_t>>(value);
-  aura::Window* top_level = helper.helper()->NewWindow(
+  aura::Window* window = helper.helper()->NewWindow(
       1, {{ui::mojom::WindowManager::kAlwaysOnTop_Property, transport}});
-  ASSERT_TRUE(top_level);
+  ASSERT_TRUE(window);
   EXPECT_EQ("ChangeCompleted id=1 sucess=true",
             SingleChangeToDescription(*helper.changes()));
-  EXPECT_TRUE(top_level->GetProperty(aura::client::kAlwaysOnTopKey));
+  EXPECT_TRUE(window->GetProperty(aura::client::kAlwaysOnTopKey));
   helper.changes()->clear();
 }
 
@@ -163,7 +163,7 @@ TEST(WindowServiceClientTest, NewTopLevelWindowWithProperties) {
   helper.changes()->clear();
 }
 
-TEST(WindowServiceClientTest, SetWindowBounds) {
+TEST(WindowServiceClientTest, SetTopLevelWindowBounds) {
   WindowServiceTestHelper helper;
   aura::Window* top_level = helper.helper()->NewTopLevelWindow(1);
   helper.changes()->clear();
@@ -203,20 +203,49 @@ TEST(WindowServiceClientTest, SetWindowBounds) {
   helper.changes()->clear();
 }
 
-TEST(WindowServiceClientTest, SetWindowProperty) {
+// Tests the ability of the client to change properties on the server.
+TEST(WindowServiceClientTest, SetTopLevelWindowProperty) {
   WindowServiceTestHelper helper;
   aura::Window* top_level = helper.helper()->NewTopLevelWindow(1);
   helper.changes()->clear();
 
   EXPECT_FALSE(top_level->GetProperty(aura::client::kAlwaysOnTopKey));
-  aura::PropertyConverter::PrimitiveType value = true;
-  std::vector<uint8_t> transport = mojo::ConvertTo<std::vector<uint8_t>>(value);
+  aura::PropertyConverter::PrimitiveType client_value = true;
+  std::vector<uint8_t> client_transport_value =
+      mojo::ConvertTo<std::vector<uint8_t>>(client_value);
   helper.helper()->SetWindowProperty(
-      top_level, ui::mojom::WindowManager::kAlwaysOnTop_Property, transport, 2);
+      top_level, ui::mojom::WindowManager::kAlwaysOnTop_Property,
+      client_transport_value, 2);
   EXPECT_EQ("ChangeCompleted id=2 sucess=true",
             SingleChangeToDescription(*helper.changes()));
   EXPECT_TRUE(top_level->GetProperty(aura::client::kAlwaysOnTopKey));
   helper.changes()->clear();
+
+  top_level->SetProperty(aura::client::kAlwaysOnTopKey, false);
+  EXPECT_EQ(
+      "PropertyChanged window=0,1 key=prop:always_on_top "
+      "value=0000000000000000",
+      SingleChangeToDescription(*helper.changes()));
+  helper.changes()->clear();
+}
+
+TEST(WindowServiceClientTest, WindowToWindowData) {
+  WindowServiceTestHelper helper;
+  aura::Window* window = helper.helper()->NewWindow(1);
+  helper.changes()->clear();
+
+  window->SetBounds(gfx::Rect(1, 2, 300, 400));
+  window->SetProperty(aura::client::kAlwaysOnTopKey, true);
+  window->Show();  // Called to make the window visible.
+  mojom::WindowDataPtr data = helper.helper()->WindowToWindowData(window);
+  EXPECT_EQ(gfx::Rect(1, 2, 300, 400), data->bounds);
+  EXPECT_TRUE(data->visible);
+  EXPECT_EQ(1u, data->properties.count(
+                    ui::mojom::WindowManager::kAlwaysOnTop_Property));
+  EXPECT_EQ(
+      aura::PropertyConverter::PrimitiveType(true),
+      mojo::ConvertTo<aura::PropertyConverter::PrimitiveType>(
+          data->properties[ui::mojom::WindowManager::kAlwaysOnTop_Property]));
 }
 
 TEST(WindowServiceClientTest, PointerWatcher) {

ui/aura/mus/property_converter.cc

@@ -347,4 +347,16 @@ void PropertyConverter::RegisterString16Property(
   transport_names_.insert(transport_name);
 }
 
+base::flat_map<std::string, std::vector<uint8_t>>
+PropertyConverter::GetTransportProperties(Window* window) {
+  base::flat_map<std::string, std::vector<uint8_t>> properties;
+  std::string name;
+  std::unique_ptr<std::vector<uint8_t>> value;
+  for (const void* key : window->GetAllPropertyKeys()) {
+    if (ConvertPropertyForTransport(window, key, &name, &value))
+      properties[name] = value ? std::move(*value) : std::vector<uint8_t>();
+  }
+  return properties;
+}
+
 }  // namespace aura

ui/aura/mus/property_converter.h

@@ -108,6 +108,10 @@ class AURA_EXPORT PropertyConverter {
   void RegisterString16Property(const WindowProperty<base::string16*>* property,
                                 const char* transport_name);
 
+  // Get a flat map of the window's registered properties, to use for transport.
+  base::flat_map<std::string, std::vector<uint8_t>> GetTransportProperties(
+      Window* window);
+
  private:
   // Contains data needed to store and convert primitive-type properties.
   struct AURA_EXPORT PrimitiveProperty {

ui/aura/mus/window_tree_client.cc

@@ -981,20 +981,9 @@ void WindowTreeClient::OnWindowMusCreated(WindowMus* window) {
 
   DCHECK(window_manager_delegate_ || !IsRoot(window));
 
-  base::flat_map<std::string, std::vector<uint8_t>> transport_properties;
-  std::set<const void*> property_keys =
-      window->GetWindow()->GetAllPropertyKeys();
   PropertyConverter* property_converter = delegate_->GetPropertyConverter();
-  for (const void* key : property_keys) {
-    std::string transport_name;
-    std::unique_ptr<std::vector<uint8_t>> transport_value;
-    if (!property_converter->ConvertPropertyForTransport(
-            window->GetWindow(), key, &transport_name, &transport_value)) {
-      continue;
-    }
-    transport_properties[transport_name] =
-        transport_value ? std::move(*transport_value) : std::vector<uint8_t>();
-  }
+  base::flat_map<std::string, std::vector<uint8_t>> transport_properties =
+      property_converter->GetTransportProperties(window->GetWindow());
 
   const uint32_t change_id = ScheduleInFlightChange(
       std::make_unique<CrashInFlightChange>(window, ChangeType::NEW_WINDOW));