snap-group: Group cycle item creation and removal

This cl creates `GroupContainerView` for snap group in window cycle view, which will be the direct child view of the `mirror_container_`, which will be used as the focus target for snap group. This cl also handles window destruction, for free forms windows the `WindowCycleItemView` will be removed. For group item, however, we will only remove the child view that represents the destroying window. With the newly added `TryRemovingItem`, `WindowCycleView` would know whether to clean up the corresponding list or not. Fixed: b/293366018 Demo: http://b/293366018#comment2 Test: Added new unit tests + manual Change-Id: Ib6c207a5d88dff34b57d028e1b31967c1f272c47 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4786951 Commit-Queue: Michele Fan <michelefan@chromium.org> Reviewed-by: Ahmed Fakhry <afakhry@chromium.org> Cr-Commit-Position: refs/heads/main@{#1186902}
chromium · Aug 22, 2023 · 5bd5895 · 5bd5895
1 parent 45a094f
commit 5bd5895
Show file tree

Hide file tree

Showing 10 changed files with 204 additions and 50 deletions.
diff --git a/ash/wm/snap_group/snap_group_unittest.cc b/ash/wm/snap_group/snap_group_unittest.cc
@@ -32,6 +32,7 @@
 #include "ash/wm/tablet_mode/tablet_mode_controller_test_api.h"
 #include "ash/wm/window_cycle/window_cycle_controller.h"
 #include "ash/wm/window_cycle/window_cycle_list.h"
+#include "ash/wm/window_cycle/window_cycle_view.h"
 #include "ash/wm/window_state.h"
 #include "ash/wm/window_util.h"
 #include "ash/wm/wm_event.h"
