Make system modal check more strict

ash/wm/system_modal_container_layout_manager.cc

 // Copyright (c) 2012 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 "ash/wm/system_modal_container_layout_manager.h"
 
 #include <cmath>
 
 #include "ash/common/session/session_state_delegate.h"
 #include "ash/common/shell_window_ids.h"
@@ skipping 39 matching lines @@
 }
 
 void SystemModalContainerLayoutManager::OnWindowAddedToLayout(
     aura::Window* child) {
   DCHECK(child == modal_background_ ||
          child->type() == ui::wm::WINDOW_TYPE_NORMAL ||
          child->type() == ui::wm::WINDOW_TYPE_POPUP);
   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);
 }
 
 void SystemModalContainerLayoutManager::SetChildBounds(
     aura::Window* child,
     const gfx::Rect& requested_bounds) {
   SnapToPixelLayoutManager::SetChildBounds(child, requested_bounds);
   child->SetProperty(kCenteredKey, DialogIsCentered(requested_bounds));
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // SystemModalContainerLayoutManager, aura::WindowObserver implementation:
 
 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);
     Shell::GetInstance()->OnModalWindowRemoved(window);
   }
 }
 
 void SystemModalContainerLayoutManager::OnWindowDestroying(
     aura::Window* window) {
   if (modal_background_ == window) {
     if (keyboard::KeyboardController::GetInstance())
       keyboard::KeyboardController::GetInstance()->RemoveObserver(this);
     modal_background_ = nullptr;
   }
 }
 
+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:
 
 void SystemModalContainerLayoutManager::OnKeyboardBoundsChanging(
     const gfx::Rect& new_bounds) {
   PositionDialogsAfterWorkAreaResize();
 }
 
 bool SystemModalContainerLayoutManager::IsPartOfActiveModalWindow(
     aura::Window* window) {
-  return modal_window() && wm::GetActivatableWindow(window) == modal_window();
+  return modal_window() && modal_window()->Contains(window);
 }
 
 bool SystemModalContainerLayoutManager::ActivateNextModalWindow() {
   if (modal_windows_.empty())
     return false;
   wm::ActivateWindow(modal_window());
   return true;
 }
 
 void SystemModalContainerLayoutManager::CreateModalBackground() {
@@ skipping 36 matching lines @@
 
 ////////////////////////////////////////////////////////////////////////////////
 // SystemModalContainerLayoutManager, private:
 
 void SystemModalContainerLayoutManager::AddModalWindow(aura::Window* window) {
   if (modal_windows_.empty()) {
     aura::Window* capture_window = aura::client::GetCaptureWindow(container_);
     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);
   Shell::GetInstance()->CreateModalBackground(window);
   window->parent()->StackChildAtTop(window);
 
   gfx::Rect target_bounds = window->bounds();
   target_bounds.AdjustToFit(GetUsableDialogArea());
   window->SetBounds(target_bounds);
 }
@@ skipping 57 matching lines @@
 bool SystemModalContainerLayoutManager::DialogIsCentered(
     const gfx::Rect& window_bounds) {
   gfx::Point window_center = window_bounds.CenterPoint();
   gfx::Point container_center = GetUsableDialogArea().CenterPoint();
   return std::abs(window_center.x() - container_center.x()) <
              kCenterPixelDelta &&
          std::abs(window_center.y() - container_center.y()) < kCenterPixelDelta;
 }
 
 }  // namespace ash