ws: Add support for WindowServiceClient::SetWindowProperty

Add a PropertyConverter to WindowService to support property syncing.
Implement SetWindowProperty[Impl] on WindowServiceClient.
Add a unit test, split up the existing fixture, refine GL init.

Add a helper function for Ash window property registration.
(consolidate out-of-sync registrations between Chrome and Ash)
Add a helper function for window service init in Ash.

Make kRenderTitleAreaProperty public (mojo side used by Chrome)
Check that properties are not registered twice with the converter.
Optionally register kShadowElevationKey in helper (for Ash, not Chrome).
Update and add crbug.com/837695 TODOs.

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

ash/BUILD.gn

@@ -1196,6 +1196,8 @@ component("ash") {
     "wm/workspace_controller.h",
     "ws/window_service_delegate_impl.cc",
     "ws/window_service_delegate_impl.h",
+    "ws/window_service_util.cc",
+    "ws/window_service_util.h",
   ]
 
   configs += [ "//build/config:precompiled_headers" ]

ash/public/cpp/BUILD.gn

@@ -95,6 +95,7 @@ component("cpp") {
   deps = [
     "//chromeos:power_manager_proto",
     "//components/prefs",
+    "//services/ui/public/interfaces",
     "//skia/public/interfaces",
     "//ui/aura",
     "//ui/chromeos/strings",
@@ -102,6 +103,7 @@ component("cpp") {
     "//ui/events/devices",
     "//ui/views",
     "//ui/views/mus",
+    "//ui/wm",
   ]
 
   public_deps = [

ash/public/cpp/window_properties.cc

@@ -5,10 +5,16 @@
 #include "ash/public/cpp/window_properties.h"
 
 #include "ash/public/cpp/shelf_types.h"
+#include "ash/public/cpp/window_pin_type.h"
+#include "ash/public/cpp/window_state_type.h"
 #include "ash/public/interfaces/window_pin_type.mojom.h"
+#include "ash/public/interfaces/window_properties.mojom.h"
 #include "ash/public/interfaces/window_state_type.mojom.h"
+#include "services/ui/public/interfaces/window_manager.mojom.h"
+#include "ui/aura/mus/property_converter.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/image/image_skia.h"
+#include "ui/wm/core/shadow_types.h"
 
 DEFINE_EXPORTED_UI_CLASS_PROPERTY_TYPE(ASH_PUBLIC_EXPORT,
                                        ash::mojom::WindowPinType)
@@ -19,6 +25,57 @@ DEFINE_EXPORTED_UI_CLASS_PROPERTY_TYPE(ASH_PUBLIC_EXPORT,
 
 namespace ash {
 
+void RegisterWindowProperties(aura::PropertyConverter* property_converter) {
+  property_converter->RegisterPrimitiveProperty(
+      kCanConsumeSystemKeysKey, mojom::kCanConsumeSystemKeys_Property,
+      aura::PropertyConverter::CreateAcceptAnyValueCallback());
+  property_converter->RegisterImageSkiaProperty(
+      kFrameImageActiveKey, mojom::kFrameImageActive_Property);
+  property_converter->RegisterPrimitiveProperty(
+      kHideShelfWhenFullscreenKey, mojom::kHideShelfWhenFullscreen_Property,
+      aura::PropertyConverter::CreateAcceptAnyValueCallback());
+  property_converter->RegisterPrimitiveProperty(
+      kPanelAttachedKey, ui::mojom::WindowManager::kPanelAttached_Property,
+      aura::PropertyConverter::CreateAcceptAnyValueCallback());
+  property_converter->RegisterPrimitiveProperty(
+      kRenderTitleAreaProperty,
+      ui::mojom::WindowManager::kRenderParentTitleArea_Property,
+      aura::PropertyConverter::CreateAcceptAnyValueCallback());
+  // This property is already registered by MusClient in Chrome, but not in Ash.
+  if (!property_converter->IsTransportNameRegistered(
+          ui::mojom::WindowManager::kShadowElevation_Property)) {
+    property_converter->RegisterPrimitiveProperty(
+        ::wm::kShadowElevationKey,
+        ui::mojom::WindowManager::kShadowElevation_Property,
+        aura::PropertyConverter::CreateAcceptAnyValueCallback());
+  }
+  property_converter->RegisterPrimitiveProperty(
+      kShelfItemTypeKey, ui::mojom::WindowManager::kShelfItemType_Property,
+      base::BindRepeating(&IsValidShelfItemType));
+  property_converter->RegisterPrimitiveProperty(
+      kWindowStateTypeKey, mojom::kWindowStateType_Property,
+      base::BindRepeating(&IsValidWindowStateType));
+  property_converter->RegisterPrimitiveProperty(
+      kWindowPinTypeKey, mojom::kWindowPinType_Property,
+      base::BindRepeating(&IsValidWindowPinType));
+  property_converter->RegisterPrimitiveProperty(
+      kWindowPositionManagedTypeKey, mojom::kWindowPositionManaged_Property,
+      aura::PropertyConverter::CreateAcceptAnyValueCallback());
+  property_converter->RegisterStringProperty(
+      kShelfIDKey, ui::mojom::WindowManager::kShelfID_Property);
+  property_converter->RegisterPrimitiveProperty(
+      kRestoreBoundsOverrideKey, mojom::kRestoreBoundsOverride_Property,
+      aura::PropertyConverter::CreateAcceptAnyValueCallback());
+  property_converter->RegisterPrimitiveProperty(
+      kRestoreWindowStateTypeOverrideKey,
+      mojom::kRestoreWindowStateTypeOverride_Property,
+      base::BindRepeating(&IsValidWindowStateType));
+  property_converter->RegisterPrimitiveProperty(
+      kWindowTitleShownKey,
+      ui::mojom::WindowManager::kWindowTitleShown_Property,
+      aura::PropertyConverter::CreateAcceptAnyValueCallback());
+}
+
 DEFINE_UI_CLASS_PROPERTY_KEY(BackdropWindowMode,
                              kBackdropWindowMode,
                              BackdropWindowMode::kAuto);
@@ -28,6 +85,7 @@ DEFINE_OWNED_UI_CLASS_PROPERTY_KEY(gfx::ImageSkia,
                                    nullptr);
 DEFINE_UI_CLASS_PROPERTY_KEY(bool, kHideShelfWhenFullscreenKey, true);
 DEFINE_UI_CLASS_PROPERTY_KEY(bool, kPanelAttachedKey, true);
+DEFINE_UI_CLASS_PROPERTY_KEY(bool, kRenderTitleAreaProperty, false);
 DEFINE_OWNED_UI_CLASS_PROPERTY_KEY(gfx::Rect,
                                    kRestoreBoundsOverrideKey,
                                    nullptr);

ash/public/cpp/window_properties.h

@@ -13,6 +13,7 @@
 #include "ui/base/class_property.h"
 
 namespace aura {
+class PropertyConverter;
 template <typename T>
 using WindowProperty = ui::ClassProperty<T>;
 }
@@ -35,6 +36,11 @@ enum class BackdropWindowMode {
   kAuto,  // The window manager decides if the window should have a backdrop.
 };
 
+// Registers Ash's properties with the given PropertyConverter. This allows Ash
+// and other services (eg. Chrome) to exchange Ash window property values.
+ASH_PUBLIC_EXPORT void RegisterWindowProperties(
+    aura::PropertyConverter* property_converter);
+
 // Shell-specific window property keys for use by ash and its clients.
 
 // Alphabetical sort.
@@ -61,6 +67,10 @@ ASH_PUBLIC_EXPORT extern const aura::WindowProperty<bool>* const
 ASH_PUBLIC_EXPORT extern const aura::WindowProperty<bool>* const
     kPanelAttachedKey;
 
+// Maps to ui::mojom::WindowManager::kRenderParentTitleArea_Property.
+ASH_PUBLIC_EXPORT extern const aura::WindowProperty<bool>* const
+    kRenderTitleAreaProperty;
+
 // A property key which stores the bounds to restore a window to. These take
 // preference over the current bounds. This is used by e.g. the tablet mode
 // window manager.

ash/shell/content/client/shell_content_browser_client.cc

@@ -9,11 +9,12 @@
 #include "ash/ash_service.h"
 #include "ash/components/quick_launch/public/mojom/constants.mojom.h"
 #include "ash/content/content_gpu_support.h"
+#include "ash/public/cpp/window_properties.h"
 #include "ash/public/interfaces/constants.mojom.h"
 #include "ash/shell.h"
 #include "ash/shell/content/client/shell_browser_main_parts.h"
 #include "ash/shell/grit/ash_shell_resources.h"
-#include "ash/ws/window_service_delegate_impl.h"
+#include "ash/ws/window_service_util.h"
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/command_line.h"
@@ -27,7 +28,6 @@
 #include "content/public/utility/content_utility_client.h"
 #include "services/ui/public/interfaces/constants.mojom.h"
 #include "services/ui/ws2/window_service.h"
-#include "services/ui/ws2/window_service_delegate.h"
 #include "storage/browser/quota/quota_settings.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -37,10 +37,9 @@ namespace ash {
 namespace shell {
 namespace {
 
-std::unique_ptr<service_manager::Service> CreateWindowService() {
-  return std::make_unique<ui::ws2::WindowService>(
-      Shell::Get()->window_service_delegate(),
-      std::make_unique<ContentGpuSupport>());
+// A factory function used in creation of the WindowService.
+std::unique_ptr<ui::ws2::GpuSupport> CreateContentGpuSupport() {
+  return std::make_unique<ContentGpuSupport>();
 }
 
 }  // namespace
@@ -97,7 +96,8 @@ void ShellContentBrowserClient::RegisterInProcessServices(
                                   AshService::CreateEmbeddedServiceInfo()));
 
   service_manager::EmbeddedServiceInfo ws_service_info;
-  ws_service_info.factory = base::BindRepeating(&CreateWindowService);
+  ws_service_info.factory = base::BindRepeating(
+      &CreateWindowService, base::BindRepeating(&CreateContentGpuSupport));
   ws_service_info.task_runner = base::ThreadTaskRunnerHandle::Get();
   services->insert(std::make_pair(ui::mojom::kServiceName, ws_service_info));
 }

ash/window_manager.cc

@@ -17,14 +17,9 @@
 #include "ash/event_matcher_util.h"
 #include "ash/host/ash_window_tree_host.h"
 #include "ash/public/cpp/config.h"
-#include "ash/public/cpp/shelf_types.h"
-#include "ash/public/cpp/window_pin_type.h"
 #include "ash/public/cpp/window_properties.h"
 #include "ash/public/cpp/window_state_type.h"
 #include "ash/public/interfaces/window_actions.mojom.h"
-#include "ash/public/interfaces/window_pin_type.mojom.h"
-#include "ash/public/interfaces/window_properties.mojom.h"
-#include "ash/public/interfaces/window_state_type.mojom.h"
 #include "ash/root_window_controller.h"
 #include "ash/root_window_settings.h"
 #include "ash/session/session_controller.h"
@@ -63,7 +58,6 @@
 #include "ui/events/mojo/event.mojom.h"
 #include "ui/views/mus/pointer_watcher_event_router.h"
 #include "ui/wm/core/capture_controller.h"
-#include "ui/wm/core/shadow_types.h"
 #include "ui/wm/core/window_animations.h"
 #include "ui/wm/core/wm_state.h"
 #include "ui/wm/public/activation_client.h"
@@ -88,52 +82,7 @@ WindowManager::WindowManager(service_manager::Connector* connector,
       show_primary_host_on_connect_(show_primary_host_on_connect),
       wm_state_(std::make_unique<::wm::WMState>()),
       property_converter_(std::make_unique<aura::PropertyConverter>()) {
-  property_converter_->RegisterPrimitiveProperty(
-      kCanConsumeSystemKeysKey, ash::mojom::kCanConsumeSystemKeys_Property,
-      aura::PropertyConverter::CreateAcceptAnyValueCallback());
-  property_converter_->RegisterPrimitiveProperty(
-      aura::client::kDrawAttentionKey,
-      ui::mojom::WindowManager::kDrawAttention_Property,
-      aura::PropertyConverter::CreateAcceptAnyValueCallback());
-  property_converter_->RegisterImageSkiaProperty(
-      kFrameImageActiveKey, ash::mojom::kFrameImageActive_Property);
-  property_converter_->RegisterPrimitiveProperty(
-      kPanelAttachedKey, ui::mojom::WindowManager::kPanelAttached_Property,
-      aura::PropertyConverter::CreateAcceptAnyValueCallback());
-  property_converter_->RegisterPrimitiveProperty(
-      kRenderTitleAreaProperty,
-      ui::mojom::WindowManager::kRenderParentTitleArea_Property,
-      aura::PropertyConverter::CreateAcceptAnyValueCallback());
-  property_converter_->RegisterPrimitiveProperty(
-      kShelfItemTypeKey, ui::mojom::WindowManager::kShelfItemType_Property,
-      base::Bind(&IsValidShelfItemType));
-  property_converter_->RegisterPrimitiveProperty(
-      ::wm::kShadowElevationKey,
-      ui::mojom::WindowManager::kShadowElevation_Property,
-      aura::PropertyConverter::CreateAcceptAnyValueCallback());
-  property_converter_->RegisterPrimitiveProperty(
-      kWindowStateTypeKey, mojom::kWindowStateType_Property,
-      base::Bind(&ash::IsValidWindowStateType));
-  property_converter_->RegisterPrimitiveProperty(
-      kWindowPinTypeKey, ash::mojom::kWindowPinType_Property,
-      base::Bind(&ash::IsValidWindowPinType));
-  property_converter_->RegisterPrimitiveProperty(
-      kWindowPositionManagedTypeKey,
-      ash::mojom::kWindowPositionManaged_Property,
-      aura::PropertyConverter::CreateAcceptAnyValueCallback());
-  property_converter_->RegisterStringProperty(
-      kShelfIDKey, ui::mojom::WindowManager::kShelfID_Property);
-  property_converter_->RegisterPrimitiveProperty(
-      kRestoreBoundsOverrideKey, ash::mojom::kRestoreBoundsOverride_Property,
-      aura::PropertyConverter::CreateAcceptAnyValueCallback());
-  property_converter_->RegisterPrimitiveProperty(
-      kRestoreWindowStateTypeOverrideKey,
-      ash::mojom::kRestoreWindowStateTypeOverride_Property,
-      base::BindRepeating(&ash::IsValidWindowStateType));
-  property_converter_->RegisterPrimitiveProperty(
-      kWindowTitleShownKey,
-      ui::mojom::WindowManager::kWindowTitleShown_Property,
-      aura::PropertyConverter::CreateAcceptAnyValueCallback());
+  RegisterWindowProperties(property_converter_.get());
 }
 
 WindowManager::~WindowManager() {

ash/wm/property_util.h

@@ -31,16 +31,16 @@ enum class WindowType;
 }  // namespace ui
 
 namespace ash {
+
 namespace mojom {
 enum class WindowStyle;
 }
 
 // Functions for extracting properties that are used at a Window creation time.
-// When an aura::Window is created at the request of a client an initial set of
-// properties is supplied to allow the WindowManager (ash) to configure the
-// newly created window. Not all of these properties need be persisted, some are
-// used solely to configure the window. This file contains the functions used
-// to extract these properties.
+// Clients pass an initial set of properties when requesting a new aura::Window.
+// Not all of these properties need be persisted, some are used solely to
+// configure the window. The functions below extract those properties.
+
 // Long lived properties are converted and stored as properties on the
 // associated aura::Window. See aura::PropertyConverter for this set of
 // properties.

ash/wm/window_properties.cc

@@ -14,8 +14,6 @@ namespace ash {
 
 DEFINE_UI_CLASS_PROPERTY_KEY(bool, kLockedToRootKey, false);
 
-DEFINE_UI_CLASS_PROPERTY_KEY(bool, kRenderTitleAreaProperty, false);
-
 DEFINE_UI_CLASS_PROPERTY_KEY(bool, kSnapChildrenToPixelBoundary, false);
 
 DEFINE_UI_CLASS_PROPERTY_KEY(WidgetCreationType,

ash/wm/window_properties.h

@@ -43,10 +43,6 @@ enum class WidgetCreationType {
 // bounds outside of its root window is set.
 ASH_EXPORT extern const aura::WindowProperty<bool>* const kLockedToRootKey;
 
-// Maps to ui::mojom::WindowManager::kRenderParentTitleArea_Property.
-ASH_EXPORT extern const aura::WindowProperty<bool>* const
-    kRenderTitleAreaProperty;
-
 // Containers with this property (true) are aligned with physical pixel
 // boundary.
 extern const aura::WindowProperty<bool>* const kSnapChildrenToPixelBoundary;

ash/ws/window_service_delegate_impl.cc

@@ -21,6 +21,7 @@ std::unique_ptr<aura::Window> WindowServiceDelegateImpl::NewTopLevel(
   window->SetType(aura::client::WINDOW_TYPE_NORMAL);
   window->Init(ui::LAYER_NOT_DRAWN);
   ash::wm::GetDefaultParent(window.get(), gfx::Rect())->AddChild(window.get());
+  // TODO(crbug.com/837695): Apply |properties|.
   return window;
 }
 

ash/ws/window_service_util.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 "ash/ws/window_service_util.h"
+
+#include "ash/public/cpp/window_properties.h"
+#include "ash/shell.h"
+#include "ash/ws/window_service_delegate_impl.h"
+#include "services/ui/ws2/gpu_support.h"
+#include "services/ui/ws2/window_service.h"
+
+namespace ash {
+
+std::unique_ptr<service_manager::Service> CreateWindowService(
+    const base::RepeatingCallback<std::unique_ptr<ui::ws2::GpuSupport>()>&
+        gpu_support_factory) {
+  auto window_service = std::make_unique<ui::ws2::WindowService>(
+      Shell::Get()->window_service_delegate(), gpu_support_factory.Run());
+  RegisterWindowProperties(window_service->property_converter());
+  return window_service;
+}
+
+}  // namespace ash

ash/ws/window_service_util.h

+// 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.
+
+#ifndef ASH_WS_WINDOW_SERVICE_UTIL_H_
+#define ASH_WS_WINDOW_SERVICE_UTIL_H_
+
+#include <memory>
+
+#include "ash/ash_export.h"
+#include "base/callback.h"
+#include "services/ui/ws2/window_service_delegate.h"
+
+namespace service_manager {
+class Service;
+}  // namespace service_manager
+
+namespace ui {
+namespace ws2 {
+class GpuSupport;
+}  // namespace ws2
+}  // namespace ui
+
+namespace ash {
+
+// A factory function to create WindowService with Ash-specific initialization.
+// This and |gpu_support_factory| should only be run once, but factory functions
+// for embedded mojo services use repeating callbacks.
+ASH_EXPORT std::unique_ptr<service_manager::Service> CreateWindowService(
+    const base::RepeatingCallback<std::unique_ptr<ui::ws2::GpuSupport>()>&
+        gpu_support_factory);
+
+}  // namespace ash
+
+#endif  // ASH_WS_WINDOW_SERVICE_UTIL_H_

chrome/browser/DEPS

@@ -215,7 +215,7 @@ specific_include_rules = {
     # Once Ash runs out of process no longer needed.
     "+ash/content/content_gpu_support.h",
     "+ash/shell.h",
-    "+ash/ws/window_service_delegate_impl.h",
+    "+ash/ws/window_service_util.h",
     "+services/ui/ws2/window_service.h",
 
     # Needed for classic mode.

chrome/browser/ash_service_registry.cc

@@ -7,9 +7,11 @@
 #include "ash/ash_service.h"
 #include "ash/components/quick_launch/public/mojom/constants.mojom.h"
 #include "ash/content/content_gpu_support.h"
+#include "ash/public/cpp/window_properties.h"
 #include "ash/public/interfaces/constants.mojom.h"
+#include "ash/public/interfaces/window_properties.mojom.h"
 #include "ash/shell.h"
-#include "ash/ws/window_service_delegate_impl.h"
+#include "ash/ws/window_service_util.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -65,10 +67,9 @@ void RegisterOutOfProcessServicesImpl(
   }
 }
 
-std::unique_ptr<service_manager::Service> CreateEmbeddedWindowService() {
-  return std::make_unique<ui::ws2::WindowService>(
-      ash::Shell::Get()->window_service_delegate(),
-      std::make_unique<ash::ContentGpuSupport>());
+// A factory function used in creation of the WindowService.
+std::unique_ptr<ui::ws2::GpuSupport> CreateContentGpuSupport() {
+  return std::make_unique<ash::ContentGpuSupport>();
 }
 
 }  // namespace
@@ -105,7 +106,8 @@ void RegisterInProcessServices(
       ash::AshService::CreateEmbeddedServiceInfo();
 
   service_manager::EmbeddedServiceInfo ui_service_info;
-  ui_service_info.factory = base::BindRepeating(&CreateEmbeddedWindowService);
+  ui_service_info.factory = base::BindRepeating(
+      &ash::CreateWindowService, base::BindRepeating(&CreateContentGpuSupport));
   ui_service_info.task_runner = base::ThreadTaskRunnerHandle::Get();
   (*services)[ui::mojom::kServiceName] = ui_service_info;
 }

chrome/browser/ui/ash/chrome_browser_main_extra_parts_ash.cc

@@ -9,14 +9,9 @@
 #include "ash/public/cpp/ash_switches.h"
 #include "ash/public/cpp/mus_property_mirror_ash.h"
 #include "ash/public/cpp/shelf_model.h"
-#include "ash/public/cpp/window_pin_type.h"
 #include "ash/public/cpp/window_properties.h"
-#include "ash/public/cpp/window_state_type.h"
 #include "ash/public/interfaces/constants.mojom.h"
 #include "ash/public/interfaces/process_creation_time_recorder.mojom.h"
-#include "ash/public/interfaces/window_pin_type.mojom.h"
-#include "ash/public/interfaces/window_properties.mojom.h"
-#include "ash/public/interfaces/window_state_type.mojom.h"
 #include "ash/shell.h"
 #include "base/command_line.h"
 #include "chrome/browser/browser_process.h"
@@ -137,56 +132,12 @@ void ChromeBrowserMainExtraPartsAsh::ServiceManagerConnectionStarted(
   if (chromeos::GetAshConfig() == ash::Config::MASH) {
     // ash::Shell will not be created because ash is running out-of-process.
     ash::Shell::SetIsBrowserProcessWithMash();
-    // Register ash-specific window properties with Chrome's property converter.
-    // This propagates ash properties set on chrome windows to ash, via mojo.
     DCHECK(views::MusClient::Exists());
     views::MusClient* mus_client = views::MusClient::Get();
     aura::WindowTreeClientDelegate* delegate = mus_client;
-    aura::PropertyConverter* converter = delegate->GetPropertyConverter();
-
-    converter->RegisterPrimitiveProperty(
-        ash::kCanConsumeSystemKeysKey,
-        ash::mojom::kCanConsumeSystemKeys_Property,
-        aura::PropertyConverter::CreateAcceptAnyValueCallback());
-    converter->RegisterImageSkiaProperty(
-        ash::kFrameImageActiveKey, ash::mojom::kFrameImageActive_Property);
-    converter->RegisterPrimitiveProperty(
-        ash::kHideShelfWhenFullscreenKey,
-        ash::mojom::kHideShelfWhenFullscreen_Property,
-        aura::PropertyConverter::CreateAcceptAnyValueCallback());
-    converter->RegisterPrimitiveProperty(
-        ash::kPanelAttachedKey,
-        ui::mojom::WindowManager::kPanelAttached_Property,
-        aura::PropertyConverter::CreateAcceptAnyValueCallback());
-    converter->RegisterPrimitiveProperty(
-        ash::kShelfItemTypeKey,
-        ui::mojom::WindowManager::kShelfItemType_Property,
-        base::Bind(&ash::IsValidShelfItemType));
-    converter->RegisterPrimitiveProperty(
-        ash::kWindowStateTypeKey, ash::mojom::kWindowStateType_Property,
-        base::Bind(&ash::IsValidWindowStateType));
-    converter->RegisterPrimitiveProperty(
-        ash::kWindowPinTypeKey, ash::mojom::kWindowPinType_Property,
-        base::Bind(&ash::IsValidWindowPinType));
-    converter->RegisterPrimitiveProperty(
-        ash::kWindowPositionManagedTypeKey,
-        ash::mojom::kWindowPositionManaged_Property,
-        aura::PropertyConverter::CreateAcceptAnyValueCallback());
-    converter->RegisterStringProperty(
-        ash::kShelfIDKey, ui::mojom::WindowManager::kShelfID_Property);
-    converter->RegisterPrimitiveProperty(
-        ash::kRestoreBoundsOverrideKey,
-        ash::mojom::kRestoreBoundsOverride_Property,
-        aura::PropertyConverter::CreateAcceptAnyValueCallback());
-    converter->RegisterPrimitiveProperty(
-        ash::kRestoreWindowStateTypeOverrideKey,
-        ash::mojom::kRestoreWindowStateTypeOverride_Property,
-        base::BindRepeating(&ash::IsValidWindowStateType));
-    converter->RegisterPrimitiveProperty(
-        ash::kWindowTitleShownKey,
-        ui::mojom::WindowManager::kWindowTitleShown_Property,
-        aura::PropertyConverter::CreateAcceptAnyValueCallback());
-
+    // Register ash-specific window properties with Chrome's property converter.
+    // Values of registered properties will be transported between the services.
+    ash::RegisterWindowProperties(delegate->GetPropertyConverter());
     mus_client->SetMusPropertyMirror(
         std::make_unique<ash::MusPropertyMirrorAsh>());
   }

services/ui/ws2/BUILD.gn

@@ -116,6 +116,7 @@ source_set("tests") {
     "//mojo/public/cpp/bindings",
     "//services/service_manager/public/cpp:service_test_support",
     "//services/service_manager/public/mojom",
+    "//services/ui/public/cpp",
     "//services/ui/public/interfaces",
     "//testing/gtest",
     "//third_party/mesa:osmesa",

services/ui/ws2/window_service.h

@@ -16,6 +16,7 @@
 #include "services/ui/public/interfaces/ime/ime.mojom.h"
 #include "services/ui/public/interfaces/window_tree.mojom.h"
 #include "services/ui/ws2/ids.h"
+#include "ui/aura/mus/property_converter.h"
 
 namespace aura {
 class Window;
@@ -58,6 +59,8 @@ class COMPONENT_EXPORT(WINDOW_SERVICE) WindowService
 
   WindowServiceDelegate* delegate() { return delegate_; }
 
+  aura::PropertyConverter* property_converter() { return &property_converter_; }
+
   // service_manager::Service:
   void OnStart() override;
   void OnBindInterface(const service_manager::BindSourceInfo& remote_info,
@@ -80,6 +83,9 @@ class COMPONENT_EXPORT(WINDOW_SERVICE) WindowService
 
   std::unique_ptr<WindowTreeFactory> window_tree_factory_;
 
+  // Helper used to serialize and deserialize window properties.
+  aura::PropertyConverter property_converter_;
+
   // Id for the next WindowServiceClient.
   ClientSpecificId next_client_id_ = kWindowServerClientId + 1;
 

services/ui/ws2/window_service_client.cc

@@ -18,6 +18,7 @@
 #include "services/ui/ws2/window_service_delegate.h"
 #include "ui/aura/client/transient_window_client.h"
 #include "ui/aura/env.h"
+#include "ui/aura/mus/property_converter.h"
 #include "ui/aura/window.h"
 #include "ui/aura/window_observer.h"
 #include "ui/compositor/layer.h"
@@ -330,7 +331,7 @@ mojom::WindowDataPtr WindowServiceClient::WindowToWindowData(
                        : kInvalidTransportId;
   client_window->bounds = window->bounds();
 
-  // TODO: use property mapping.
+  // TODO(crbug.com/837695): Include the window properties.
   client_window->visible = window->TargetVisibility();
   return client_window;
 }
@@ -368,6 +369,7 @@ bool WindowServiceClient::NewWindowImpl(
   window->Init(LAYER_NOT_DRAWN);
   // Windows created by the client should only be destroyed by the client.
   window->set_owned_by_parent(false);
+  // TODO(crbug.com/837695): Apply |properties|.
   return true;
 }
 
@@ -476,6 +478,31 @@ bool WindowServiceClient::SetWindowVisibilityImpl(
   return false;
 }
 
+bool WindowServiceClient::SetWindowPropertyImpl(
+    const ClientWindowId& window_id,
+    const std::string& name,
+    const base::Optional<std::vector<uint8_t>>& value) {
+  aura::Window* window = GetWindowByClientId(window_id);
+  DVLOG(3) << "SetWindowProperty client=" << client_id_
+           << " client window_id= " << window_id.ToString();
+  if (!window) {
+    DVLOG(1) << "SetWindowProperty failed (no window)";
+    return false;
+  }
+  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;
+  }
+  DVLOG(1) << "SetWindowProperty failed (access policy denied change)";
+  return false;
+}
+
 bool WindowServiceClient::SetWindowOpacityImpl(const ClientWindowId& window_id,
                                                float opacity) {
   aura::Window* window = GetWindowByClientId(window_id);
@@ -632,7 +659,7 @@ void WindowServiceClient::NewWindow(
     Id transport_window_id,
     const base::Optional<base::flat_map<std::string, std::vector<uint8_t>>>&
         transport_properties) {
-  // TODO: needs to map and validate |transport_properties|.
+  // TODO(crbug.com/837695): Map and validate |transport_properties|.
   std::map<std::string, std::vector<uint8_t>> properties;
 
   window_tree_client_->OnChangeCompleted(
@@ -752,7 +779,9 @@ void WindowServiceClient::SetWindowProperty(
     Id window_id,
     const std::string& name,
     const base::Optional<std::vector<uint8_t>>& value) {
-  NOTIMPLEMENTED();
+  window_tree_client_->OnChangeCompleted(
+      change_id,
+      SetWindowPropertyImpl(MakeClientWindowId(window_id), name, value));
 }
 
 void WindowServiceClient::SetWindowOpacity(uint32_t change_id,

services/ui/ws2/window_service_client.h

@@ -172,6 +172,9 @@ class COMPONENT_EXPORT(WINDOW_SERVICE) WindowServiceClient
                      const ClientWindowId& child_id);
   bool RemoveWindowFromParentImpl(const ClientWindowId& client_window_id);
   bool SetWindowVisibilityImpl(const ClientWindowId& window_id, bool visible);
+  bool SetWindowPropertyImpl(const ClientWindowId& window_id,
+                             const std::string& name,
+                             const base::Optional<std::vector<uint8_t>>& value);
   bool EmbedImpl(const ClientWindowId& window_id,
                  mojom::WindowTreeClientPtr window_tree_client,
                  uint32_t flags);

services/ui/ws2/window_service_client_test_helper.cc

@@ -34,5 +34,15 @@ void WindowServiceClientTestHelper::SetWindowBounds(aura::Window* window,
       local_surface_id);
 }
 
+void WindowServiceClientTestHelper::SetWindowProperty(
+    aura::Window* window,
+    const std::string& name,
+    const std::vector<uint8_t>& value,
+    uint32_t change_id) {
+  window_service_client_->SetWindowProperty(
+      change_id, window_service_client_->TransportIdForWindow(window), name,
+      value);
+}
+
 }  // namespace ws2
 }  // namespace ui

services/ui/ws2/window_service_client_test_helper.h

@@ -32,6 +32,10 @@ class WindowServiceClientTestHelper {
   void SetWindowBounds(aura::Window* window,
                        const gfx::Rect& bounds,
                        uint32_t change_id = 1);
+  void SetWindowProperty(aura::Window* window,
+                         const std::string& name,
+                         const std::vector<uint8_t>& value,
+                         uint32_t change_id = 1);
 
  private:
   WindowServiceClient* window_service_client_;

services/ui/ws2/window_service_client_unittest.cc

@@ -10,12 +10,15 @@
 #include <memory>
 
 #include "base/test/scoped_task_environment.h"
+#include "services/ui/public/cpp/property_type_converters.h"
+#include "services/ui/public/interfaces/window_manager.mojom.h"
 #include "services/ui/ws2/gpu_support.h"
 #include "services/ui/ws2/test_window_service_delegate.h"
 #include "services/ui/ws2/test_window_tree_client.h"
 #include "services/ui/ws2/window_service.h"
 #include "services/ui/ws2/window_service_client_test_helper.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "ui/aura/client/aura_constants.h"
 #include "ui/aura/layout_manager.h"
 #include "ui/aura/test/aura_test_helper.h"
 #include "ui/aura/window.h"
@@ -30,23 +33,36 @@ namespace {
 class WindowServiceTestHelper {
  public:
   WindowServiceTestHelper() {
+    if (gl::GetGLImplementation() == gl::kGLImplementationNone)
+      gl::GLSurfaceTestSupport::InitializeOneOff();
+
     ui::ContextFactory* context_factory = nullptr;
     ui::ContextFactoryPrivate* context_factory_private = nullptr;
     const bool enable_pixel_output = false;
-    gl::GLSurfaceTestSupport::InitializeOneOff();
     ui::InitializeContextFactoryForTests(enable_pixel_output, &context_factory,
                                          &context_factory_private);
     aura_test_helper_.SetUp(context_factory, context_factory_private);
     service_ = std::make_unique<WindowService>(&delegate_, nullptr);
-    delegate_.set_top_level_parent(root());
+    delegate_.set_top_level_parent(aura_test_helper_.root_window());
+
+    const bool intercepts_events = false;
+    window_service_client_ = service_->CreateWindowServiceClient(
+        &window_tree_client_, intercepts_events);
+    window_service_client_->InitFromFactory();
+    helper_ = std::make_unique<WindowServiceClientTestHelper>(
+        window_service_client_.get());
   }
+
   ~WindowServiceTestHelper() {
     aura_test_helper_.TearDown();
     ui::TerminateContextFactoryForTests();
   }
 
-  WindowService* service() { return service_.get(); }
-  aura::Window* root() { return aura_test_helper_.root_window(); }
+  std::vector<Change>* changes() {
+    return window_tree_client_.tracker()->changes();
+  }
+
+  std::unique_ptr<WindowServiceClientTestHelper> helper_;
 
  private:
   base::test::ScopedTaskEnvironment task_environment_{
@@ -54,6 +70,8 @@ class WindowServiceTestHelper {
   aura::test::AuraTestHelper aura_test_helper_;
   TestWindowServiceDelegate delegate_;
   std::unique_ptr<WindowService> service_;
+  TestWindowTreeClient window_tree_client_;
+  std::unique_ptr<WindowServiceClient> window_service_client_;
 
   DISALLOW_COPY_AND_ASSIGN(WindowServiceTestHelper);
 };
@@ -88,38 +106,34 @@ class TestLayoutManager : public aura::LayoutManager {
   DISALLOW_COPY_AND_ASSIGN(TestLayoutManager);
 };
 
-TEST(WindowServiceClientTest, SetBounds) {
+TEST(WindowServiceClientTest, CreateTopLevel) {
   WindowServiceTestHelper helper;
-  TestWindowTreeClient window_tree_client;
-  const bool intercepts_events = false;
-  std::unique_ptr<WindowServiceClient> window_service_client =
-      helper.service()->CreateWindowServiceClient(&window_tree_client,
-                                                  intercepts_events);
-  window_service_client->InitFromFactory();
-  std::vector<Change>* changes = window_tree_client.tracker()->changes();
-  EXPECT_TRUE(changes->empty());
-  WindowServiceClientTestHelper window_service_client_helper(
-      window_service_client.get());
-  aura::Window* top_level = window_service_client_helper.NewTopLevelWindow(1);
+  EXPECT_TRUE(helper.changes()->empty());
+  aura::Window* top_level = helper.helper_->NewTopLevelWindow(1);
   ASSERT_TRUE(top_level);
   EXPECT_EQ("TopLevelCreated id=1 window_id=0,1 drawn=false",
-            SingleChangeToDescription(*changes));
-  changes->clear();
+            SingleChangeToDescription(*helper.changes()));
+  helper.changes()->clear();
+}
+
+TEST(WindowServiceClientTest, SetBounds) {
+  WindowServiceTestHelper helper;
+  aura::Window* top_level = helper.helper_->NewTopLevelWindow(1);
+  helper.changes()->clear();
 
   const gfx::Rect bounds_from_client = gfx::Rect(1, 2, 300, 400);
-  window_service_client_helper.SetWindowBounds(top_level, bounds_from_client,
-                                               2);
+  helper.helper_->SetWindowBounds(top_level, bounds_from_client, 2);
   EXPECT_EQ(bounds_from_client, top_level->bounds());
   EXPECT_EQ("ChangeCompleted id=2 sucess=true",
-            SingleChangeToDescription(*changes));
-  changes->clear();
+            SingleChangeToDescription(*helper.changes()));
+  helper.changes()->clear();
 
   const gfx::Rect bounds_from_server = gfx::Rect(101, 102, 103, 104);
   top_level->SetBounds(bounds_from_server);
-  ASSERT_EQ(1u, changes->size());
-  EXPECT_EQ(CHANGE_TYPE_NODE_BOUNDS_CHANGED, (*changes)[0].type);
-  EXPECT_EQ(bounds_from_server, (*changes)[0].bounds2);
-  changes->clear();
+  ASSERT_EQ(1u, helper.changes()->size());
+  EXPECT_EQ(CHANGE_TYPE_NODE_BOUNDS_CHANGED, (*helper.changes())[0].type);
+  EXPECT_EQ(bounds_from_server, (*helper.changes())[0].bounds2);
+  helper.changes()->clear();
 
   // Set a LayoutManager so that when the client requests a bounds change the
   // window is resized to a different bounds.
@@ -128,19 +142,34 @@ TEST(WindowServiceClientTest, SetBounds) {
   const gfx::Rect restricted_bounds = gfx::Rect(401, 405, 406, 407);
   layout_manager->set_next_bounds(restricted_bounds);
   top_level->parent()->SetLayoutManager(layout_manager);
-  window_service_client_helper.SetWindowBounds(top_level, bounds_from_client,
-                                               3);
-  ASSERT_EQ(2u, changes->size());
+  helper.helper_->SetWindowBounds(top_level, bounds_from_client, 3);
+  ASSERT_EQ(2u, helper.changes()->size());
   // The layout manager changes the bounds to a different value than the client
   // requested, so the client should get OnWindowBoundsChanged() with
   // |restricted_bounds|.
-  EXPECT_EQ(CHANGE_TYPE_NODE_BOUNDS_CHANGED, (*changes)[0].type);
-  EXPECT_EQ(restricted_bounds, (*changes)[0].bounds2);
+  EXPECT_EQ(CHANGE_TYPE_NODE_BOUNDS_CHANGED, (*helper.changes())[0].type);
+  EXPECT_EQ(restricted_bounds, (*helper.changes())[0].bounds2);
 
   // And because the layout manager changed the bounds the result is false.
   EXPECT_EQ("ChangeCompleted id=3 sucess=false",
-            ChangeToDescription((*changes)[1]));
-  changes->clear();
+            ChangeToDescription((*helper.changes())[1]));
+  helper.changes()->clear();
+}
+
+TEST(WindowServiceClientTest, SetProperty) {
+  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);
+  helper.helper_->SetWindowProperty(
+      top_level, ui::mojom::WindowManager::kAlwaysOnTop_Property, transport, 2);
+  EXPECT_EQ("ChangeCompleted id=2 sucess=true",
+            SingleChangeToDescription(*helper.changes()));
+  EXPECT_TRUE(top_level->GetProperty(aura::client::kAlwaysOnTopKey));
+  helper.changes()->clear();
 }
 
 }  // namespace

ui/aura/client/aura_constants.cc

@@ -64,8 +64,8 @@ DEFINE_UI_CLASS_PROPERTY_KEY(
 DEFINE_UI_CLASS_PROPERTY_KEY(
     ui::WindowShowState, kPreFullscreenShowStateKey, ui::SHOW_STATE_DEFAULT);
 DEFINE_UI_CLASS_PROPERTY_KEY(int32_t,
-                           kResizeBehaviorKey,
-                           ui::mojom::kResizeBehaviorCanResize);
+                             kResizeBehaviorKey,
+                             ui::mojom::kResizeBehaviorCanResize);
 DEFINE_OWNED_UI_CLASS_PROPERTY_KEY(gfx::Rect, kRestoreBoundsKey, nullptr);
 DEFINE_UI_CLASS_PROPERTY_KEY(
     ui::WindowShowState, kShowStateKey, ui::SHOW_STATE_DEFAULT);

ui/aura/mus/property_converter.cc

@@ -305,6 +305,8 @@ bool PropertyConverter::GetPropertyValueFromTransportValue(
 void PropertyConverter::RegisterImageSkiaProperty(
     const WindowProperty<gfx::ImageSkia*>* property,
     const char* transport_name) {
+  DCHECK(!IsTransportNameRegistered(transport_name))
+      << "Property already registered: " << transport_name;
   image_properties_[property] = transport_name;
   transport_names_.insert(transport_name);
 }
@@ -312,6 +314,8 @@ void PropertyConverter::RegisterImageSkiaProperty(
 void PropertyConverter::RegisterRectProperty(
     const WindowProperty<gfx::Rect*>* property,
     const char* transport_name) {
+  DCHECK(!IsTransportNameRegistered(transport_name))
+      << "Property already registered: " << transport_name;
   rect_properties_[property] = transport_name;
   transport_names_.insert(transport_name);
 }
@@ -319,6 +323,8 @@ void PropertyConverter::RegisterRectProperty(
 void PropertyConverter::RegisterSizeProperty(
     const WindowProperty<gfx::Size*>* property,
     const char* transport_name) {
+  DCHECK(!IsTransportNameRegistered(transport_name))
+      << "Property already registered: " << transport_name;
   size_properties_[property] = transport_name;
   transport_names_.insert(transport_name);
 }
@@ -326,6 +332,8 @@ void PropertyConverter::RegisterSizeProperty(
 void PropertyConverter::RegisterStringProperty(
     const WindowProperty<std::string*>* property,
     const char* transport_name) {
+  DCHECK(!IsTransportNameRegistered(transport_name))
+      << "Property already registered: " << transport_name;
   string_properties_[property] = transport_name;
   transport_names_.insert(transport_name);
 }
@@ -333,6 +341,8 @@ void PropertyConverter::RegisterStringProperty(
 void PropertyConverter::RegisterString16Property(
     const WindowProperty<base::string16*>* property,
     const char* transport_name) {
+  DCHECK(!IsTransportNameRegistered(transport_name))
+      << "Property already registered: " << transport_name;
   string16_properties_[property] = transport_name;
   transport_names_.insert(transport_name);
 }

ui/aura/mus/property_converter.h

@@ -83,6 +83,8 @@ class AURA_EXPORT PropertyConverter {
       const WindowProperty<T>* property,
       const char* transport_name,
       const base::RepeatingCallback<bool(int64_t)>& validator) {
+    DCHECK(!IsTransportNameRegistered(transport_name))
+        << "Property already registered: " << transport_name;
     PrimitiveProperty primitive_property;
     primitive_property.property_name = property->name;
     primitive_property.transport_name = transport_name;

ui/views/mus/desktop_window_tree_host_mus_unittest.cc

@@ -7,6 +7,7 @@
 #include "base/debug/stack_trace.h"
 #include "base/run_loop.h"
 
+#include "ui/aura/client/aura_constants.h"
 #include "ui/aura/client/cursor_client.h"
 #include "ui/aura/client/focus_client.h"
 #include "ui/aura/client/transient_window_client.h"

ui/views/mus/mus_client.cc

@@ -92,7 +92,6 @@ MusClient::MusClient(service_manager::Connector* connector,
     io_task_runner = io_thread_->task_runner();
   }
 
-  // TODO(msw): Avoid this... use some default value? Allow clients to extend?
   property_converter_ = std::make_unique<aura::PropertyConverter>();
   property_converter_->RegisterPrimitiveProperty(
       ::wm::kShadowElevationKey,

ui/wm/core/shadow_controller.cc

@@ -189,9 +189,8 @@ void ShadowController::Impl::OnWindowInitialized(aura::Window* window) {
 void ShadowController::Impl::OnWindowPropertyChanged(aura::Window* window,
                                                      const void* key,
                                                      intptr_t old) {
-  bool shadow_will_change = false;
-  if (key == kShadowElevationKey)
-    shadow_will_change = window->GetProperty(kShadowElevationKey) != old;
+  bool shadow_will_change = key == kShadowElevationKey &&
+                            window->GetProperty(kShadowElevationKey) != old;
 
   if (key == aura::client::kShowStateKey) {
     shadow_will_change = window->GetProperty(aura::client::kShowStateKey) !=