@@ -494,6 +495,7 @@ class SnapGroupEntryPointArm1Test : public SnapGroupTest {
     WindowCycleController* window_cycle_controller =
         Shell::Get()->window_cycle_controller();
     window_cycle_controller->CompleteCycling();
+    EXPECT_FALSE(window_cycle_controller->IsCycling());
   }
 
   void CycleWindow(WindowCyclingDirection direction, int steps) {
@@ -1098,8 +1100,6 @@ TEST_F(SnapGroupEntryPointArm1Test, WindowReorderInAltTab) {
   // Test that the two windows in a snap group are reordered to be adjacent
   // with each other to reflect the window layout with the revised order as :
   // window2 --> window0--> window1.
-  // TODO(b/293365678): `cycle_list` should contain two items, update the test
-  // when the container view is implemented.
   ASSERT_EQ(windows.size(), 3u);
   EXPECT_EQ(windows.at(0), window2.get());
   EXPECT_EQ(windows.at(1), window0.get());
@@ -1116,6 +1116,65 @@ TEST_F(SnapGroupEntryPointArm1Test, WindowReorderInAltTab) {
   EXPECT_TRUE(wm::IsActiveWindow(window2.get()));
 }
 
+// Tests that the number of views to be cycled through inside the mirror
+// container view of window cycle view will be the number of free-form windows
+// plus snap groups.
+TEST_F(SnapGroupEntryPointArm1Test, WindowCycleViewTest) {
+  std::unique_ptr<aura::Window> window0(CreateTestWindowInShellWithId(0));
+  std::unique_ptr<aura::Window> window1(CreateTestWindowInShellWithId(1));
+  std::unique_ptr<aura::Window> window2(CreateTestWindowInShellWithId(2));
+  SnapTwoTestWindowsInArm1(window0.get(), window1.get());
+
+  WindowCycleController* window_cycle_controller =
+      Shell::Get()->window_cycle_controller();
+  CycleWindow(WindowCyclingDirection::kForward, /*steps=*/3);
+  const auto* window_cycle_list = window_cycle_controller->window_cycle_list();
+  const auto& windows = window_cycle_list->windows_for_testing();
+  EXPECT_EQ(windows.size(), 3u);
+
+  const WindowCycleView* cycle_view = window_cycle_list->cycle_view();
+  ASSERT_TRUE(cycle_view);
+  EXPECT_EQ(cycle_view->mirror_container_for_testing()->children().size(), 2u);
+  CompleteWindowCycling();
+}
+
+// Tests that on window that belongs to a snap group destroying while cycling
+// the window list with Alt + Tab, there will be no crash. The corresponding
+// child mini view hosted by the group container view will be destroyed, the
+// group container view will host the other child mini view.
+TEST_F(SnapGroupEntryPointArm1Test, WindowInSnapGroupDestructionInAltTab) {
+  std::unique_ptr<aura::Window> window0(CreateTestWindowInShellWithId(0));
+  std::unique_ptr<aura::Window> window1(CreateTestWindowInShellWithId(1));
+  std::unique_ptr<aura::Window> window2(CreateTestWindowInShellWithId(2));
+  SnapTwoTestWindowsInArm1(window0.get(), window1.get());
+
+  WindowCycleController* window_cycle_controller =
+      Shell::Get()->window_cycle_controller();
+  CycleWindow(WindowCyclingDirection::kForward, /*steps=*/3);
+  const auto* window_cycle_list = window_cycle_controller->window_cycle_list();
+  const auto& windows = window_cycle_list->windows_for_testing();
+  EXPECT_EQ(windows.size(), 3u);
+
+  const WindowCycleView* cycle_view = window_cycle_list->cycle_view();
+  ASSERT_TRUE(cycle_view);
+  // Verify that the number of child views hosted by mirror container is two at
+  // the beginning.
+  EXPECT_EQ(cycle_view->mirror_container_for_testing()->children().size(), 2u);
+
+  // Destroy `window0` which belongs to a snap group.
+  window0.reset();
+  const auto* updated_window_cycle_list =
+      window_cycle_controller->window_cycle_list();
+  const auto& updated_windows =
+      updated_window_cycle_list->windows_for_testing();
+  // Verify that the updated windows list size decreased.
+  EXPECT_EQ(updated_windows.size(), 2u);
+
+  // Verify that the number of child views hosted by mirror container will still
+  // be two.
+  EXPECT_EQ(cycle_view->mirror_container_for_testing()->children().size(), 2u);
+}
+
 // Tests that after creating a snap group in clamshell, transition to tablet
 // mode won't crash (b/288179725).
 TEST_F(SnapGroupEntryPointArm1Test, NoCrashWhenRemovingGroupInTabletMode) {

diff --git a/ash/wm/window_cycle/window_cycle_controller.cc b/ash/wm/window_cycle/window_cycle_controller.cc
@@ -4,15 +4,12 @@
 
 #include "ash/wm/window_cycle/window_cycle_controller.h"
 
-#include "ash/accelerators/accelerator_controller_impl.h"
 #include "ash/accessibility/accessibility_controller_impl.h"
 #include "ash/constants/ash_pref_names.h"
 #include "ash/events/event_rewriter_controller_impl.h"
 #include "ash/metrics/task_switch_metrics_recorder.h"
 #include "ash/metrics/task_switch_source.h"
 #include "ash/metrics/user_metrics_recorder.h"
-#include "ash/public/cpp/accelerators.h"
-#include "ash/public/cpp/window_properties.h"
 #include "ash/session/session_controller_impl.h"
 #include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
@@ -25,7 +22,6 @@
 #include "ash/wm/snap_group/snap_group.h"
 #include "ash/wm/window_cycle/window_cycle_event_filter.h"
 #include "ash/wm/window_cycle/window_cycle_list.h"
-#include "ash/wm/window_state.h"
 #include "ash/wm/window_util.h"
 #include "base/check.h"
 #include "base/containers/contains.h"

diff --git a/ash/wm/window_cycle/window_cycle_item_view.cc b/ash/wm/window_cycle/window_cycle_item_view.cc
@@ -144,7 +144,7 @@ void WindowCycleItemView::RefreshItemVisuals() {
 BEGIN_METADATA(WindowCycleItemView, WindowMiniView)
 END_METADATA
 
-GroupContainerView::GroupContainerView(SnapGroup* snap_group) {
+GroupContainerCycleView::GroupContainerCycleView(SnapGroup* snap_group) {
   mini_view1_ = AddChildView(
       std::make_unique<WindowCycleItemView>(snap_group->window1()));
   mini_view2_ = AddChildView(
@@ -153,7 +153,6 @@ GroupContainerView::GroupContainerView(SnapGroup* snap_group) {
   SetFocusBehavior(FocusBehavior::ALWAYS);
   SetPaintToLayer();
   layer()->SetFillsBoundsOpaquely(false);
-  SetAccessibleName(u"Group container view");
 
   // TODO(michelefan@): Orientation should correspond to the window layout.
   views::BoxLayout* layout =
@@ -164,23 +163,62 @@ GroupContainerView::GroupContainerView(SnapGroup* snap_group) {
       views::BoxLayout::CrossAxisAlignment::kCenter);
 }
 
-GroupContainerView::~GroupContainerView() = default;
+GroupContainerCycleView::~GroupContainerCycleView() = default;
 
-bool GroupContainerView::Contains(aura::Window* window) const {
-  return mini_view1_->Contains(window) || mini_view2_->Contains(window);
+bool GroupContainerCycleView::Contains(aura::Window* window) const {
+  return (mini_view1_ && mini_view1_->Contains(window)) ||
+         (mini_view2_ && mini_view2_->Contains(window));
 }
 
-aura::Window* GroupContainerView::GetWindowAtPoint(
+aura::Window* GroupContainerCycleView::GetWindowAtPoint(
     const gfx::Point& screen_point) const {
   for (auto mini_view : {mini_view1_, mini_view2_}) {
+    if (!mini_view) {
+      continue;
+    }
     if (auto* window = mini_view->GetWindowAtPoint(screen_point)) {
       return window;
     }
   }
   return nullptr;
 }
 
-BEGIN_METADATA(GroupContainerView, WindowMiniViewBase)
+void GroupContainerCycleView::RefreshItemVisuals() {
+  for (auto mini_view : {mini_view1_, mini_view2_}) {
+    if (mini_view) {
+      mini_view->RefreshItemVisuals();
+    }
+  }
+}
+
+int GroupContainerCycleView::TryRemovingChildItem(
+    aura::Window* destroying_window) {
+  std::vector<raw_ptr<WindowCycleItemView>*> mini_views_ptrs = {&mini_view1_,
+                                                                &mini_view2_};
+  for (auto* mini_view_ptr : mini_views_ptrs) {
+    if (auto& mini_view = *mini_view_ptr;
+        mini_view && mini_view->Contains(destroying_window)) {
+      auto* temp = mini_view.get();
+      // Explicitly reset the `mini_view` to avoid dangling pointer detection.
+      mini_view = nullptr;
+      RemoveChildViewT(temp);
+    }
+  }
+
+  return base::ranges::count_if(
+      mini_views_ptrs,
+      [](raw_ptr<WindowCycleItemView>* ptr) { return *ptr != nullptr; });
+}
+
+void GroupContainerCycleView::GetAccessibleNodeData(ui::AXNodeData* node_data) {
+  views::View::GetAccessibleNodeData(node_data);
+  node_data->role = ax::mojom::Role::kGroup;
+  // TODO(b/297062026): Update the string after been finalized by consulting
+  // with a11y team.
+  node_data->SetName(u"Group container view");
+}
+
+BEGIN_METADATA(GroupContainerCycleView, WindowMiniViewBase)
 END_METADATA
 
 }  // namespace ash
diff --git a/ash/wm/window_cycle/window_cycle_item_view.h b/ash/wm/window_cycle/window_cycle_item_view.h
@@ -10,12 +10,15 @@
 #include "base/memory/raw_ptr.h"
 #include "ui/base/metadata/metadata_header_macros.h"
 #include "ui/events/event.h"
-#include "ui/gfx/geometry/size.h"
 
 namespace aura {
 class Window;
 }  // namespace aura
 
+namespace gfx {
+class Size;
+}  // namespace gfx
+
 namespace ash {
 
 class SnapGroup;
@@ -49,22 +52,29 @@ class ASH_EXPORT WindowCycleItemView : public WindowMiniView {
 
 // Container view used to host multiple `WindowCycleItemView`s and be the focus
 // target for window groups while tabbing in window cycle view.
-class GroupContainerView : public WindowMiniViewBase {
+class GroupContainerCycleView : public WindowMiniViewBase {
  public:
-  METADATA_HEADER(GroupContainerView);
+  METADATA_HEADER(GroupContainerCycleView);
 
-  explicit GroupContainerView(SnapGroup* snap_group);
-  GroupContainerView(const GroupContainerView&) = delete;
-  GroupContainerView& operator=(const GroupContainerView&) = delete;
-  ~GroupContainerView() override;
+  explicit GroupContainerCycleView(SnapGroup* snap_group);
+  GroupContainerCycleView(const GroupContainerCycleView&) = delete;
+  GroupContainerCycleView& operator=(const GroupContainerCycleView&) = delete;
+  ~GroupContainerCycleView() override;
 
   // WindowMiniViewBase:
   bool Contains(aura::Window* window) const override;
   aura::Window* GetWindowAtPoint(const gfx::Point& screen_point) const override;
+  void RefreshItemVisuals() override;
+  int TryRemovingChildItem(aura::Window* destroying_window) override;
+
+  // views::View:
+  void GetAccessibleNodeData(ui::AXNodeData* node_data) override;
 
  private:
-  raw_ptr<WindowCycleItemView, ExperimentalAsh> mini_view1_;
-  raw_ptr<WindowCycleItemView, ExperimentalAsh> mini_view2_;
+  // TODO(b/297070130): Use vector store the child `WindowCycleItemView` hosted
+  // by this.
+  raw_ptr<WindowCycleItemView> mini_view1_;
+  raw_ptr<WindowCycleItemView> mini_view2_;
 };
 
 }  // namespace ash

diff --git a/ash/wm/window_cycle/window_cycle_list.cc b/ash/wm/window_cycle/window_cycle_list.cc
@@ -6,7 +6,6 @@
 
 #include "ash/accessibility/accessibility_controller_impl.h"
 #include "ash/app_list/app_list_controller_impl.h"
-#include "ash/constants/ash_features.h"
 #include "ash/frame_throttler/frame_throttling_controller.h"
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/public/cpp/window_properties.h"
@@ -16,12 +15,12 @@
 #include "ash/system/tray/tray_background_view.h"
 #include "ash/wm/mru_window_tracker.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
+#include "ash/wm/window_cycle/window_cycle_view.h"
 #include "ash/wm/window_state.h"
 #include "ash/wm/window_util.h"
 #include "base/check.h"
 #include "base/location.h"
 #include "base/metrics/histogram_functions.h"
-#include "base/metrics/user_metrics.h"
 #include "base/ranges/algorithm.h"
 #include "ui/aura/scoped_window_targeter.h"
 #include "ui/aura/window.h"
@@ -283,7 +282,6 @@ void WindowCycleList::OnWindowDestroying(aura::Window* window) {
   window->RemoveObserver(this);
 
   WindowList::iterator i = base::ranges::find(windows_, window);
-  // TODO(oshima): Change this back to DCHECK once crbug.com/483491 is fixed.
   CHECK(i != windows_.end());
   int removed_index = static_cast<int>(i - windows_.begin());
   windows_.erase(i);

diff --git a/ash/wm/window_cycle/window_cycle_list.h b/ash/wm/window_cycle/window_cycle_list.h
@@ -10,14 +10,11 @@
 
 #include "ash/ash_export.h"
 #include "ash/wm/window_cycle/window_cycle_controller.h"
-#include "ash/wm/window_cycle/window_cycle_view.h"
 #include "base/memory/raw_ptr.h"
 #include "base/timer/timer.h"
 #include "ui/aura/window_observer.h"
 #include "ui/display/display_observer.h"
-#include "ui/display/screen.h"
-#include "ui/views/controls/label.h"
-#include "ui/views/view.h"
+#include "ui/events/event.h"
 
 namespace aura {
 class ScopedWindowTargeter;
@@ -30,6 +27,8 @@ class Widget;
 
 namespace ash {
 
+class WindowCycleView;
+
 // Tracks a set of Windows that can be stepped through. This class is used by
 // the WindowCycleController.
 class ASH_EXPORT WindowCycleList : public aura::WindowObserver,