Fix ink drop crash in holding space item views.

When deleting holding space item views (such as occurs when unpinning)
the ink drop layers for the view can attempt to be reordered on the
parent which causes a crash. This is most easily reproducible by trying
to unpin the last pinned file in the pinned files section.

Note that the solution reparents ink drop layers to be within the
holding space item view hierarchy which also addresses an issue where
ink drop layers did not animate in/out with the layer for animating
holding space item view.

Bug: 1157280
Change-Id: I4d7daf831e46d3bdd7bf049b49e15b53319cc2c0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587519
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Toni Baržić <tbarzic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836400}

ash/public/cpp/holding_space/holding_space_constants.h

@@ -54,11 +54,12 @@ enum HoldingSpaceCommandId {
 };
 
 // View IDs.
-constexpr int kHoldingSpacePinnedFilesBubbleId = 1;
-constexpr int kHoldingSpaceRecentFilesBubbleId = 2;
-constexpr int kHoldingSpaceScreenCapturePlayIconId = 3;
-constexpr int kHoldingSpaceTrayDefaultIconId = 4;
-constexpr int kHoldingSpaceTrayPreviewsIconId = 5;
+constexpr int kHoldingSpaceItemPinButtonId = 1;
+constexpr int kHoldingSpacePinnedFilesBubbleId = 2;
+constexpr int kHoldingSpaceRecentFilesBubbleId = 3;
+constexpr int kHoldingSpaceScreenCapturePlayIconId = 4;
+constexpr int kHoldingSpaceTrayDefaultIconId = 5;
+constexpr int kHoldingSpaceTrayPreviewsIconId = 6;
 
 // The maximum allowed age for files restored into the holding space model.
 // Note that this is not enforced for pinned items.

ash/system/holding_space/holding_space_item_view.cc

@@ -23,6 +23,7 @@
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/accessibility/view_accessibility.h"
+#include "ui/views/animation/ink_drop.h"
 #include "ui/views/background.h"
 #include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/highlight_path_generator.h"
