chromeos: makes changing modal type not reparent window

Ash in mash mode was reparenting system modal dialogs. Problem is the
code was using GetDefaultParent(), which makes some assumptions that
are not always right for Chrome code. This fixes the issue by making
it so that when the modal type changes ash doesn't change the parent.

I suspect we will have to revisit this at some point, but this is the
easy fix for now.

BUG=743064
TEST=covered by test

Change-Id: I21b097b5ad2c52e153b0880ec33c96e950fad624
Reviewed-on: https://chromium-review.googlesource.com/580631Reviewed-by: James Cook <jamescook@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488503}

ash/mus/window_manager.cc

@@ -31,7 +31,6 @@
 #include "ash/shell.h"
 #include "ash/shell_init_params.h"
 #include "ash/wm/ash_focus_rules.h"
-#include "ash/wm/container_finder.h"
 #include "ash/wm/window_state.h"
 #include "base/memory/ptr_util.h"
 #include "services/service_manager/public/cpp/connector.h"
@@ -320,14 +319,13 @@ void WindowManager::OnWmSetModalType(aura::Window* window, ui::ModalType type) {
     return;
 
   window->SetProperty(aura::client::kModalKey, type);
-  if (type != ui::MODAL_TYPE_SYSTEM && old_type != ui::MODAL_TYPE_SYSTEM)
-    return;
-
-  aura::Window* new_parent = wm::GetDefaultParent(window, window->bounds());
-  DCHECK(new_parent);
-  if (window->parent())
-    window->parent()->RemoveChild(window);
-  new_parent->AddChild(window);
+  // Assume the client has positioned the window in the right container and
+  // don't attempt to change the parent. This is currently true for Chrome
+  // code, but likely there should be checks. Adding checks is complicated by
+  // the fact that assumptions in GetSystemModalContainer() are not always
+  // valid for Chrome code. In particular Chrome code may create system modal
+  // dialogs without a transient parent that Chrome wants shown on the lock
+  // screen.
 }
 
 void WindowManager::OnWmSetCanFocus(aura::Window* window, bool can_focus) {

ash/mus/window_manager_unittest.cc

@@ -6,10 +6,17 @@
 #include <memory>
 #include <vector>
 
+#include "ash/mus/window_manager.h"
+#include "ash/mus/window_manager_application.h"
 #include "ash/public/interfaces/constants.mojom.h"
+#include "ash/session/test_session_controller_client.h"
+#include "ash/shell.h"
+#include "ash/test/ash_test_base.h"
+#include "ash/test/ash_test_helper.h"
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
+#include "components/session_manager/session_manager_types.h"
 #include "services/service_manager/public/cpp/service_test.h"
 #include "services/ui/public/cpp/property_type_converters.h"
 #include "services/ui/public/interfaces/window_tree.mojom.h"
@@ -56,30 +63,34 @@ class WindowTreeClientDelegate : public aura::WindowTreeClientDelegate {
   }
 
   base::RunLoop run_loop_;
-  wm::WMState wm_state_;
+  ::wm::WMState wm_state_;
   aura::PropertyConverter property_converter_;
   std::unique_ptr<aura::WindowTreeHostMus> window_tree_host_;
 
   DISALLOW_COPY_AND_ASSIGN(WindowTreeClientDelegate);
 };
 
-// NOTE: this isn't named WindowManagerTest because gtest complains about tests
-// with the same name (there is already a WindowManagerTest in ash).
-class MusWindowManagerTest : public service_manager::test::ServiceTest {
+class WindowManagerServiceTest : public service_manager::test::ServiceTest {
  public:
-  MusWindowManagerTest()
+  WindowManagerServiceTest()
       : service_manager::test::ServiceTest("mash_unittests") {}
-  ~MusWindowManagerTest() override {}
+  ~WindowManagerServiceTest() override {}
+
+  void TearDown() override {
+    // Unset the screen installed by the test.
+    display::Screen::SetScreenInstance(nullptr);
+    service_manager::test::ServiceTest::TearDown();
+  }
 
  private:
-  DISALLOW_COPY_AND_ASSIGN(MusWindowManagerTest);
+  DISALLOW_COPY_AND_ASSIGN(WindowManagerServiceTest);
 };
 
 void OnEmbed(bool success) {
   ASSERT_TRUE(success);
 }
 
-TEST_F(MusWindowManagerTest, OpenWindow) {
+TEST_F(WindowManagerServiceTest, OpenWindow) {
   display::ScreenBase screen;
   screen.display_list().AddDisplay(
       display::Display(1, gfx::Rect(0, 0, 200, 200)),
@@ -120,5 +131,23 @@ TEST_F(MusWindowManagerTest, OpenWindow) {
   window_tree_delegate.DestroyWindowTreeHost();
 }
 
+using WindowManagerTest = AshTestBase;
+
+TEST_F(WindowManagerTest, SystemModalLockIsntReparented) {
+  ash_test_helper()->test_session_controller_client()->SetSessionState(
+      session_manager::SessionState::LOCKED);
+  std::unique_ptr<aura::Window> window = CreateTestWindow();
+  aura::Window* system_modal_container = Shell::GetContainer(
+      Shell::GetPrimaryRootWindow(), kShellWindowId_LockSystemModalContainer);
+  system_modal_container->AddChild(window.get());
+  aura::WindowManagerDelegate* window_manager_delegate =
+      ash_test_helper()->window_manager_app()->window_manager();
+  window_manager_delegate->OnWmSetModalType(window.get(),
+                                            ui::MODAL_TYPE_SYSTEM);
+  ASSERT_TRUE(window->parent());
+  // Setting to system modal should not reparent.
+  EXPECT_EQ(kShellWindowId_LockSystemModalContainer, window->parent()->id());
+}
+
 }  // namespace mus
 }  // namespace ash