CloseAll should allow windows to close cleanly

Bug: 1319973 Change-Id: I11150b6648a0d4072a64d984f8c6f184675594ea Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3632035 Commit-Queue: Yanzhu Du <yzd@google.com> Reviewed-by: Xiaoqian Dai <xdai@chromium.org> Cr-Commit-Position: refs/heads/main@{#1001808}
chromium · May 10, 2022 · 7b0078b · 7b0078b
1 parent 4daede5
commit 7b0078b
Show file tree

Hide file tree

Showing 9 changed files with 227 additions and 57 deletions.
diff --git a/ash/test/ash_test_base.cc b/ash/test/ash_test_base.cc
@@ -218,15 +218,23 @@ std::unique_ptr<views::Widget> AshTestBase::CreateFramelessTestWidget() {
 std::unique_ptr<aura::Window> AshTestBase::CreateAppWindow(
     const gfx::Rect& bounds_in_screen,
     AppType app_type,
-    int shell_window_id) {
+    int shell_window_id,
+    views::WidgetDelegate* delegate) {
   TestWidgetBuilder builder;
   if (app_type != AppType::NON_APP) {
     builder.SetWindowProperty(aura::client::kAppType,
                               static_cast<int>(app_type));
   }
+
+  if (delegate) {
+    builder.SetDelegate(delegate);
+  } else {
+    builder.SetTestWidgetDelegate();
+  }
+
   // |widget| is configured to be owned by the underlying window.
   views::Widget* widget =
-      builder.SetTestWidgetDelegate()
+      builder
           .SetBounds(bounds_in_screen.IsEmpty() ? gfx::Rect(0, 0, 300, 300)
                                                 : bounds_in_screen)
           .SetContext(Shell::GetPrimaryRootWindow())

diff --git a/ash/test/ash_test_base.h b/ash/test/ash_test_base.h
@@ -141,10 +141,13 @@ class AshTestBase : public testing::Test {
   // window, otherwise the window is added to the display matching
   // |bounds_in_screen|. |shell_window_id| is the shell window id to give to
   // the new window.
+  // If |delegate| is empty, a new |TestWidgetDelegate| instance will be set as
+  // this widget's delegate.
   std::unique_ptr<aura::Window> CreateAppWindow(
       const gfx::Rect& bounds_in_screen = gfx::Rect(),
       AppType app_type = AppType::SYSTEM_APP,
-      int shell_window_id = kShellWindowId_Invalid);
+      int shell_window_id = kShellWindowId_Invalid,
+      views::WidgetDelegate* delegate = nullptr);
 
   // Creates a visible window in the appropriate container. If
   // |bounds_in_screen| is empty the window is added to the primary root

diff --git a/ash/wm/desks/desk.cc b/ash/wm/desks/desk.cc
@@ -653,11 +653,7 @@ void Desk::RecordAndResetConsecutiveDailyVisits(bool being_removed) {
   first_day_visited_ = -1;
 }
 
-void Desk::CloseAllAppWindows() {
-  // Content changed notifications for this desk should be disabled when
-  // we are destroying the windows.
-  auto throttle_desk_notifications = GetScopedNotifyContentChangedDisabler();
-
+std::vector<aura::Window*> Desk::GetAllAppWindows() {
   // We need to copy the app windows from `windows_` into `app_windows` so
   // that we do not modify `windows_` in place. This also gives us a filtered
   // list with all of the app windows that we need to remove.
@@ -668,18 +664,7 @@ void Desk::CloseAllAppWindows() {
                                  static_cast<int>(AppType::NON_APP);
                         });
 
-  // We initialize `window_tracker` from the `app_windows` list, and
-  // pop and close windows from the `window_tracker` list. This avoids us
-  // revisiting windows that may have already been indirectly closed due to
-  // the closure of other windows, as the `window_tracker` automatically
-  // removes windows when they are closed.
-  aura::WindowTracker window_tracker(app_windows);
-  while (!window_tracker.windows().empty()) {
-    aura::Window* window = window_tracker.Pop();
-    views::Widget* widget = views::Widget::GetWidgetForNativeView(window);
-    DCHECK(widget);
-    widget->CloseNow();
-  }
+  return app_windows;
 }
 
 void Desk::MoveWindowToDeskInternal(aura::Window* window,

diff --git a/ash/wm/desks/desk.h b/ash/wm/desks/desk.h
@@ -184,8 +184,8 @@ class ASH_EXPORT Desk {
   // accounts for cases where the user removes the active desk.
   void RecordAndResetConsecutiveDailyVisits(bool being_removed);
 
-  // Closes all app windows associated with this desk.
-  void CloseAllAppWindows();
+  // Gets all app windows on this desk that should be closed.
+  std::vector<aura::Window*> GetAllAppWindows();
 
  private:
   friend class DesksTestApi;

diff --git a/ash/wm/desks/desk_animation_impl.cc b/ash/wm/desks/desk_animation_impl.cc
@@ -320,12 +320,14 @@ void DeskActivationAnimation::PrepareDeskForScreenshot(int index) {
 DeskRemovalAnimation::DeskRemovalAnimation(DesksController* controller,
                                            int desk_to_remove_index,
                                            int desk_to_activate_index,
-                                           DesksCreationRemovalSource source)
+                                           DesksCreationRemovalSource source,
+                                           DeskCloseType close_type)
     : DeskAnimationBase(controller,
                         desk_to_activate_index,
                         /*is_continuous_gesture_animation=*/false),
       desk_to_remove_index_(desk_to_remove_index),
-      request_source_(source) {
+      request_source_(source),
+      close_type_(close_type) {
   DCHECK(!Shell::Get()->overview_controller()->InOverviewSession());
   DCHECK_EQ(controller_->active_desk(),
             controller_->desks()[desk_to_remove_index_].get());
@@ -372,8 +374,7 @@ void DeskRemovalAnimation::OnDeskSwitchAnimationFinishedInternal() {
   // are destroyed.
   controller_->RemoveDeskInternal(
       controller_->desks()[desk_to_remove_index_].get(), request_source_,
-      DeskCloseType::kCombineDesks);
-
+      close_type_);
   MaybeRestoreSplitView(/*refresh_snapped_windows=*/true);
 }
 

diff --git a/ash/wm/desks/desk_animation_impl.h b/ash/wm/desks/desk_animation_impl.h
@@ -8,6 +8,7 @@
 #include "ash/ash_export.h"
 #include "ash/public/cpp/metrics_util.h"
 #include "ash/wm/desks/desk_animation_base.h"
+#include "ash/wm/desks/desks_controller.h"
 #include "ash/wm/desks/desks_histogram_enums.h"
 #include "base/memory/weak_ptr.h"
 #include "base/time/time.h"
@@ -18,8 +19,6 @@ class PresentationTimeRecorder;
 
 namespace ash {
 
-class DesksController;
-
 class ASH_EXPORT DeskActivationAnimation : public DeskAnimationBase {
  public:
   DeskActivationAnimation(DesksController* controller,
@@ -75,7 +74,8 @@ class DeskRemovalAnimation : public DeskAnimationBase {
   DeskRemovalAnimation(DesksController* controller,
                        int desk_to_remove_index,
                        int desk_to_activate_index,
-                       DesksCreationRemovalSource source);
+                       DesksCreationRemovalSource source,
+                       DeskCloseType close_type);
   DeskRemovalAnimation(const DeskRemovalAnimation&) = delete;
   DeskRemovalAnimation& operator=(const DeskRemovalAnimation&) = delete;
   ~DeskRemovalAnimation() override;
@@ -89,6 +89,7 @@ class DeskRemovalAnimation : public DeskAnimationBase {
  private:
   const int desk_to_remove_index_;
   const DesksCreationRemovalSource request_source_;
+  const DeskCloseType close_type_;
 };
 
 }  // namespace ash

diff --git a/ash/wm/desks/desks_controller.cc b/ash/wm/desks/desks_controller.cc
@@ -517,25 +517,12 @@ void DesksController::RemoveDesk(const Desk* desk,
     DCHECK_GE(target_desk_index, 0);
     DCHECK_LT(target_desk_index, static_cast<int>(desks_.size()));
     animation_ = std::make_unique<DeskRemovalAnimation>(
-        this, current_desk_index, target_desk_index, source);
+        this, current_desk_index, target_desk_index, source, close_type);
     animation_->Launch();
     return;
   }
 
-  // Cancelling the close-all toast closes the toast and also runs the callback
-  // that deletes any existing data in `temporary_removed_desk_`.
-  ToastManager::Get()->Cancel(kCloseAllToastID);
-
   RemoveDeskInternal(desk, source, close_type);
-
-  if (close_type == DeskCloseType::kCloseAllWindowsAndWait) {
-    ShowDeskRemovalUndoToast(
-        /*dismiss_callback=*/base::BindRepeating(
-            &DesksController::UndoDeskRemoval, base::Unretained(this)),
-        /*expired_callback=*/base::BindRepeating(
-            &DesksController::CommitPendingDeskRemoval,
-            base::Unretained(this)));
-  }
 }
 
 void DesksController::ReorderDesk(int old_index, int new_index) {
@@ -819,9 +806,10 @@ bool DesksController::MoveWindowFromActiveDeskTo(
 }
 
 void DesksController::AddVisibleOnAllDesksWindow(aura::Window* window) {
-  // Now that WorkspaceLayoutManager requests Add/MaybeRemoveVisibleOnAllDesksWindow
-  // when a child window is added in OnWindowAddedToLayout, the window could be
-  // the one that has already been added.
+  // Now that WorkspaceLayoutManager requests
+  // Add/MaybeRemoveVisibleOnAllDesksWindow when a child window is added in
+  // OnWindowAddedToLayout, the window could be the one that has already been
+  // added.
   if (!visible_on_all_desks_windows_.emplace(window).second)
     return;
   wm::AnimateWindow(window, wm::WINDOW_ANIMATION_TYPE_BOUNCE);
@@ -1368,6 +1356,10 @@ void DesksController::ActivateDeskInternal(const Desk* desk,
 void DesksController::RemoveDeskInternal(const Desk* desk,
                                          DesksCreationRemovalSource source,
                                          DeskCloseType close_type) {
+  // Cancelling the close-all toast closes the toast and also runs the callback
+  // that deletes any existing data in `temporary_removed_desk_`.
+  ToastManager::Get()->Cancel(kCloseAllToastID);
+
   DCHECK(CanRemoveDesks());
 
   base::AutoReset<bool> in_progress(&are_desks_being_modified_, true);
@@ -1539,6 +1531,15 @@ void DesksController::RemoveDeskInternal(const Desk* desk,
   desks_restore_util::UpdatePrimaryUserDeskMetricsPrefs();
 
   DCHECK_LE(available_container_ids_.size(), desks_util::kMaxNumberOfDesks);
+
+  if (close_type == DeskCloseType::kCloseAllWindowsAndWait) {
+    ShowDeskRemovalUndoToast(
+        /*dismiss_callback=*/base::BindRepeating(
+            &DesksController::UndoDeskRemoval, base::Unretained(this)),
+        /*expired_callback=*/base::BindRepeating(
+            &DesksController::CommitPendingDeskRemoval,
+            base::Unretained(this)));
+  }
 }
 
 void DesksController::UndoDeskRemoval() {
@@ -1591,7 +1592,46 @@ void DesksController::FinalizeDeskRemoval(RemovedDeskData* removed_desk_data) {
   // We need to ensure there are no app windows in the desk before destruction.
   // During a combine desks operation, the windows would have already been
   // moved, so in that case this would be a no-op.
-  removed_desk->CloseAllAppWindows();
+
+  // Content changed notifications for this desk should be disabled when
+  // we are destroying the windows.
+  auto throttle_desk_notifications =
+      removed_desk->GetScopedNotifyContentChangedDisabler();
+
+  std::vector<aura::Window*> app_windows = removed_desk->GetAllAppWindows();
+
+  // We use `closing_window_tracker` to track all app windows that should be
+  // closed from the removed desk, `WindowTracker` will handle windows that may
+  // have already been indirectly closed due to the closure of other windows, as
+  // the `WindowTracker` automatically removes windows when they are closed.
+  std::unique_ptr<aura::WindowTracker> closing_window_tracker =
+      std::make_unique<aura::WindowTracker>(app_windows);
+
+  for (aura::Window* window : app_windows) {
+    views::Widget* widget = views::Widget::GetWidgetForNativeView(window);
+    DCHECK(widget);
+
+    widget->Close();
+
+    // `widget->Close();` calls the underlying `native_widget_->Close()` which
+    // will schedule `native_widget_->CloseNow()` as an async task. Only
+    // when `native_widget_->CloseNow()` finishes running, the window will
+    // finally be removed from desk. Therefore, to remove the desk now, we have
+    // to manually remove the window from desk now.
+    removed_desk->RemoveWindowFromDesk(window);
+  }
+
+  // Schedules a delayed task to forcefully close all windows that have not
+  // finish closing.
+  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      FROM_HERE,
+      base::BindOnce(&DesksController::CleanUpClosedAppWindowsTask,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     std::move(closing_window_tracker)),
+      kCloseAllWindowCloseTimeout);
+
+  // Remove the desk.
+  temporary_removed_desk_.reset();
 }
 
 void DesksController::CommitPendingDeskRemoval() {
@@ -1600,6 +1640,25 @@ void DesksController::CommitPendingDeskRemoval() {
   temporary_removed_desk_.reset();
 }
 
+void DesksController::CleanUpClosedAppWindowsTask(
+    std::unique_ptr<aura::WindowTracker> closing_window_tracker) {
+  // We have waited long enough for these app windows to close cleanly.
+  // If there is any app windows still around, we will close them forcefully.
+  // These window's desk has already been removed. We should not let these
+  // windows linger around.
+  while (!closing_window_tracker->windows().empty()) {
+    aura::Window* window = closing_window_tracker->Pop();
+    views::Widget* widget = views::Widget::GetWidgetForNativeView(window);
+    DCHECK(widget);
+
+    // Forcefully close this app window. `CloseNow` which directly deleted the
+    // associated native widget. This will skip many Window shutdown hook
+    // logic. However, the desk controller has waited for the app window to
+    // close cleanly before this.
+    widget->CloseNow();
+  }
+}
+
 void DesksController::MoveVisibleOnAllDesksWindowsFromActiveDeskTo(
     Desk* new_desk) {
   // Ignore activations in the MRU tracker until we finish moving all visible on

diff --git a/ash/wm/desks/desks_controller.h b/ash/wm/desks/desks_controller.h
@@ -17,7 +17,9 @@
 #include "ash/wm/desks/templates/restore_data_collector.h"
 #include "base/containers/flat_map.h"
 #include "base/containers/flat_set.h"
+#include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "chromeos/ui/wm/desks/desks_helper.h"
 #include "components/account_id/account_id.h"
@@ -98,6 +100,11 @@ class ASH_EXPORT DesksController : public chromeos::DesksHelper,
     virtual ~Observer() = default;
   };
 
+  // The timeout duration that we allow an app window on a closed desk to run
+  // its "close" hooks before being forcefully closed.
+  static constexpr base::TimeDelta kCloseAllWindowCloseTimeout =
+      base::Seconds(1);
+
   DesksController();
 
   DesksController(const DesksController&) = delete;
@@ -376,6 +383,10 @@ class ASH_EXPORT DesksController : public chromeos::DesksHelper,
 
   void CommitPendingDeskRemoval();
 
+  // Forcefully cleans up app windows that should be closed.
+  void CleanUpClosedAppWindowsTask(
+      std::unique_ptr<aura::WindowTracker> closing_window_tracker);
+
   // Moves all the windows that are visible on all desks that currently
   // reside on |active_desk_| to |new_desk|.
   void MoveVisibleOnAllDesksWindowsFromActiveDeskTo(Desk* new_desk);
@@ -450,6 +461,10 @@ class ASH_EXPORT DesksController : public chromeos::DesksHelper,
 
   // Does the job for the `CaptureActiveDeskAsTemplate()` method.
   mutable RestoreDataCollector restore_data_collector_;
+
+  // Note: This should remain the last member so it'll be destroyed and
+  // invalidate its weak pointers before any other members are destroyed.
+  base::WeakPtrFactory<DesksController> weak_ptr_factory_{this};
 };
 
 }  // namespace ash