Virtual Desks: Disallow moving windows that don't belong to desks

Previously this used to crash on a DCHECK. Now we should explicitly
disallow this, since an active window (requested to be moved to
another desk via shortcut for example) might be an always-on-top
window, or a pip.

BUG=988886
TEST=Added new test

Change-Id: Iacb3562e8cb8bd789b9e3a88ac6b977242a79c7e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1725549
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682337}

ash/accelerators/accelerator_controller_impl.cc

@@ -316,9 +316,11 @@ void HandleMoveActiveItem(const ui::Accelerator& accelerator) {
         /*going_left=*/accelerator.key_code() == ui::VKEY_OEM_4);
   }
 
-  desks_controller->MoveWindowFromActiveDeskTo(
-      window_to_move, target_desk,
-      DesksMoveWindowFromActiveDeskSource::kShortcut);
+  if (!desks_controller->MoveWindowFromActiveDeskTo(
+          window_to_move, target_desk,
+          DesksMoveWindowFromActiveDeskSource::kShortcut)) {
+    return;
+  }
 
   if (in_overview) {
     // We should not exit overview as a result of this shortcut.

ash/wm/desks/desks_controller.cc

@@ -359,12 +359,17 @@ bool DesksController::ActivateAdjacentDesk(bool going_left,
   return true;
 }
 
-void DesksController::MoveWindowFromActiveDeskTo(
+bool DesksController::MoveWindowFromActiveDeskTo(
     aura::Window* window,
     Desk* target_desk,
     DesksMoveWindowFromActiveDeskSource source) {
   DCHECK_NE(active_desk_, target_desk);
 
+  // An active window might be an always-on-top or pip which doesn't belong to
+  // the active desk, and hence cannot be removed.
+  if (!base::Contains(active_desk_->windows(), window))
+    return false;
+
   base::AutoReset<bool> in_progress(&are_desks_being_modified_, true);
 
   auto* overview_controller = Shell::Get()->overview_controller();
@@ -380,6 +385,9 @@ void DesksController::MoveWindowFromActiveDeskTo(
           base::NumberToString16(GetDeskIndex(active_desk_) + 1),
           base::NumberToString16(GetDeskIndex(target_desk) + 1)));
 
+  UMA_HISTOGRAM_ENUMERATION(kMoveWindowFromActiveDeskHistogramName, source);
+  ReportNumberOfWindowsPerDeskHistogram();
+
   if (in_overview) {
     DCHECK(overview_controller->InOverviewSession());
     auto* overview_session = overview_controller->overview_session();
@@ -393,14 +401,12 @@ void DesksController::MoveWindowFromActiveDeskTo(
     // When in overview, we should return immediately and not change the window
     // activation as we do below, since the dummy "OverviewModeFocusedWidget"
     // should remain active while overview mode is active..
-    return;
+    return true;
   }
 
-  UMA_HISTOGRAM_ENUMERATION(kMoveWindowFromActiveDeskHistogramName, source);
-  ReportNumberOfWindowsPerDeskHistogram();
-
   // A window moving out of the active desk cannot be active.
   wm::DeactivateWindow(window);
+  return true;
 }
 
 void DesksController::OnRootWindowAdded(aura::Window* root_window) {

ash/wm/desks/desks_controller.h

@@ -108,7 +108,9 @@ class ASH_EXPORT DesksController
   // Moves |window| (which must belong to the currently active desk) to
   // |target_desk| (which must be a different desk). If |window| is minimized,
   // it will be unminimized after it's moved to |target_desk|.
-  void MoveWindowFromActiveDeskTo(aura::Window* window,
+  // Returns true on success, false otherwise (e.g. if |window| doesn't belong
+  // to the active desk).
+  bool MoveWindowFromActiveDeskTo(aura::Window* window,
                                   Desk* target_desk,
                                   DesksMoveWindowFromActiveDeskSource source);
 

ash/wm/desks/desks_unittests.cc

@@ -38,6 +38,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/client/window_parenting_client.h"
+#include "ui/base/ui_base_types.h"
 #include "ui/chromeos/events/event_rewriter_chromeos.h"
 #include "ui/compositor_extra/shadow.h"
 #include "ui/events/gesture_detection/gesture_configuration.h"
@@ -2162,6 +2163,34 @@ TEST_F(DesksAcceleratorsTest, MoveWindowLeftRightDeskOverview) {
   EXPECT_FALSE(overview_session->GetHighlightedWindow());
 }
 
+TEST_F(DesksAcceleratorsTest, CannotMoveAlwaysOnTopWindows) {
+  auto* controller = DesksController::Get();
+  NewDesk();
+  ASSERT_EQ(2u, controller->desks().size());
+  Desk* desk_1 = controller->desks()[0].get();
+  Desk* desk_2 = controller->desks()[1].get();
+  EXPECT_TRUE(desk_1->is_active());
+
+  // An always-on-top window does not belong to any desk and hence cannot be
+  // removed.
+  auto win0 = CreateTestWindow(gfx::Rect(0, 0, 250, 100));
+  win0->SetProperty(aura::client::kZOrderingKey,
+                    ui::ZOrderLevel::kFloatingWindow);
+  wm::ActivateWindow(win0.get());
+  EXPECT_EQ(win0.get(), window_util::GetActiveWindow());
+  EXPECT_FALSE(DoesActiveDeskContainWindow(win0.get()));
+  EXPECT_FALSE(controller->MoveWindowFromActiveDeskTo(
+      win0.get(), desk_2, DesksMoveWindowFromActiveDeskSource::kDragAndDrop));
+  const int flags = ui::EF_COMMAND_DOWN | ui::EF_SHIFT_DOWN;
+  SendAccelerator(ui::VKEY_OEM_4, flags);
+  EXPECT_EQ(win0.get(), window_util::GetActiveWindow());
+  EXPECT_TRUE(win0->IsVisible());
+
+  // It remains visible even after switching desks.
+  ActivateDesk(desk_2);
+  EXPECT_TRUE(win0->IsVisible());
+}
+
 // TODO(afakhry): Add more tests:
 // - Always on top windows are not tracked by any desk.
 // - Reusing containers when desks are removed and created.

ash/wm/overview/overview_grid.cc

@@ -1230,10 +1230,9 @@ bool OverviewGrid::MaybeDropItemOnDeskMiniView(
     if (target_desk == desks_controller->active_desk())
       return false;
 
-    desks_controller->MoveWindowFromActiveDeskTo(
+    return desks_controller->MoveWindowFromActiveDeskTo(
         dragged_window, target_desk,
         DesksMoveWindowFromActiveDeskSource::kDragAndDrop);
-    return true;
   }
 
   return false;