Fixes bug in handling restacking because of transients

ui/aura/mus/window_port_mus.cc

 // Copyright 2016 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 "ui/aura/mus/window_port_mus.h"
 
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/client/transient_window_client.h"
 #include "ui/aura/mus/property_converter.h"
 #include "ui/aura/mus/surface_id_handler.h"
@@ skipping 93 matching lines @@
        ++iter) {
     if (iter->server_change_id == change_id) {
       server_changes_.erase(--(iter.base()));
       return;
     }
   }
 }
 
 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;
 
     switch (type) {
       case ServerChangeType::ADD:
       case ServerChangeType::ADD_TRANSIENT:
       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() {
   return window_tree_client_->delegate_->GetPropertyConverter();
 }
 
 Window* WindowPortMus::GetWindow() {
   return window_;
 }
 
@@ skipping 144 matching lines @@
 WindowPortMus::PrepareForServerBoundsChange(const gfx::Rect& bounds) {
   std::unique_ptr<WindowMusChangeDataImpl> data(
       base::MakeUnique<WindowMusChangeDataImpl>());
   ServerChangeData change_data;
   change_data.bounds = bounds;
   data->change = base::MakeUnique<ScopedServerChange>(
       this, ServerChangeType::BOUNDS, change_data);
   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(
       base::MakeUnique<WindowMusChangeDataImpl>());
   ServerChangeData change_data;
   change_data.visible = value;
   data->change = base::MakeUnique<ScopedServerChange>(
       this, ServerChangeType::VISIBLE, change_data);
   return std::move(data);
 }
@@ skipping 23 matching lines @@
 void WindowPortMus::OnWillRemoveChild(Window* child) {
   ServerChangeData change_data;
   change_data.child_id = Get(child)->server_id();
   if (!RemoveChangeByTypeAndData(ServerChangeType::REMOVE, change_data))
     window_tree_client_->OnWindowMusRemoveChild(this, Get(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) {
   ServerChangeData change_data;
   change_data.visible = visible;
   if (!RemoveChangeByTypeAndData(ServerChangeType::VISIBLE, change_data))
     window_tree_client_->OnWindowMusSetVisible(this, visible);
 }
 
 void WindowPortMus::OnDidChangeBounds(const gfx::Rect& old_bounds,
@@ skipping 16 matching lines @@
   change_data.property_name =
       GetPropertyConverter()->GetTransportNameForPropertyKey(key);
   // TODO(sky): investigate to see if we need to compare data. In particular do
   // we ever have a case where changing a property cascades into changing the
   // same property?
   if (!RemoveChangeByTypeAndData(ServerChangeType::PROPERTY, change_data))
     window_tree_client_->OnWindowMusPropertyChanged(this, key, std::move(data));
 }
 
 }  // namespace aura