Gets WindowTree::SetBounds() working correctly

General logic for changes from client is not to notify client back of
changes. For example, if WindowTree::SetBounds() succeeds it should not call
WindowTreeClient::OnWindowBoundsChanged.

Refactors some test code for sharing and adds test coverage.

BUG=none
TEST=covered by tests

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

services/ui/ws2/BUILD.gn

@@ -11,6 +11,11 @@ import("//services/service_manager/public/tools/test/service_test.gni")
 
 component("lib") {
   sources = [
+    "client_change.cc",
+    "client_change.h",
+    "client_change_tracker.cc",
+    "client_change_tracker.h",
+
     # TODO: client_root should be internal and moved to a different target.
     "client_root.cc",
     "client_root.h",
@@ -48,11 +53,18 @@ static_library("test_support") {
   sources = [
     "test_change_tracker.cc",
     "test_change_tracker.h",
+    "test_window_service_delegate.cc",
+    "test_window_service_delegate.h",
+    "test_window_tree_client.cc",
+    "test_window_tree_client.h",
     "window_server_service_test_base.cc",
     "window_server_service_test_base.h",
+    "window_service_client_test_helper.cc",
+    "window_service_client_test_helper.h",
   ]
 
   deps = [
+    ":lib",
     "//base",
     "//base/test:test_config",
     "//components/viz/test:test_support",
@@ -91,6 +103,7 @@ source_set("tests") {
   testonly = true
 
   sources = [
+    "window_service_client_unittest.cc",
     "window_tree_client_unittest.cc",
   ]
 
@@ -98,6 +111,7 @@ source_set("tests") {
     ":lib",
     ":test_support",
     "//base",
+    "//base/test:test_support",
     "//components/viz/common",
     "//mojo/public/cpp/bindings",
     "//services/service_manager/public/cpp:service_test_support",
@@ -105,6 +119,9 @@ source_set("tests") {
     "//services/ui/public/interfaces",
     "//testing/gtest",
     "//third_party/mesa:osmesa",
+    "//ui/aura:test_support",
+    "//ui/compositor:test_support",
+    "//ui/gl:test_support",
   ]
 
   data_deps = [

services/ui/ws2/client_change.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/ui/ws2/client_change.h"
+
+#include <utility>
+
+#include "services/ui/ws2/client_change_tracker.h"
+#include "ui/aura/window.h"
+
+namespace ui {
+namespace ws2 {
+
+ClientChange::ClientChange(ClientChangeTracker* tracker,
+                           aura::Window* window,
+                           ClientChangeType type)
+    : tracker_(tracker), type_(type) {
+  DCHECK(!tracker_->current_change_);
+  tracker_->current_change_ = this;
+  window_tracker_.Add(window);
+}
+
+ClientChange::~ClientChange() {
+  DCHECK_EQ(this, tracker_->current_change_);
+  tracker_->current_change_ = nullptr;
+}
+
+}  // namespace ws2
+}  // namespace ui

services/ui/ws2/client_change.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 SERVICES_UI_WS2_CLIENT_CHANGE_H_
+#define SERVICES_UI_WS2_CLIENT_CHANGE_H_
+
+#include <stdint.h>
+
+#include <memory>
+
+#include "base/component_export.h"
+#include "base/macros.h"
+#include "ui/aura/window_tracker.h"
+
+namespace aura {
+class Window;
+}
+
+namespace ui {
+namespace ws2 {
+
+class ClientChangeTracker;
+
+// Describes the type of the change. Maps to the incoming change from the
+// client.
+enum class ClientChangeType {
+  // Used for WindowTree::SetWindowBounds().
+  kBounds,
+};
+
+// ClientChange represents an incoming request from a WindowTreeClient. For
+// example, SetWindowBounds() is a request to change the kBounds property of
+// the window.
+class COMPONENT_EXPORT(WINDOW_SERVICE) ClientChange {
+ public:
+  ClientChange(ClientChangeTracker* tracker,
+               aura::Window* window,
+               ClientChangeType type);
+  ~ClientChange();
+
+  // The window the changes associated with. Is null if the window has been
+  // destroyed during processing.
+  aura::Window* window() {
+    return !window_tracker_.windows().empty() ? window_tracker_.windows()[0]
+                                              : nullptr;
+  }
+
+  ClientChangeType type() const { return type_; }
+
+ private:
+  ClientChangeTracker* tracker_;
+  aura::WindowTracker window_tracker_;
+  const ClientChangeType type_;
+
+  DISALLOW_COPY_AND_ASSIGN(ClientChange);
+};
+
+}  // namespace ws2
+}  // namespace ui
+
+#endif  // SERVICES_UI_WS2_CLIENT_CHANGE_H_

services/ui/ws2/client_change_tracker.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/ui/ws2/client_change_tracker.h"
+
+#include "services/ui/ws2/client_change.h"
+
+namespace ui {
+namespace ws2 {
+
+ClientChangeTracker::ClientChangeTracker() = default;
+
+ClientChangeTracker::~ClientChangeTracker() = default;
+
+bool ClientChangeTracker::IsProcessingChangeForWindow(aura::Window* window,
+                                                      ClientChangeType type) {
+  return current_change_ && current_change_->window() == window &&
+         current_change_->type() == type;
+}
+
+}  // namespace ws2
+}  // namespace ui

services/ui/ws2/client_change_tracker.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 SERVICES_UI_WS2_CLIENT_CHANGE_TRACKER_H_
+#define SERVICES_UI_WS2_CLIENT_CHANGE_TRACKER_H_
+
+#include <stdint.h>
+
+#include <memory>
+
+#include "base/component_export.h"
+#include "base/macros.h"
+
+namespace aura {
+class Window;
+}
+
+namespace ui {
+namespace ws2 {
+
+class ClientChange;
+
+enum class ClientChangeType;
+
+// Tracks the active change from the client. There is at most one change from a
+// client at a time.
+class COMPONENT_EXPORT(WINDOW_SERVICE) ClientChangeTracker {
+ public:
+  ClientChangeTracker();
+  ~ClientChangeTracker();
+
+  bool IsProcessingChangeForWindow(aura::Window* window, ClientChangeType type);
+
+ private:
+  friend class ClientChange;
+
+  // Owned by the caller that created the ClientChange. This is set in
+  // ClientChange's constructor and reset in the destructor.
+  ClientChange* current_change_ = nullptr;
+
+  DISALLOW_COPY_AND_ASSIGN(ClientChangeTracker);
+};
+
+}  // namespace ws2
+}  // namespace ui
+
+#endif  // SERVICES_UI_WS2_CLIENT_CHANGE_TRACKER_H_

services/ui/ws2/client_root.cc

@@ -4,6 +4,8 @@
 
 #include "services/ui/ws2/client_root.h"
 
+#include "services/ui/ws2/client_change.h"
+#include "services/ui/ws2/client_change_tracker.h"
 #include "services/ui/ws2/window_data.h"
 #include "services/ui/ws2/window_service_client.h"
 #include "ui/aura/mus/client_surface_embedder.h"
@@ -70,6 +72,12 @@ void ClientRoot::OnWindowBoundsChanged(aura::Window* window,
   UpdatePrimarySurfaceId();
   client_surface_embedder_->UpdateSizeAndGutters();
   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.
+    return;
+  }
   window_service_client_->window_tree_client_->OnWindowBoundsChanged(
       window_service_client_->TransportIdForWindow(window), old_bounds,
       new_bounds, std::move(surface_id));

services/ui/ws2/test_change_tracker.cc

@@ -28,12 +28,7 @@ std::string DirectionToString(mojom::OrderDirection direction) {
   return direction == mojom::OrderDirection::ABOVE ? "above" : "below";
 }
 
-enum class ChangeDescriptionType {
-  ONE,
-  TWO,
-  // Includes display id and location of events.
-  THREE,
-};
+enum class ChangeDescriptionType { ONE, TWO };
 
 std::string ChangeToDescription(const Change& change,
                                 ChangeDescriptionType type) {
@@ -175,6 +170,10 @@ std::string SingleChangeToDescriptionImpl(const std::vector<Change>& changes,
 
 }  // namespace
 
+std::string ChangeToDescription(const Change& change) {
+  return ChangeToDescription(change, ChangeDescriptionType::ONE);
+}
+
 std::vector<std::string> ChangesToDescription1(
     const std::vector<Change>& changes) {
   std::vector<std::string> strings(changes.size());

services/ui/ws2/test_change_tracker.h

@@ -103,6 +103,15 @@ struct Change {
   gfx::PointF location2;
 };
 
+// The ChangeToDescription related functions convert a Change into a string.
+// To avoid updating all tests as more descriptive strings are added, new
+// variants are added and identified with a numeric suffix. Differences
+// between versions:
+// 1 and no suffix is the original version.
+// 2: OnEmbed() includes the boolean value supplied to OnEmbed().
+
+std::string ChangeToDescription(const Change& change);
+
 // Converts Changes to string descriptions.
 std::vector<std::string> ChangesToDescription1(
     const std::vector<Change>& changes);
@@ -110,6 +119,7 @@ std::vector<std::string> ChangesToDescription1(
 // Convenience for returning the description of the first item in |changes|.
 // Returns an empty string if |changes| has something other than one entry.
 std::string SingleChangeToDescription(const std::vector<Change>& changes);
+
 std::string SingleChangeToDescription2(const std::vector<Change>& changes);
 
 // Convenience for returning the description of the first item in |windows|.

services/ui/ws2/test_window_service_delegate.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/ui/ws2/test_window_service_delegate.h"
+
+#include "ui/aura/window.h"
+
+namespace ui {
+namespace ws2 {
+
+TestWindowServiceDelegate::TestWindowServiceDelegate(
+    aura::Window* top_level_parent)
+    : top_level_parent_(top_level_parent) {}
+
+TestWindowServiceDelegate::~TestWindowServiceDelegate() = default;
+
+std::unique_ptr<aura::Window> TestWindowServiceDelegate::NewTopLevel(
+    const std::unordered_map<std::string, std::vector<uint8_t>>& properties) {
+  std::unique_ptr<aura::Window> window =
+      std::make_unique<aura::Window>(nullptr);
+  window->Init(LAYER_NOT_DRAWN);
+  if (top_level_parent_)
+    top_level_parent_->AddChild(window.get());
+  return window;
+}
+
+}  // namespace ws2
+}  // namespace ui

services/ui/ws2/test_window_service_delegate.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 SERVICES_UI_WS2_TEST_WINDOW_SERVICE_DELEGATE_H_
+#define SERVICES_UI_WS2_TEST_WINDOW_SERVICE_DELEGATE_H_
+
+#include "base/macros.h"
+#include "services/ui/ws2/window_service_delegate.h"
+
+namespace ui {
+namespace ws2 {
+
+class TestWindowServiceDelegate : public WindowServiceDelegate {
+ public:
+  // |top_level_parent| is the parent of new top-levels. If null, top-levels
+  // have no parent.
+  explicit TestWindowServiceDelegate(aura::Window* top_level_parent = nullptr);
+  ~TestWindowServiceDelegate() override;
+
+  void set_top_level_parent(aura::Window* parent) {
+    top_level_parent_ = parent;
+  }
+
+  // WindowServiceDelegate:
+  std::unique_ptr<aura::Window> NewTopLevel(
+      const std::unordered_map<std::string, std::vector<uint8_t>>& properties)
+      override;
+
+ private:
+  aura::Window* top_level_parent_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestWindowServiceDelegate);
+};
+
+}  // namespace ws2
+}  // namespace ui
+
+#endif  // SERVICES_UI_WS2_TEST_WINDOW_SERVICE_DELEGATE_H_

services/ui/ws2/test_window_tree_client.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/ui/ws2/test_window_tree_client.h"
+
+#include <utility>
+
+#include "ui/aura/window.h"
+#include "ui/gfx/geometry/rect.h"
+
+namespace ui {
+namespace ws2 {
+
+TestWindowTreeClient::TestWindowTreeClient() {
+  tracker_.set_delegate(this);
+}
+
+TestWindowTreeClient::~TestWindowTreeClient() = default;
+
+void TestWindowTreeClient::OnChangeAdded() {}
+
+void TestWindowTreeClient::OnEmbed(
+    mojom::WindowDataPtr root,
+    mojom::WindowTreePtr tree,
+    int64_t display_id,
+    Id focused_window_id,
+    bool drawn,
+    const base::Optional<viz::LocalSurfaceId>& local_surface_id) {
+  root_window_id_ = root->window_id;
+  tree_ = std::move(tree);
+  tracker_.OnEmbed(std::move(root), drawn);
+}
+
+void TestWindowTreeClient::OnEmbedFromToken(
+    const base::UnguessableToken& token,
+    mojom::WindowDataPtr root,
+    int64_t display_id,
+    const base::Optional<viz::LocalSurfaceId>& local_surface_id) {}
+
+void TestWindowTreeClient::OnEmbeddedAppDisconnected(Id window_id) {
+  tracker_.OnEmbeddedAppDisconnected(window_id);
+}
+
+void TestWindowTreeClient::OnUnembed(Id window_id) {
+  tracker_.OnUnembed(window_id);
+}
+
+void TestWindowTreeClient::OnCaptureChanged(Id new_capture_window_id,
+                                            Id old_capture_window_id) {
+  tracker_.OnCaptureChanged(new_capture_window_id, old_capture_window_id);
+}
+
+void TestWindowTreeClient::OnFrameSinkIdAllocated(
+    Id window_id,
+    const viz::FrameSinkId& frame_sink_id) {}
+
+void TestWindowTreeClient::OnTopLevelCreated(
+    uint32_t change_id,
+    mojom::WindowDataPtr data,
+    int64_t display_id,
+    bool drawn,
+    const base::Optional<viz::LocalSurfaceId>& local_surface_id) {
+  tracker_.OnTopLevelCreated(change_id, std::move(data), drawn);
+}
+
+void TestWindowTreeClient::OnWindowBoundsChanged(
+    Id window_id,
+    const gfx::Rect& old_bounds,
+    const gfx::Rect& new_bounds,
+    const base::Optional<viz::LocalSurfaceId>& local_surface_id) {
+  // The bounds of the root may change during startup on Android at random
+  // times. As this doesn't matter, and shouldn't impact test exepctations,
+  // it is ignored.
+  if (window_id == root_window_id_ && !track_root_bounds_changes_)
+    return;
+  tracker_.OnWindowBoundsChanged(window_id, old_bounds, new_bounds,
+                                 local_surface_id);
+}
+
+void TestWindowTreeClient::OnWindowTransformChanged(
+    Id window_id,
+    const gfx::Transform& old_transform,
+    const gfx::Transform& new_transform) {
+  tracker_.OnWindowTransformChanged(window_id);
+}
+
+void TestWindowTreeClient::OnClientAreaChanged(
+    Id window_id,
+    const gfx::Insets& new_client_area,
+    const std::vector<gfx::Rect>& new_additional_client_areas) {}
+
+void TestWindowTreeClient::OnTransientWindowAdded(Id window_id,
+                                                  Id transient_window_id) {
+  tracker_.OnTransientWindowAdded(window_id, transient_window_id);
+}
+
+void TestWindowTreeClient::OnTransientWindowRemoved(Id window_id,
+                                                    Id transient_window_id) {
+  tracker_.OnTransientWindowRemoved(window_id, transient_window_id);
+}
+
+void TestWindowTreeClient::OnWindowHierarchyChanged(
+    Id window,
+    Id old_parent,
+    Id new_parent,
+    std::vector<mojom::WindowDataPtr> windows) {
+  tracker_.OnWindowHierarchyChanged(window, old_parent, new_parent,
+                                    std::move(windows));
+}
+
+void TestWindowTreeClient::OnWindowReordered(Id window_id,
+                                             Id relative_window_id,
+                                             mojom::OrderDirection direction) {
+  tracker_.OnWindowReordered(window_id, relative_window_id, direction);
+}
+
+void TestWindowTreeClient::OnWindowDeleted(Id window) {
+  tracker_.OnWindowDeleted(window);
+}
+
+void TestWindowTreeClient::OnWindowVisibilityChanged(Id window, bool visible) {
+  tracker_.OnWindowVisibilityChanged(window, visible);
+}
+
+void TestWindowTreeClient::OnWindowOpacityChanged(Id window,
+                                                  float old_opacity,
+                                                  float new_opacity) {
+  tracker_.OnWindowOpacityChanged(window, new_opacity);
+}
+
+void TestWindowTreeClient::OnWindowParentDrawnStateChanged(Id window,
+                                                           bool drawn) {
+  tracker_.OnWindowParentDrawnStateChanged(window, drawn);
+}
+
+void TestWindowTreeClient::OnWindowInputEvent(
+    uint32_t event_id,
+    Id window_id,
+    int64_t display_id,
+    Id display_root_window_id,
+    const gfx::PointF& event_location_in_screen_pixel_layout,
+    std::unique_ptr<ui::Event> event,
+    bool matches_pointer_watcher) {
+  tree_->OnWindowInputEventAck(event_id, mojom::EventResult::HANDLED);
+}
+
+void TestWindowTreeClient::OnPointerEventObserved(std::unique_ptr<ui::Event>,
+                                                  Id window_id,
+                                                  int64_t display_id) {}
+
+void TestWindowTreeClient::OnWindowSharedPropertyChanged(
+    Id window,
+    const std::string& name,
+    const base::Optional<std::vector<uint8_t>>& new_data) {
+  tracker_.OnWindowSharedPropertyChanged(window, name, new_data);
+}
+
+void TestWindowTreeClient::OnWindowFocused(Id focused_window_id) {}
+
+void TestWindowTreeClient::OnWindowCursorChanged(Id window_id,
+                                                 ui::CursorData cursor) {
+  tracker_.OnWindowCursorChanged(window_id, cursor);
+}
+
+void TestWindowTreeClient::OnDragDropStart(
+    const std::unordered_map<std::string, std::vector<uint8_t>>& drag_data) {}
+
+void TestWindowTreeClient::OnWindowSurfaceChanged(
+    Id window_id,
+    const viz::SurfaceInfo& surface_info) {
+  tracker_.OnWindowSurfaceChanged(window_id, surface_info);
+}
+
+void TestWindowTreeClient::OnDragEnter(Id window,
+                                       uint32_t key_state,
+                                       const gfx::Point& position,
+                                       uint32_t effect_bitmask,
+                                       OnDragEnterCallback callback) {}
+
+void TestWindowTreeClient::OnDragOver(Id window,
+                                      uint32_t key_state,
+                                      const gfx::Point& position,
+                                      uint32_t effect_bitmask,
+                                      OnDragOverCallback callback) {}
+
+void TestWindowTreeClient::OnDragLeave(Id window) {}
+
+void TestWindowTreeClient::OnCompleteDrop(Id window,
+                                          uint32_t key_state,
+                                          const gfx::Point& position,
+                                          uint32_t effect_bitmask,
+                                          OnCompleteDropCallback callback) {}
+
+void TestWindowTreeClient::OnPerformDragDropCompleted(uint32_t change_id,
+                                                      bool success,
+                                                      uint32_t action_taken) {}
+
+void TestWindowTreeClient::OnDragDropDone() {}
+
+void TestWindowTreeClient::OnChangeCompleted(uint32_t change_id, bool success) {
+  tracker_.OnChangeCompleted(change_id, success);
+}
+
+void TestWindowTreeClient::RequestClose(Id window_id) {}
+
+void TestWindowTreeClient::GetWindowManager(
+    mojo::AssociatedInterfaceRequest<mojom::WindowManager> internal) {}
+
+}  // namespace ws2
+}  // namespace ui

services/ui/ws2/test_window_tree_client.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 SERVICES_UI_WS2_TEST_WINDOW_TREE_CLIENT_H_
+#define SERVICES_UI_WS2_TEST_WINDOW_TREE_CLIENT_H_
+
+#include "base/component_export.h"
+#include "base/macros.h"
+#include "services/ui/public/interfaces/window_tree.mojom.h"
+#include "services/ui/ws2/test_change_tracker.h"
+
+namespace ui {
+namespace ws2 {
+
+// WindowTreeClient implementation that logs all changes to a tracker.
+class TestWindowTreeClient : public mojom::WindowTreeClient,
+                             public TestChangeTracker::Delegate {
+ public:
+  TestWindowTreeClient();
+  ~TestWindowTreeClient() override;
+
+  mojom::WindowTree* tree() { return tree_.get(); }
+  TestChangeTracker* tracker() { return &tracker_; }
+  Id root_window_id() const { return root_window_id_; }
+
+  // Sets whether changes to the bounds of the root should be tracked. Normally
+  // they are ignored (as during startup we often times get random size
+  // changes).
+  void set_track_root_bounds_changes(bool value) {
+    track_root_bounds_changes_ = value;
+  }
+
+  // TestChangeTracker::Delegate:
+  void OnChangeAdded() override;
+
+  // mojom::WindowTreeClient:
+  void OnEmbed(
+      mojom::WindowDataPtr root,
+      mojom::WindowTreePtr tree,
+      int64_t display_id,
+      Id focused_window_id,
+      bool drawn,
+      const base::Optional<viz::LocalSurfaceId>& local_surface_id) override;
+  void OnEmbedFromToken(
+      const base::UnguessableToken& token,
+      mojom::WindowDataPtr root,
+      int64_t display_id,
+      const base::Optional<viz::LocalSurfaceId>& local_surface_id) override;
+  void OnEmbeddedAppDisconnected(Id window_id) override;
+  void OnUnembed(Id window_id) override;
+  void OnCaptureChanged(Id new_capture_window_id,
+                        Id old_capture_window_id) override;
+  void OnFrameSinkIdAllocated(Id window_id,
+                              const viz::FrameSinkId& frame_sink_id) override;
+  void OnTopLevelCreated(
+      uint32_t change_id,
+      mojom::WindowDataPtr data,
+      int64_t display_id,
+      bool drawn,
+      const base::Optional<viz::LocalSurfaceId>& local_surface_id) override;
+  void OnWindowBoundsChanged(
+      Id window_id,
+      const gfx::Rect& old_bounds,
+      const gfx::Rect& new_bounds,
+      const base::Optional<viz::LocalSurfaceId>& local_surface_id) override;
+  void OnWindowTransformChanged(Id window_id,
+                                const gfx::Transform& old_transform,
+                                const gfx::Transform& new_transform) override;
+  void OnClientAreaChanged(
+      Id window_id,
+      const gfx::Insets& new_client_area,
+      const std::vector<gfx::Rect>& new_additional_client_areas) override;
+  void OnTransientWindowAdded(Id window_id, Id transient_window_id) override;
+  void OnTransientWindowRemoved(Id window_id, Id transient_window_id) override;
+  void OnWindowHierarchyChanged(
+      Id window,
+      Id old_parent,
+      Id new_parent,
+      std::vector<mojom::WindowDataPtr> windows) override;
+  void OnWindowReordered(Id window_id,
+                         Id relative_window_id,
+                         mojom::OrderDirection direction) override;
+  void OnWindowDeleted(Id window) override;
+  void OnWindowVisibilityChanged(Id window, bool visible) override;
+  void OnWindowOpacityChanged(Id window,
+                              float old_opacity,
+                              float new_opacity) override;
+  void OnWindowParentDrawnStateChanged(Id window, bool drawn) override;
+  void OnWindowInputEvent(
+      uint32_t event_id,
+      Id window_id,
+      int64_t display_id,
+      Id display_root_window_id,
+      const gfx::PointF& event_location_in_screen_pixel_layout,
+      std::unique_ptr<ui::Event> event,
+      bool matches_pointer_watcher) override;
+  void OnPointerEventObserved(std::unique_ptr<ui::Event>,
+                              Id window_id,
+                              int64_t display_id) override;
+  void OnWindowSharedPropertyChanged(
+      Id window,
+      const std::string& name,
+      const base::Optional<std::vector<uint8_t>>& new_data) override;
+  void OnWindowFocused(Id focused_window_id) override;
+  void OnWindowCursorChanged(Id window_id, ui::CursorData cursor) override;
+  void OnDragDropStart(
+      const std::unordered_map<std::string, std::vector<uint8_t>>& drag_data)
+      override;
+  void OnWindowSurfaceChanged(Id window_id,
+                              const viz::SurfaceInfo& surface_info) override;
+  void OnDragEnter(Id window,
+                   uint32_t key_state,
+                   const gfx::Point& position,
+                   uint32_t effect_bitmask,
+                   OnDragEnterCallback callback) override;
+  void OnDragOver(Id window,
+                  uint32_t key_state,
+                  const gfx::Point& position,
+                  uint32_t effect_bitmask,
+                  OnDragOverCallback callback) override;
+  void OnDragLeave(Id window) override;
+  void OnCompleteDrop(Id window,
+                      uint32_t key_state,
+                      const gfx::Point& position,
+                      uint32_t effect_bitmask,
+                      OnCompleteDropCallback callback) override;
+  void OnPerformDragDropCompleted(uint32_t change_id,
+                                  bool success,
+                                  uint32_t action_taken) override;
+  void OnDragDropDone() override;
+  void OnChangeCompleted(uint32_t change_id, bool success) override;
+  void RequestClose(Id window_id) override;
+  void GetWindowManager(
+      mojo::AssociatedInterfaceRequest<mojom::WindowManager> internal) override;
+
+ protected:
+  TestChangeTracker tracker_;
+  mojom::WindowTreePtr tree_;
+  Id root_window_id_ = 0;
+  bool track_root_bounds_changes_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(TestWindowTreeClient);
+};
+
+}  // namespace ws2
+}  // namespace ui
+
+#endif  // SERVICES_UI_WS2_TEST_WINDOW_TREE_CLIENT_H_

services/ui/ws2/window_service_client.cc

@@ -9,6 +9,8 @@
 #include "components/viz/common/surfaces/surface_info.h"
 #include "mojo/public/cpp/bindings/map.h"
 #include "services/ui/common/util.h"
+#include "services/ui/ws2/client_change.h"
+#include "services/ui/ws2/client_change_tracker.h"
 #include "services/ui/ws2/client_root.h"
 #include "services/ui/ws2/window_data.h"
 #include "services/ui/ws2/window_service.h"
@@ -32,7 +34,8 @@ WindowServiceClient::WindowServiceClient(WindowService* window_service,
     : window_service_(window_service),
       client_id_(client_id),
       window_tree_client_(client),
-      intercepts_events_(intercepts_events) {}
+      intercepts_events_(intercepts_events),
+      property_change_tracker_(std::make_unique<ClientChangeTracker>()) {}
 
 void WindowServiceClient::InitForEmbed(aura::Window* root,
                                        mojom::WindowTreePtr window_tree_ptr) {
@@ -149,7 +152,7 @@ void WindowServiceClient::DeleteClientRoot(ClientRoot* client_root,
   std::vector<aura::Window*> created_windows;
   RemoveWindowFromKnownWindowsRecursive(window, &created_windows);
   for (aura::Window* created_window : created_windows) {
-    if (created_window != window)
+    if (created_window != window && created_window->parent())
       created_window->parent()->RemoveChild(created_window);
   }
 
@@ -343,7 +346,9 @@ void WindowServiceClient::OnWillBecomeClientRootWindow(aura::Window* window) {
     DCHECK(!window_data->embedded_window_service_client());
   }
 
-  // When embedding there should be no children.
+  // Because a new client is being embedded all existing children are removed.
+  // This is because this client is no longer able to add children to |window|
+  // (until the embedding is removed).
   while (!window->children().empty())
     window->RemoveChild(window->children().front());
 }
@@ -507,8 +512,26 @@ bool WindowServiceClient::SetWindowBoundsImpl(
     return false;
   }
 
+  ClientChange change(property_change_tracker_.get(), window,
+                      ClientChangeType::kBounds);
+  const gfx::Rect original_bounds = window->bounds();
   window->SetBounds(bounds);
+  if (!change.window())
+    return true;  // Return value doesn't matter if window destroyed.
+
+  if (change.window()->bounds() == bounds)
     return true;
+
+  if (window->bounds() == original_bounds)
+    return false;
+
+  // The window's bounds changed, but not to the value the client requested.
+  // Tell the client the new value, and return false, which triggers the client
+  // to use the value supplied to OnWindowBoundsChanged().
+  window_tree_client_->OnWindowBoundsChanged(TransportIdForWindow(window),
+                                             original_bounds, window->bounds(),
+                                             local_surface_id);
+  return false;
 }
 
 bool WindowServiceClient::EmbedImpl(
@@ -643,6 +666,7 @@ void WindowServiceClient::NewTopLevelWindow(
     window_tree_client_->OnChangeCompleted(change_id, false);
     return;
   }
+  top_level_ptr->set_owned_by_parent(false);
   aura::Window* top_level =
       AddClientCreatedWindow(client_window_id, std::move(top_level_ptr));
   WindowData::GetMayBeNull(top_level)->SetFrameSinkId(client_window_id);

services/ui/ws2/window_service_client.h

@@ -23,6 +23,7 @@ class Window;
 namespace ui {
 namespace ws2 {
 
+class ClientChangeTracker;
 class ClientRoot;
 class WindowService;
 class WindowServiceClientBinding;
@@ -64,6 +65,7 @@ class COMPONENT_EXPORT(WINDOW_SERVICE) WindowServiceClient
 
  private:
   friend class ClientRoot;
+  friend class WindowServiceClientTestHelper;
 
   using ClientRoots = std::vector<std::unique_ptr<ClientRoot>>;
 
@@ -346,6 +348,9 @@ class COMPONENT_EXPORT(WINDOW_SERVICE) WindowServiceClient
   std::vector<std::unique_ptr<WindowServiceClientBinding>>
       embedded_client_bindings_;
 
+  // Used to track the active change from the client.
+  std::unique_ptr<ClientChangeTracker> property_change_tracker_;
+
   DISALLOW_COPY_AND_ASSIGN(WindowServiceClient);
 };
 

services/ui/ws2/window_service_client_test_helper.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/ui/ws2/window_service_client_test_helper.h"
+
+#include "services/ui/ws2/window_service_client.h"
+
+namespace ui {
+namespace ws2 {
+
+WindowServiceClientTestHelper::WindowServiceClientTestHelper(
+    WindowServiceClient* window_service_client)
+    : window_service_client_(window_service_client) {}
+
+WindowServiceClientTestHelper::~WindowServiceClientTestHelper() = default;
+
+aura::Window* WindowServiceClientTestHelper::NewTopLevelWindow(
+    Id transport_window_id) {
+  std::unordered_map<std::string, std::vector<uint8_t>> properties;
+  const uint32_t change_id = 1u;
+  window_service_client_->NewTopLevelWindow(change_id, transport_window_id,
+                                            properties);
+  return window_service_client_->GetWindowByClientId(
+      window_service_client_->MakeClientWindowId(transport_window_id));
+}
+
+void WindowServiceClientTestHelper::SetWindowBounds(aura::Window* window,
+                                                    const gfx::Rect& bounds,
+                                                    uint32_t change_id) {
+  base::Optional<viz::LocalSurfaceId> local_surface_id;
+  window_service_client_->SetWindowBounds(
+      change_id, window_service_client_->TransportIdForWindow(window), bounds,
+      local_surface_id);
+}
+
+}  // namespace ws2
+}  // namespace ui

services/ui/ws2/window_service_client_test_helper.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 SERVICES_UI_WS2_WINDOW_SERVICE_CLIENT_TEST_HELPER_H_
+#define SERVICES_UI_WS2_WINDOW_SERVICE_CLIENT_TEST_HELPER_H_
+
+#include "base/macros.h"
+#include "services/ui/ws2/ids.h"
+
+namespace aura {
+class Window;
+}
+
+namespace gfx {
+class Rect;
+}
+
+namespace ui {
+namespace ws2 {
+
+class WindowServiceClient;
+
+// Used for accessing private members of WindowServiceClient in tests.
+class WindowServiceClientTestHelper {
+ public:
+  explicit WindowServiceClientTestHelper(
+      WindowServiceClient* window_service_client);
+  ~WindowServiceClientTestHelper();
+
+  aura::Window* NewTopLevelWindow(Id transport_window_id);
+  void SetWindowBounds(aura::Window* window,
+                       const gfx::Rect& bounds,
+                       uint32_t change_id = 1);
+
+ private:
+  WindowServiceClient* window_service_client_;
+
+  DISALLOW_COPY_AND_ASSIGN(WindowServiceClientTestHelper);
+};
+
+}  // namespace ws2
+}  // namespace ui
+
+#endif  // SERVICES_UI_WS2_WINDOW_SERVICE_CLIENT_TEST_HELPER_H_

services/ui/ws2/window_service_client_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/ui/ws2/window_service_client.h"
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include "base/message_loop/message_loop.h"
+#include "base/test/scoped_task_environment.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/layout_manager.h"
+#include "ui/aura/test/aura_test_helper.h"
+#include "ui/aura/window.h"
+#include "ui/compositor/test/context_factories_for_test.h"
+#include "ui/gl/test/gl_surface_test_support.h"
+
+namespace ui {
+namespace ws2 {
+namespace {
+
+// Sets up state needed for WindowService tests.
+class WindowServiceTestHelper {
+ public:
+  WindowServiceTestHelper() {
+    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);
+    delegate_.set_top_level_parent(root());
+  }
+  ~WindowServiceTestHelper() {
+    aura_test_helper_.TearDown();
+    ui::TerminateContextFactoryForTests();
+  }
+
+  WindowService* service() { return &service_; }
+  aura::Window* root() { return aura_test_helper_.root_window(); }
+
+ private:
+  base::test::ScopedTaskEnvironment task_environment_{
+      base::test::ScopedTaskEnvironment::MainThreadType::UI};
+  aura::test::AuraTestHelper aura_test_helper_;
+  TestWindowServiceDelegate delegate_;
+  WindowService service_{&delegate_};
+
+  DISALLOW_COPY_AND_ASSIGN(WindowServiceTestHelper);
+};
+
+class TestLayoutManager : public aura::LayoutManager {
+ public:
+  TestLayoutManager() = default;
+  ~TestLayoutManager() override = default;
+
+  void set_next_bounds(const gfx::Rect& bounds) { next_bounds_ = bounds; }
+
+  // aura::LayoutManager:
+  void OnWindowResized() override {}
+  void OnWindowAddedToLayout(aura::Window* child) override {}
+  void OnWillRemoveWindowFromLayout(aura::Window* child) override {}
+  void OnWindowRemovedFromLayout(aura::Window* child) override {}
+  void OnChildWindowVisibilityChanged(aura::Window* child,
+                                      bool visible) override {}
+  void SetChildBounds(aura::Window* child,
+                      const gfx::Rect& requested_bounds) override {
+    if (next_bounds_) {
+      SetChildBoundsDirect(child, *next_bounds_);
+      next_bounds_.reset();
+    } else {
+      SetChildBoundsDirect(child, requested_bounds);
+    }
+  }
+
+ private:
+  base::Optional<gfx::Rect> next_bounds_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestLayoutManager);
+};
+
+TEST(WindowServiceClientTest, SetBounds) {
+  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);
+  ASSERT_TRUE(top_level);
+  EXPECT_EQ("TopLevelCreated id=1 window_id=0,1 drawn=false",
+            SingleChangeToDescription(*changes));
+  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);
+  EXPECT_EQ(bounds_from_client, top_level->bounds());
+  EXPECT_EQ("ChangeCompleted id=2 sucess=true",
+            SingleChangeToDescription(*changes));
+  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();
+
+  // Set a LayoutManager so that when the client requests a bounds change the
+  // window is resized to a different bounds.
+  // |layout_manager| is owned by top_level->parent();
+  TestLayoutManager* layout_manager = new TestLayoutManager();
+  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());
+  // 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);
+
+  // And because the layout manager changed the bounds the result is false.
+  EXPECT_EQ("ChangeCompleted id=3 sucess=false",
+            ChangeToDescription((*changes)[1]));
+  changes->clear();
+}
+
+}  // namespace
+}  // namespace ws2
+}  // namespace ui