Fixes bug in handling restacking because of transients

ui/aura/mus/window_port_mus.cc

Index: ui/aura/mus/window_port_mus.cc
diff --git a/ui/aura/mus/window_port_mus.cc b/ui/aura/mus/window_port_mus.cc
index 7797d107e40479042958345d1e8870b8e61342b1..d6ec918bd2e545853db3f16238d71d8227f0078b 100644
--- a/ui/aura/mus/window_port_mus.cc
+++ b/ui/aura/mus/window_port_mus.cc
@@ -111,8 +111,18 @@ void WindowPortMus::RemoveChangeById(ServerChangeIdType change_id) {
 
 bool WindowPortMus::RemoveChangeByTypeAndData(const ServerChangeType type,
                                               const ServerChangeData& data) {
-  for (auto iter = server_changes_.begin(); iter != server_changes_.end();
-       ++iter) {
+  auto iter = FindServerChangeByTypeAndData(type, data);
+  if (iter == server_changes_.end())
+    return false;
+  server_changes_.erase(iter);
+  return true;
+}
+
+WindowPortMus::ServerChanges::iterator
+WindowPortMus::FindServerChangeByTypeAndData(const ServerChangeType type,
+                                             const ServerChangeData& data) {
+  auto iter = server_changes_.begin();
+  for (; iter != server_changes_.end(); ++iter) {
     if (iter->type != type)
       continue;
 
@@ -122,29 +132,28 @@ bool WindowPortMus::RemoveChangeByTypeAndData(const ServerChangeType type,
       case ServerChangeType::REMOVE:
       case ServerChangeType::REMOVE_TRANSIENT:
       case ServerChangeType::REORDER:
+      case ServerChangeType::TRANSIENT_REORDER:
         if (iter->data.child_id == data.child_id)
-          break;
-        continue;
+          return iter;
+        break;
       case ServerChangeType::BOUNDS:
         if (iter->data.bounds == data.bounds)
-          break;
-        continue;
+          return iter;
+        break;
       case ServerChangeType::DESTROY:
         // No extra data for delete.
-        break;
+        return iter;
       case ServerChangeType::PROPERTY:
         if (iter->data.property_name == data.property_name)
-          break;
-        continue;
+          return iter;
+        break;
       case ServerChangeType::VISIBLE:
         if (iter->data.visible == data.visible)
-          break;
-        continue;
+          return iter;
+        break;
     }
-    server_changes_.erase(iter);
-    return true;
   }
-  return false;
+  return iter;
 }
 
 PropertyConverter* WindowPortMus::GetPropertyConverter() {
@@ -309,6 +318,20 @@ WindowPortMus::PrepareForServerBoundsChange(const gfx::Rect& bounds) {
   return std::move(data);
 }
 
+void WindowPortMus::PrepareForTransientRestack(WindowMus* window) {
+  ServerChangeData change_data;
+  change_data.child_id = window->server_id();
+  ScheduleChange(ServerChangeType::TRANSIENT_REORDER, change_data);
+}
+
+void WindowPortMus::OnTransientRestackDone(WindowMus* window) {
+  ServerChangeData change_data;
+  change_data.child_id = window->server_id();
+  const bool removed = RemoveChangeByTypeAndData(
+      ServerChangeType::TRANSIENT_REORDER, change_data);
+  DCHECK(removed);

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

Just so I understand: is OnTransientRestackDone only called when the server
performed the restack, so the scheduled TRANSIENT_REORDER can be removed;
otherwise, this functions isn't called and the change is left scheduled (and to
be performed soon)?

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

OnTransientRestackDone is always called.
Yes, this is to balance the OnWillRestackTransientChildAbove and is necessary
because the call from TransientWindowManager to StackChildAbove() may do nothing
(the window may already be in place).  Ideally OnDidRestackTransientChildAbove
wouldn't be necessary, but it is because TransientWindowManager is outside of
aura. That is, the only way to possibly get rid of
OnWillRestackTransientChildAbove  is to add context to why the reorder is
happening. That would be a pain though. Another possibility is to move transient
management back into aura proper, most likely on aura::Window. Doing that would
mean aura::Window knows the reorder is the result of a transient being
added/removed and could call different functions on WindowPort. That a bigger
change than I want to bite off here though.

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

Okay, so the transient reordering step sometimes does nothing, but we always
schedule and remove TRANSIENT_REORDER, because OnWillMoveChild may notify
window_tree_client_, but it never removes the TRANSIENT_REORDER change.

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

You got it.

+}
+
 std::unique_ptr<WindowMusChangeData>
 WindowPortMus::PrepareForServerVisibilityChange(bool value) {
   std::unique_ptr<WindowMusChangeDataImpl> data(
@@ -352,8 +375,13 @@ void WindowPortMus::OnWillRemoveChild(Window* child) {
 void WindowPortMus::OnWillMoveChild(size_t current_index, size_t dest_index) {
   ServerChangeData change_data;
   change_data.child_id = Get(window_->children()[current_index])->server_id();
-  if (!RemoveChangeByTypeAndData(ServerChangeType::REORDER, change_data))
+  // See description of TRANSIENT_REORDER for details on why it isn't removed
+  // here.
+  if (!RemoveChangeByTypeAndData(ServerChangeType::REORDER, change_data) &&
+      FindServerChangeByTypeAndData(ServerChangeType::TRANSIENT_REORDER,
+                                    change_data) == server_changes_.end()) {
     window_tree_client_->OnWindowMusMoveChild(this, current_index, dest_index);
+  }
 }
 
 void WindowPortMus::OnVisibilityChanged(bool visible) {