Fixes bug in handling restacking because of transients

ui/aura/mus/window_tree_client_unittest.cc

Index: ui/aura/mus/window_tree_client_unittest.cc
diff --git a/ui/aura/mus/window_tree_client_unittest.cc b/ui/aura/mus/window_tree_client_unittest.cc
index e2bd3439f5b28ca727c8ed2682fcd94660a6bf3e..23d8c1ecbc0d7e11d8eb7b82928947fdbae7bc4d 100644
--- a/ui/aura/mus/window_tree_client_unittest.cc
+++ b/ui/aura/mus/window_tree_client_unittest.cc
@@ -906,6 +906,97 @@ TEST_F(WindowTreeClientClientTest, Transients) {
   EXPECT_EQ(server_id(&transient), window_tree()->transient_data().child_id);
 }
 
+// Verifies adding/removing a transient child doesn't notify the server of the
+// restack when the change originates from the server.
+TEST_F(WindowTreeClientClientTest,
+       TransientChildServerMutateDoesntNotifyOfRestack) {
+  Window* w1 = new Window(nullptr);
+  w1->Init(ui::LAYER_NOT_DRAWN);
+  root_window()->AddChild(w1);
+  Window* w2 = new Window(nullptr);
+  w2->Init(ui::LAYER_NOT_DRAWN);
+  root_window()->AddChild(w2);
+  Window* w3 = new Window(nullptr);
+  w3->Init(ui::LAYER_NOT_DRAWN);
+  root_window()->AddChild(w3);
+  // Three children of root: |w1|, |w2| and |w3| (in that order). Make |w1| a
+  // transient child of |w2|. Should trigger moving |w1| on top of |w2|, but not
+  // notify the server of the reorder.
+  window_tree()->AckAllChanges();
+  window_tree_client()->OnTransientWindowAdded(server_id(w2), server_id(w1));
+  EXPECT_EQ(w2, root_window()->children()[0]);
+  EXPECT_EQ(w1, root_window()->children()[1]);
+  EXPECT_EQ(w3, root_window()->children()[2]);
+  // No changes should be scheduled.
+  EXPECT_EQ(0u, window_tree()->number_of_changes());
+
+  // Make |w3| also a transient child of |w2|. Order shouldn't change.
+  window_tree_client()->OnTransientWindowAdded(server_id(w2), server_id(w3));
+  EXPECT_EQ(w2, root_window()->children()[0]);
+  EXPECT_EQ(w1, root_window()->children()[1]);
+  EXPECT_EQ(w3, root_window()->children()[2]);
+  EXPECT_EQ(0u, window_tree()->number_of_changes());
+
+  // Remove |w1| as a transient child, this should move |w3| on top of |w2|.
+  window_tree_client()->OnTransientWindowRemoved(server_id(w2), server_id(w1));
+  EXPECT_EQ(w2, root_window()->children()[0]);
+  EXPECT_EQ(w3, root_window()->children()[1]);
+  EXPECT_EQ(w1, root_window()->children()[2]);
+  EXPECT_EQ(0u, window_tree()->number_of_changes());
+}
+
+// Verifies adding/removing a transient child doesn't notify the server of the

[msw, 2016/11/21 19:56:34]

When is the server notified of restack? Add a test fixture for that?

[sky, 2016/11/21 21:31:43]

For a transient? Never. The assertions on 971/974 verify no restacking is
scheduled.

[msw, 2016/11/21 22:34:49]

Okay, maybe I get it now. The server gets the call to add/remove transient and
*sometimes* causes a reordering. The server is never told by a client to reorder
for an added/removed transient. It's not entirely obvious, or I'm just slow...

[sky, 2016/11/21 22:41:10]

You got it. If a reorder needs to happen, the server does it too. So that if the
client tells the server about the reorder as well we get failures.

+// restack when the change originates from the client.
+TEST_F(WindowTreeClientClientTest,
+       TransientChildClientMutateDoesntNotifyOfRestack) {
+  client::TransientWindowClient* transient_client =
+      client::GetTransientWindowClient();
+  Window* w1 = new Window(nullptr);
+  w1->Init(ui::LAYER_NOT_DRAWN);
+  root_window()->AddChild(w1);
+  Window* w2 = new Window(nullptr);
+  w2->Init(ui::LAYER_NOT_DRAWN);
+  root_window()->AddChild(w2);
+  Window* w3 = new Window(nullptr);
+  w3->Init(ui::LAYER_NOT_DRAWN);
+  root_window()->AddChild(w3);
+  // Three children of root: |w1|, |w2| and |w3| (in that order). Make |w1| a
+  // transient child of |w2|. Should trigger moving |w1| on top of |w2|, but not
+  // notify the server of the reorder.
+  window_tree()->AckAllChanges();
+  transient_client->AddTransientChild(w2, w1);
+  EXPECT_EQ(w2, root_window()->children()[0]);
+  EXPECT_EQ(w1, root_window()->children()[1]);
+  EXPECT_EQ(w3, root_window()->children()[2]);
+  // Only a single add transient change should be added.
+  EXPECT_TRUE(window_tree()->AckSingleChangeOfType(
+      WindowTreeChangeType::ADD_TRANSIENT, true));
+  EXPECT_EQ(0u, window_tree()->number_of_changes());
+
+  // Make |w3| also a transient child of |w2|. Order shouldn't change.
+  window_tree_client()->OnTransientWindowAdded(server_id(w2), server_id(w3));

[msw, 2016/11/21 19:56:34]

Did you mean to use transient_client here and below?

[sky, 2016/11/21 21:31:43]

Done.

+  EXPECT_EQ(w2, root_window()->children()[0]);
+  EXPECT_EQ(w1, root_window()->children()[1]);
+  EXPECT_EQ(w3, root_window()->children()[2]);
+  EXPECT_EQ(0u, window_tree()->number_of_changes());
+
+  // Remove |w1| as a transient child, this should move |w3| on top of |w2|.
+  window_tree_client()->OnTransientWindowRemoved(server_id(w2), server_id(w1));
+  EXPECT_EQ(w2, root_window()->children()[0]);
+  EXPECT_EQ(w3, root_window()->children()[1]);
+  EXPECT_EQ(w1, root_window()->children()[2]);
+  EXPECT_EQ(0u, window_tree()->number_of_changes());
+
+  // Make |w1| the first child and ensure a REORDER was scheduled.
+  root_window()->StackChildAtBottom(w1);

[msw, 2016/11/21 19:56:34]

Should we test some bad behavior, like StackChildAtBottom(w3)?

[sky, 2016/11/21 21:31:43]

Ugh. This uncovered a bug in aura 667460.

+  EXPECT_EQ(w1, root_window()->children()[0]);
+  EXPECT_EQ(w2, root_window()->children()[1]);
+  EXPECT_EQ(w3, root_window()->children()[2]);
+  EXPECT_TRUE(window_tree()->AckSingleChangeOfType(
+      WindowTreeChangeType::REORDER, true));
+  EXPECT_EQ(0u, window_tree()->number_of_changes());
+}
+
 TEST_F(WindowTreeClientClientTest,
        TopLevelWindowDestroyedBeforeCreateComplete) {
   const size_t initial_root_count =