@@ -156,6 +157,12 @@ HoldingSpaceItemView::HoldingSpaceItemView(
   layer()->Add(selected_layer_owner_->layer());
 
   // Ink drop.
+  // Note that `ink_drop_container_` is added to the view hierarchy to parent
+  // any created ink drop layers. This will allow ink drop layers to animate
+  // in/out with the layer for this view as well as fix a crash in which ink
+  // drop layers were attempted to be reordered during destruction of this view.
+  ink_drop_container_ =
+      AddChildView(std::make_unique<views::InkDropContainerView>());
   SetInkDropMode(InkDropMode::ON_NO_GESTURE_HANDLER);
   SetInkDropVisibleOpacity(
       AshColorProvider::Get()->GetRippleAttributes().inkdrop_opacity);
@@ -180,6 +187,14 @@ bool HoldingSpaceItemView::IsInstance(views::View* view) {
   return view->GetProperty(kIsHoldingSpaceItemViewProperty);
 }
 
+void HoldingSpaceItemView::AddLayerBeneathView(ui::Layer* layer) {
+  ink_drop_container_->AddInkDropLayer(layer);
+}
+
+void HoldingSpaceItemView::RemoveLayerBeneathView(ui::Layer* layer) {
+  ink_drop_container_->RemoveInkDropLayer(layer);
+}
+
 SkColor HoldingSpaceItemView::GetInkDropBaseColor() const {
   return AshColorProvider::Get()->GetRippleAttributes().base_color;
 }
@@ -289,6 +304,7 @@ views::ToggleImageButton* HoldingSpaceItemView::AddPin(views::View* parent) {
   DCHECK(!pin_);
 
   pin_ = parent->AddChildView(std::make_unique<views::ToggleImageButton>());
+  pin_->SetID(kHoldingSpaceItemPinButtonId);
   pin_->SetFocusBehavior(views::View::FocusBehavior::ACCESSIBLE_ONLY);
   pin_->SetVisible(false);
 

ash/system/holding_space/holding_space_item_view.h

@@ -19,6 +19,7 @@ class LayerAnimationObserver;
 }  // namespace ui
 
 namespace views {
+class InkDropContainerView;
 class ToggleImageButton;
 }  // namespace views
 
@@ -49,6 +50,8 @@ class ASH_EXPORT HoldingSpaceItemView : public views::InkDropHostView,
   static bool IsInstance(views::View* view);
 
   // views::InkDropHostView:
+  void AddLayerBeneathView(ui::Layer* layer) override;
+  void RemoveLayerBeneathView(ui::Layer* layer) override;
   SkColor GetInkDropBaseColor() const override;
   bool HandleAccessibleAction(const ui::AXActionData& action_data) override;
   void OnBoundsChanged(const gfx::Rect& previous_bounds) override;
@@ -105,7 +108,9 @@ class ASH_EXPORT HoldingSpaceItemView : public views::InkDropHostView,
   // destroyed if this view is in the process of animating out.
   const std::string item_id_;
 
-  views::ToggleImageButton* pin_ = nullptr;  // Owned by view hierarchy.
+  // Owned by view hierarchy.
+  views::InkDropContainerView* ink_drop_container_ = nullptr;
+  views::ToggleImageButton* pin_ = nullptr;
 
   // Owners for the layers used to paint focused and selected states.
   std::unique_ptr<ui::LayerOwner> selected_layer_owner_;

chrome/browser/ui/ash/holding_space/holding_space_ui_browsertest.cc

@@ -6,12 +6,14 @@
 
 #include "ash/public/cpp/ash_features.h"
 #include "ash/public/cpp/capture_mode_test_api.h"
+#include "ash/public/cpp/holding_space/holding_space_constants.h"
 #include "ash/public/cpp/holding_space/holding_space_controller.h"
 #include "ash/public/cpp/holding_space/holding_space_item.h"
 #include "ash/public/cpp/holding_space/holding_space_model.h"
 #include "ash/public/cpp/holding_space/holding_space_model_observer.h"
 #include "ash/public/cpp/holding_space/holding_space_prefs.h"
 #include "ash/public/cpp/holding_space/holding_space_test_api.h"
+#include "base/scoped_observation.h"
 #include "base/scoped_observer.h"
 #include "base/test/bind.h"
 #include "chrome/browser/profiles/profile.h"
@@ -45,6 +47,14 @@ void FlushMessageLoop() {
   run_loop.Run();
 }
 
+// Performs a click on `view`.
+void Click(const views::View* view) {
+  auto* root_window = HoldingSpaceBrowserTestBase::GetRootWindowForNewWindows();
+  ui::test::EventGenerator event_generator(root_window);
+  event_generator.MoveMouseTo(view->GetBoundsInScreen().CenterPoint());
+  event_generator.ClickLeftButton();
+}
+
 // Performs a double click on `view`.
 void DoubleClick(const views::View* view) {
   auto* root_window = HoldingSpaceBrowserTestBase::GetRootWindowForNewWindows();
@@ -88,6 +98,13 @@ void MouseDrag(const views::View* from, const views::View* to) {
   event_generator.ReleaseLeftButton();
 }
 
+// Moves mouse to `view` over `count` number of events.
+void MoveMouseTo(const views::View* view, size_t count = 1u) {
+  auto* root_window = HoldingSpaceBrowserTestBase::GetRootWindowForNewWindows();
+  ui::test::EventGenerator event_generator(root_window);
+  event_generator.MoveMouseTo(view->GetBoundsInScreen().CenterPoint(), count);
+}
+
 // Performs a press and release of the specified `key_code` with `flags`.
 void PressAndReleaseKey(ui::KeyboardCode key_code, int flags = ui::EF_NONE) {
   auto* root_window = HoldingSpaceBrowserTestBase::GetRootWindowForNewWindows();
@@ -184,6 +201,53 @@ class DropTargetView : public views::WidgetDelegateView {
   base::FilePath copied_file_path_;
 };
 
+// ViewDrawnWaiter -------------------------------------------------------------
+
+class ViewDrawnWaiter : public views::ViewObserver {
+ public:
+  ViewDrawnWaiter() = default;
+  ViewDrawnWaiter(const ViewDrawnWaiter&) = delete;
+  ViewDrawnWaiter& operator=(const ViewDrawnWaiter&) = delete;
+  ~ViewDrawnWaiter() override = default;
+
+  void Wait(views::View* view) {
+    if (IsDrawn(view))
+      return;
+
+    DCHECK(!wait_loop_);
+    DCHECK(!view_observer_.IsObserving());
+
+    view_observer_.Observe(view);
+
+    wait_loop_ = std::make_unique<base::RunLoop>();
+    wait_loop_->Run();
+    wait_loop_.reset();
+
+    view_observer_.Reset();
+  }
+
+ private:
+  // views::ViewObserver:
+  void OnViewVisibilityChanged(views::View* view,
+                               views::View* starting_view) override {
+    if (IsDrawn(view))
+      wait_loop_->Quit();
+  }
+
+  void OnViewBoundsChanged(views::View* view) override {
+    if (IsDrawn(view))
+      wait_loop_->Quit();
+  }
+
+  bool IsDrawn(views::View* view) {
+    return view->IsDrawn() && !view->size().IsEmpty();
+  }
+
+  std::unique_ptr<base::RunLoop> wait_loop_;
+  base::ScopedObservation<views::View, views::ViewObserver> view_observer_{
+      this};
+};
+
 // HoldingSpaceUiBrowserTest ---------------------------------------------------
 
 // Base class for holding space UI browser tests.
@@ -376,6 +440,41 @@ IN_PROC_BROWSER_TEST_F(HoldingSpaceUiBrowserTest, OpenItem) {
   }
 }
 
+// Verifies that unpinning a pinned holding space item works as intended.
+IN_PROC_BROWSER_TEST_F(HoldingSpaceUiBrowserTest, UnpinItem) {
+  ui::ScopedAnimationDurationScaleMode scoped_animation_duration_scale_mode(
+      ui::ScopedAnimationDurationScaleMode::ZERO_DURATION);
+
+  // Add enough pinned items for there to be multiple rows in the section.
+  constexpr size_t kNumPinnedItems = 3u;
+  for (size_t i = 0; i < kNumPinnedItems; ++i)
+    AddPinnedFile();
+
+  Show();
+  ASSERT_TRUE(IsShowing());
+
+  std::vector<views::View*> pinned_file_chips = GetPinnedFileChips();
+  ASSERT_EQ(kNumPinnedItems, pinned_file_chips.size());
+
+  // Operate on the last `pinned_file_chip` as there was an easy to reproduce
+  // bug in which unpinning a chip *not* in the top row resulted in a crash on
+  // destruction due to its ink drop layer attempting to be reordered.
+  views::View* pinned_file_chip = pinned_file_chips.back();
+
+  // The pin button is only visible after mousing over the `pinned_file_chip`,
+  // so move the mouse and wait for the pin button to be drawn. Note that the
+  // mouse is moved over multiple events to ensure that the appropriate mouse
+  // enter event is also generated.
+  MoveMouseTo(pinned_file_chip, /*count=*/10);
+  auto* pin_btn = pinned_file_chip->GetViewByID(kHoldingSpaceItemPinButtonId);
+  ViewDrawnWaiter().Wait(pin_btn);
+
+  Click(pin_btn);
+
+  pinned_file_chips = GetPinnedFileChips();
+  ASSERT_EQ(kNumPinnedItems - 1, pinned_file_chips.size());
+}
+
 // Base class for holding space UI browser tests that test previews.
 class HoldingSpaceUiPreviewsBrowserTest : public HoldingSpaceUiBrowserTest {
  public: