Make system modal check more strict

ash/wm/system_modal_container_layout_manager.cc

Index: ash/wm/system_modal_container_layout_manager.cc
diff --git a/ash/wm/system_modal_container_layout_manager.cc b/ash/wm/system_modal_container_layout_manager.cc
index 16e53330c8d587d76eff9a40e484091afe505e65..03cf102685db23f8f5edc6441f46423b48c20829 100644
--- a/ash/wm/system_modal_container_layout_manager.cc
+++ b/ash/wm/system_modal_container_layout_manager.cc
@@ -57,16 +57,22 @@ void SystemModalContainerLayoutManager::OnWindowAddedToLayout(
   DCHECK(
       container_->id() != kShellWindowId_LockSystemModalContainer ||
       Shell::GetInstance()->session_state_delegate()->IsUserSessionBlocked());
+  // There is no reason to add these modal window in this container.
+  // DCHECK to avoid simple mistake.

[James Cook, 2016/06/27 00:33:43]

optional: Maybe add a line to the header file class comment explaining what type
of windows this layout manager/container manages?

[oshima, 2016/06/27 02:26:07]

Done.

+  DCHECK_NE(child->GetProperty(aura::client::kModalKey), ui::MODAL_TYPE_CHILD);
+  DCHECK_NE(child->GetProperty(aura::client::kModalKey), ui::MODAL_TYPE_WINDOW);
 
   child->AddObserver(this);
-  if (child->GetProperty(aura::client::kModalKey) != ui::MODAL_TYPE_NONE)
+  if (child->GetProperty(aura::client::kModalKey) == ui::MODAL_TYPE_SYSTEM &&
+      child->IsVisible()) {
     AddModalWindow(child);
+  }
 }
 
 void SystemModalContainerLayoutManager::OnWillRemoveWindowFromLayout(
     aura::Window* child) {
   child->RemoveObserver(this);
-  if (child->GetProperty(aura::client::kModalKey) != ui::MODAL_TYPE_NONE)
+  if (child->GetProperty(aura::client::kModalKey) == ui::MODAL_TYPE_SYSTEM)
     RemoveModalWindow(child);
 }
 
@@ -84,14 +90,14 @@ void SystemModalContainerLayoutManager::OnWindowPropertyChanged(
     aura::Window* window,
     const void* key,
     intptr_t old) {
-  if (key != aura::client::kModalKey)
+  if (key != aura::client::kModalKey || !window->IsVisible())
     return;
 
   ui::ModalType new_modal = window->GetProperty(aura::client::kModalKey);
   if (static_cast<ui::ModalType>(old) == new_modal)
     return;
 
-  if (new_modal != ui::MODAL_TYPE_NONE) {
+  if (new_modal == ui::MODAL_TYPE_SYSTEM) {
     AddModalWindow(window);
   } else {
     RemoveModalWindow(window);
@@ -108,6 +114,19 @@ void SystemModalContainerLayoutManager::OnWindowDestroying(
   }
 }
 
+void SystemModalContainerLayoutManager::OnWindowVisibilityChanged(
+    aura::Window* window,
+    bool visible) {
+  if (window->GetProperty(aura::client::kModalKey) != ui::MODAL_TYPE_SYSTEM)
+    return;
+  if (window->IsVisible()) {
+    AddModalWindow(window);
+  } else {
+    RemoveModalWindow(window);
+    Shell::GetInstance()->OnModalWindowRemoved(window);
+  }
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // SystemModalContainerLayoutManager, Keyboard::KeybaordControllerObserver
 // implementation:
@@ -119,7 +138,7 @@ void SystemModalContainerLayoutManager::OnKeyboardBoundsChanging(
 
 bool SystemModalContainerLayoutManager::IsPartOfActiveModalWindow(
     aura::Window* window) {
-  return modal_window() && wm::GetActivatableWindow(window) == modal_window();
+  return modal_window() && modal_window()->Contains(window);
 }
 
 bool SystemModalContainerLayoutManager::ActivateNextModalWindow() {
@@ -176,6 +195,7 @@ void SystemModalContainerLayoutManager::AddModalWindow(aura::Window* window) {
     if (capture_window)
       capture_window->ReleaseCapture();
   }
+  DCHECK(window->IsVisible());

[James Cook, 2016/06/27 00:33:43]

Thanks for adding all the DCHECKs. It makes the code easier to understand.

   DCHECK(!ContainsValue(modal_windows_, window));
 
   modal_windows_.push_back(window);