Changes OnWindowHierarchyChanged() to include transient parent

This is necessary for the case where a client creates a window, sets
the transient parent and then adds it. Without this change the wm
wouldn't know about the transient parent and would be in a weird
state.

BUG=663903
TEST=covered by tests
R=ben@chromium.org, tsepez@chromium.org

Review-Url: https://codereview.chromium.org/2557353003
Cr-Commit-Position: refs/heads/master@{#437450}

services/ui/public/interfaces/window_tree.mojom

@@ -357,7 +357,7 @@ interface WindowTreeClient {
   // If |window| was not visible to this client, but is visible now because
   // |new_parent| is visible to this client, then |windows| contains details
   // about |window|, and all its descendants. |windows| includes any windows
-  // that client may already know about, but did not know the parent because
+  // the client may already know about, but did not know the parent because
   // the parent was previously not visible to this client.
   // This is not sent for hierarchy changes of windows not known to this client
   // or not attached to the tree.

services/ui/public/interfaces/window_tree_constants.mojom

@@ -15,6 +15,9 @@ struct WindowData {
   // Unique identifier of the window.
   uint32 window_id;
 
+  // Id of the transient parent, 0 if there isn't one.
+  uint32 transient_parent_id;
+
   gfx.mojom.Rect bounds;
 
   // Arbitrary key/value pairs. The interpretation of these is left to the

services/ui/ws/window_manager_client_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/run_loop.h"
 #include "services/ui/common/util.h"
 #include "services/ui/ws/window_server_test_base.h"
+#include "ui/aura/client/transient_window_client.h"
 #include "ui/aura/env.h"
 #include "ui/aura/mus/window_port_mus.h"
 #include "ui/aura/mus/window_tree_client.h"
@@ -111,7 +112,6 @@ bool WaitForBoundsToChange(aura::Window* window) {
   return WindowServerTestBase::DoRunLoopWithTimeout();
 }
 
-
 // Spins a run loop until the tree beginning at |root| has |tree_size| windows
 // (including |root|).
 class TreeSizeMatchesObserver : public aura::WindowObserver {
@@ -204,7 +204,8 @@ class WindowTracker : public aura::WindowObserver {
   DISALLOW_COPY_AND_ASSIGN(WindowTracker);
 };
 
-// Creates a new Window parented to |parent| that is made visible.
+// Creates a new visible Window. If |parent| is non-null the newly created
+// window is added to it.
 aura::Window* NewVisibleWindow(aura::Window* parent,
                                aura::WindowTreeClient* client) {
   std::unique_ptr<aura::WindowPortMus> window_port_mus =
@@ -212,7 +213,8 @@ aura::Window* NewVisibleWindow(aura::Window* parent,
   aura::Window* window = new aura::Window(nullptr, std::move(window_port_mus));
   window->Init(ui::LAYER_NOT_DRAWN);
   window->Show();
-  parent->AddChild(window);
+  if (parent)
+    parent->AddChild(window);
   return window;
 }
 
@@ -718,5 +720,41 @@ TEST_F(WindowServerTest, EstablishConnectionViaFactory) {
             window_tree_host_in_second_client.GetBoundsInPixels());
 }
 
+TEST_F(WindowServerTest, OnWindowHierarchyChangedIncludesTransientParent) {
+  // Create a second connection. In the second connection create a window,
+  // parent it to the root, create another window, mark it as a transient parent
+  // of the first window and then add it.
+  EstablishConnectionViaFactoryDelegate delegate(window_manager());
+  set_window_manager_delegate(&delegate);
+  aura::WindowTreeClient second_client(connector(), this);
+  second_client.ConnectViaWindowTreeFactory();
+  aura::WindowTreeHostMus window_tree_host_in_second_client(&second_client);
+  aura::Window* second_client_child = NewVisibleWindow(
+      window_tree_host_in_second_client.window(), &second_client);
+  std::unique_ptr<aura::WindowPortMus> window_port_mus =
+      base::MakeUnique<aura::WindowPortMus>(&second_client,
+                                            aura::WindowMusType::LOCAL);
+  // Create the transient without a parent, set transient parent, then add.
+  aura::Window* transient = NewVisibleWindow(nullptr, &second_client);
+  aura::client::TransientWindowClient* transient_window_client =
+      aura::client::GetTransientWindowClient();
+  transient_window_client->AddTransientChild(second_client_child, transient);
+  second_client_child->AddChild(transient);
+
+  // Wait for the top-level to appear in the window manager.
+  ASSERT_TRUE(delegate.QuitOnCreate());
+  aura::Window* top_level_in_wm = delegate.created_window();
+
+  // Makes sure the window manager sees the same structure and the transient
+  // parent is connected correctly.
+  ASSERT_TRUE(WaitForTreeSizeToMatch(top_level_in_wm, 3u));
+  ASSERT_EQ(1u, top_level_in_wm->children().size());
+  aura::Window* second_client_child_in_wm = top_level_in_wm->children()[0];
+  ASSERT_EQ(1u, second_client_child_in_wm->children().size());
+  aura::Window* transient_in_wm = second_client_child_in_wm->children()[0];
+  ASSERT_EQ(second_client_child_in_wm,
+            transient_window_client->GetTransientParent(transient_in_wm));
+}
+
 }  // namespace ws
 }  // namespace ui

services/ui/ws/window_tree.cc

@@ -1014,15 +1014,21 @@ mojom::WindowDataPtr WindowTree::WindowToWindowData(
     const ServerWindow* window) {
   DCHECK(IsWindowKnown(window));
   const ServerWindow* parent = window->parent();
-  // If the parent isn't known, it means the parent is not visible to us (not
-  // in roots), and should not be sent over.
-  if (parent && !IsWindowKnown(parent))
+  const ServerWindow* transient_parent = window->transient_parent();
+  // If the parent or transient parent isn't known, it means it is not visible
+  // to the client and should not be sent over.
+  if (!IsWindowKnown(parent))
     parent = nullptr;
+  if (!IsWindowKnown(transient_parent))
+    transient_parent = nullptr;
   mojom::WindowDataPtr window_data(mojom::WindowData::New());
   window_data->parent_id =
       parent ? ClientWindowIdForWindow(parent).id : ClientWindowId().id;
   window_data->window_id =
       window ? ClientWindowIdForWindow(window).id : ClientWindowId().id;
+  window_data->transient_parent_id =
+      transient_parent ? ClientWindowIdForWindow(transient_parent).id
+                       : ClientWindowId().id;
   window_data->bounds = window->bounds();
   window_data->properties = mojo::MapToUnorderedMap(window->properties());
   window_data->visible = window->visible();

ui/aura/mus/window_tree_client.cc

@@ -296,6 +296,11 @@ WindowMus* WindowTreeClient::GetWindowByServerId(Id id) {
   return it != windows_.end() ? it->second : nullptr;
 }
 
+bool WindowTreeClient::IsWindowKnown(aura::Window* window) {
+  WindowMus* window_mus = WindowMus::Get(window);
+  return windows_.count(window_mus->server_id()) > 0;
+}
+
 InFlightChange* WindowTreeClient::GetOldestInFlightChangeMatching(
     const InFlightChange& change) {
   for (const auto& pair : in_flight_map_) {
@@ -329,38 +334,64 @@ bool WindowTreeClient::ApplyServerChangeToExistingInFlightChange(
 
 void WindowTreeClient::BuildWindowTree(
     const std::vector<ui::mojom::WindowDataPtr>& windows) {
-  for (const auto& window_data : windows) {
-    WindowMus* parent = window_data->parent_id == kInvalidServerId
-                            ? nullptr
-                            : GetWindowByServerId(window_data->parent_id);
-    WindowMus* existing_window = GetWindowByServerId(window_data->window_id);
-    if (!existing_window)
-      NewWindowFromWindowData(parent, window_data);
-    else if (parent)
-      parent->AddChildFromServer(existing_window);
+  for (const auto& window_data : windows)
+    CreateOrUpdateWindowFromWindowData(*window_data);
+}
+
+void WindowTreeClient::CreateOrUpdateWindowFromWindowData(
+    const ui::mojom::WindowData& window_data) {
+  WindowMus* parent = window_data.parent_id == kInvalidServerId
+                          ? nullptr
+                          : GetWindowByServerId(window_data.parent_id);
+  WindowMus* window = GetWindowByServerId(window_data.window_id);
+  if (!window)
+    window = NewWindowFromWindowData(parent, window_data);
+  else if (parent)
+    parent->AddChildFromServer(window);
+
+  if (window_data.transient_parent_id == kInvalidServerId)
+    return;
+
+  // Adjust the transient parent if necessary.
+  client::TransientWindowClient* transient_window_client =
+      client::GetTransientWindowClient();
+  Window* existing_transient_parent =
+      transient_window_client->GetTransientParent(window->GetWindow());
+  WindowMus* new_transient_parent =
+      GetWindowByServerId(window_data.transient_parent_id);
+  if (!new_transient_parent && existing_transient_parent) {
+    WindowMus::Get(existing_transient_parent)
+        ->RemoveTransientChildFromServer(window);
+  } else if (new_transient_parent &&
+             new_transient_parent->GetWindow() != existing_transient_parent) {
+    if (existing_transient_parent) {
+      WindowMus::Get(existing_transient_parent)
+          ->RemoveTransientChildFromServer(window);
+    }
+    new_transient_parent->AddTransientChildFromServer(window);
   }
 }
 
 std::unique_ptr<WindowPortMus> WindowTreeClient::CreateWindowPortMus(
-    const ui::mojom::WindowDataPtr& window_data,
+    const ui::mojom::WindowData& window_data,
     WindowMusType window_mus_type) {
   std::unique_ptr<WindowPortMus> window_port_mus(
       base::MakeUnique<WindowPortMus>(this, window_mus_type));
-  window_port_mus->set_server_id(window_data->window_id);
+  window_port_mus->set_server_id(window_data.window_id);
   RegisterWindowMus(window_port_mus.get());
   return window_port_mus;
 }
 
 void WindowTreeClient::SetLocalPropertiesFromServerProperties(
     WindowMus* window,
-    const ui::mojom::WindowDataPtr& window_data) {
-  for (auto& pair : window_data->properties)
+    const ui::mojom::WindowData& window_data) {
+  for (auto& pair : window_data.properties)
     window->SetPropertyFromServer(pair.first, &pair.second);
 }
 
 std::unique_ptr<WindowTreeHostMus> WindowTreeClient::CreateWindowTreeHost(
     WindowMusType window_mus_type,
-    const ui::mojom::WindowDataPtr& window_data,
+    const ui::mojom::WindowData& window_data,
     int64_t display_id) {
   std::unique_ptr<WindowPortMus> window_port =
       CreateWindowPortMus(window_data, window_mus_type);
@@ -368,35 +399,33 @@ std::unique_ptr<WindowTreeHostMus> WindowTreeClient::CreateWindowTreeHost(
   std::unique_ptr<WindowTreeHostMus> window_tree_host =
       base::MakeUnique<WindowTreeHostMus>(std::move(window_port), this,
                                           display_id);
-  if (!window_data.is_null()) {
-    SetLocalPropertiesFromServerProperties(
-        WindowMus::Get(window_tree_host->window()), window_data);
-    if (window_data->visible) {
-      SetWindowVisibleFromServer(WindowMus::Get(window_tree_host->window()),
-                                 true);
-    }
-    SetWindowBoundsFromServer(WindowMus::Get(window_tree_host->window()),
-                              window_data->bounds);
+  SetLocalPropertiesFromServerProperties(
+      WindowMus::Get(window_tree_host->window()), window_data);
+  if (window_data.visible) {
+    SetWindowVisibleFromServer(WindowMus::Get(window_tree_host->window()),
+                               true);
   }
+  SetWindowBoundsFromServer(WindowMus::Get(window_tree_host->window()),
+                            window_data.bounds);
   return window_tree_host;
 }
 
 WindowMus* WindowTreeClient::NewWindowFromWindowData(
     WindowMus* parent,
-    const ui::mojom::WindowDataPtr& window_data) {
+    const ui::mojom::WindowData& window_data) {
   // This function is only called for windows coming from other clients.
   std::unique_ptr<WindowPortMus> window_port_mus(
       CreateWindowPortMus(window_data, WindowMusType::OTHER));
   WindowPortMus* window_port_mus_ptr = window_port_mus.get();
   Window* window = new Window(nullptr, std::move(window_port_mus));
   WindowMus* window_mus = window_port_mus_ptr;
-  SetWindowTypeFromProperties(window, window_data->properties);
+  SetWindowTypeFromProperties(window, window_data.properties);
   window->Init(ui::LAYER_NOT_DRAWN);
   SetLocalPropertiesFromServerProperties(window_mus, window_data);
-  window_mus->SetBoundsFromServer(window_data->bounds);
+  window_mus->SetBoundsFromServer(window_data.bounds);
   if (parent)
     parent->AddChildFromServer(window_port_mus_ptr);
-  if (window_data->visible)
+  if (window_data.visible)
     window_mus->SetVisibleFromServer(true);
   return window_port_mus_ptr;
 }
@@ -464,7 +493,7 @@ void WindowTreeClient::OnEmbedImpl(ui::mojom::WindowTree* window_tree,
 
   DCHECK(roots_.empty());
   std::unique_ptr<WindowTreeHostMus> window_tree_host =
-      CreateWindowTreeHost(WindowMusType::EMBED, root_data, display_id);
+      CreateWindowTreeHost(WindowMusType::EMBED, *root_data, display_id);
 
   focus_synchronizer_->SetFocusFromServer(
       GetWindowByServerId(focused_window_id));
@@ -481,7 +510,7 @@ WindowTreeHostMus* WindowTreeClient::WmNewDisplayAddedImpl(
   window_manager_delegate_->OnWmWillCreateDisplay(display);
 
   std::unique_ptr<WindowTreeHostMus> window_tree_host =
-      CreateWindowTreeHost(WindowMusType::DISPLAY, root_data, display.id());
+      CreateWindowTreeHost(WindowMusType::DISPLAY, *root_data, display.id());
 
   WindowTreeHostMus* window_tree_host_ptr = window_tree_host.get();
   window_manager_delegate_->OnWmNewDisplay(std::move(window_tree_host),
@@ -1583,10 +1612,17 @@ void WindowTreeClient::OnWindowTreeHostCreated(
 
 void WindowTreeClient::OnTransientChildWindowAdded(Window* parent,
                                                    Window* transient_child) {
+  // TransientWindowClient is a singleton and we allow multiple
+  // WindowTreeClients. Ignore changes to windows we don't know about (assume
+  // they came from another connection).
+  if (!IsWindowKnown(parent) || !IsWindowKnown(transient_child))
+    return;
+
   if (WindowMus::Get(parent)->OnTransientChildAdded(
           WindowMus::Get(transient_child)) == WindowMus::ChangeSource::SERVER) {
     return;
   }
+
   // The change originated from client code and needs to be sent to the server.
   DCHECK(tree_);
   WindowMus* parent_mus = WindowMus::Get(parent);
@@ -1599,6 +1635,10 @@ void WindowTreeClient::OnTransientChildWindowAdded(Window* parent,
 
 void WindowTreeClient::OnTransientChildWindowRemoved(Window* parent,
                                                      Window* transient_child) {
+  // See comments in OnTransientChildWindowAdded() for details on early return.
+  if (!IsWindowKnown(parent) || !IsWindowKnown(transient_child))
+    return;
+
   if (WindowMus::Get(parent)->OnTransientChildRemoved(
           WindowMus::Get(transient_child)) == WindowMus::ChangeSource::SERVER) {
     return;
@@ -1616,6 +1656,10 @@ void WindowTreeClient::OnWillRestackTransientChildAbove(
     Window* parent,
     Window* transient_child) {
   DCHECK(parent->parent());
+  // See comments in OnTransientChildWindowAdded() for details on early return.
+  if (!IsWindowKnown(parent->parent()))
+    return;
+
   DCHECK_EQ(parent->parent(), transient_child->parent());
   WindowMus::Get(parent->parent())
       ->PrepareForTransientRestack(WindowMus::Get(transient_child));
@@ -1625,6 +1669,9 @@ void WindowTreeClient::OnDidRestackTransientChildAbove(
     Window* parent,
     Window* transient_child) {
   DCHECK(parent->parent());
+  // See comments in OnTransientChildWindowAdded() for details on early return.
+  if (!IsWindowKnown(parent->parent()))
+    return;
   DCHECK_EQ(parent->parent(), transient_child->parent());
   WindowMus::Get(parent->parent())
       ->OnTransientRestackDone(WindowMus::Get(transient_child));

ui/aura/mus/window_tree_client.h

@@ -181,6 +181,8 @@ class AURA_EXPORT WindowTreeClient
 
   WindowMus* GetWindowByServerId(Id id);
 
+  bool IsWindowKnown(aura::Window* window);
+
   // Returns the oldest InFlightChange that matches |change|.
   InFlightChange* GetOldestInFlightChangeMatching(const InFlightChange& change);
 
@@ -195,25 +197,29 @@ class AURA_EXPORT WindowTreeClient
 
   void BuildWindowTree(const std::vector<ui::mojom::WindowDataPtr>& windows);
 
+  // If the window identified by |window_data| doesn't exist a new window is
+  // created, otherwise the existing window is updated based on |window_data|.
+  void CreateOrUpdateWindowFromWindowData(
+      const ui::mojom::WindowData& window_data);
+
   // Creates a WindowPortMus from the server side data.
   std::unique_ptr<WindowPortMus> CreateWindowPortMus(
-      const ui::mojom::WindowDataPtr& window_data,
+      const ui::mojom::WindowData& window_data,
       WindowMusType window_mus_type);
 
   // Sets local properties on the associated Window from the server properties.
   void SetLocalPropertiesFromServerProperties(
       WindowMus* window,
-      const ui::mojom::WindowDataPtr& window_data);
+      const ui::mojom::WindowData& window_data);
 
   // Creates a new WindowTreeHostMus.
   std::unique_ptr<WindowTreeHostMus> CreateWindowTreeHost(
       WindowMusType window_mus_type,
-      const ui::mojom::WindowDataPtr& window_data,
+      const ui::mojom::WindowData& window_data,
       int64_t display_id);
 
-  WindowMus* NewWindowFromWindowData(
-      WindowMus* parent,
-      const ui::mojom::WindowDataPtr& window_data);
+  WindowMus* NewWindowFromWindowData(WindowMus* parent,
+                                     const ui::mojom::WindowData& window_data);
 
   // Sets the ui::mojom::WindowTree implementation.
   void SetWindowTree(ui::mojom::WindowTreePtr window_tree_ptr);

ui/views/widget/widget_unittest.cc

@@ -857,10 +857,6 @@ TEST_F(WidgetObserverTest, DISABLED_VisibilityChange) {
 }
 
 TEST_F(WidgetObserverTest, DestroyBubble) {
-  // TODO: reenable once http://crbug.com/663903 is fixed.
-  if (IsAuraMusClient())
-    return;
-
   // This test expect NativeWidgetAura, force its creation.
   ViewsDelegate::GetInstance()->set_native_widget_factory(
       ViewsDelegate::NativeWidgetFactory());
@@ -1289,10 +1285,6 @@ TEST_F(WidgetTest, DISABLED_FocusChangesOnBubble) {
 }
 
 TEST_F(WidgetTest, BubbleControlsResetOnInit) {
-  // TODO: enable once http://crbug.com/660994 is fixed.
-  if (IsAuraMusClient())
-    return;
-
   WidgetAutoclosePtr anchor(CreateTopLevelPlatformWidget());
   anchor->Show();