Ensure Native Widget owns Widget and UniqueWidgetPtr usage.

Added unit test to ensure full destruction of the Widget.

Bug: 1146238
Change-Id: I4b5a463e5f657fb3084f664f1ecc2acf0e2ac0a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2527849
Commit-Queue: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825981}

ash/capture_mode/capture_mode_controller.cc

@@ -608,6 +608,11 @@ void CaptureModeController::RecordNumberOfScreenshotsTakenInLastWeek() {
 }
 
 void CaptureModeController::OnVideoRecordCountDownFinished() {
+  // If this event is dispatched after the capture session was cancelled or
+  // destroyed, this should be a no-op.
+  if (!IsActive())
+    return;
+
   const base::Optional<CaptureParams> capture_params = GetCaptureParams();
   // Stop the capture session now, so the bar doesn't show up in the captured
   // video.

ash/capture_mode/capture_mode_session.cc

@@ -167,7 +167,6 @@ views::Widget::InitParams CreateWidgetParams(aura::Window* parent,
   // Use a popup widget to get transient properties, such as not needing to
   // click on the widget first to get capture before receiving events.
   views::Widget::InitParams params(views::Widget::InitParams::TYPE_POPUP);
-  params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
   params.opacity = views::Widget::InitParams::WindowOpacity::kTranslucent;
   params.parent = parent;
   params.bounds = bounds;
@@ -270,12 +269,12 @@ CaptureModeSession::CaptureModeSession(CaptureModeController* controller)
   parent->layer()->Add(layer());
   layer()->SetBounds(parent->bounds());
 
-  capture_mode_bar_widget_.Init(
+  capture_mode_bar_widget_->Init(
       CreateWidgetParams(parent, CaptureModeBarView::GetBounds(current_root_),
                          "CaptureModeBarWidget"));
-  capture_mode_bar_view_ = capture_mode_bar_widget_.SetContentsView(
+  capture_mode_bar_view_ = capture_mode_bar_widget_->SetContentsView(
       std::make_unique<CaptureModeBarView>());
-  capture_mode_bar_widget_.Show();
+  capture_mode_bar_widget_->Show();
 
   UpdateCaptureLabelWidget();
   RefreshStackingOrder(parent);
@@ -320,7 +319,7 @@ void CaptureModeSession::OnCaptureSourceChanged(CaptureModeSource new_source) {
 
   if (new_source == CaptureModeSource::kRegion) {
     cursor_setter_ = std::make_unique<ScopedCursorSetter>(
-        capture_mode_bar_widget_.GetWindowBoundsInScreen().Contains(
+        capture_mode_bar_widget_->GetWindowBoundsInScreen().Contains(
             previous_location_in_root_)
             ? ui::mojom::CursorType::kPointer
             : ui::mojom::CursorType::kCell);
@@ -351,7 +350,7 @@ void CaptureModeSession::StartCountDown(
   UpdateCaptureLabelWidgetBounds(/*animate=*/true);
 
   // Fade out toolbar.
-  ui::Layer* toolbar_layer = capture_mode_bar_widget_.GetLayer();
+  ui::Layer* toolbar_layer = capture_mode_bar_widget_->GetLayer();
   ui::ScopedLayerAnimationSettings toolbar_settings(
       toolbar_layer->GetAnimator());
   toolbar_settings.SetTransitionDuration(kCaptureBarFadeOutDuration);
@@ -419,11 +418,11 @@ void CaptureModeSession::OnMouseEvent(ui::MouseEvent* event) {
   if (event->type() == ui::ET_MOUSE_RELEASED &&
       source == CaptureModeSource::kRegion &&
       current_root_ !=
-          capture_mode_bar_widget_.GetNativeWindow()->GetRootWindow()) {
-    capture_mode_bar_widget_.SetBounds(
+          capture_mode_bar_widget_->GetNativeWindow()->GetRootWindow()) {
+    capture_mode_bar_widget_->SetBounds(
         CaptureModeBarView::GetBounds(current_root_));
     auto* parent = GetParentContainer(current_root_);
-    parent->StackChildAtTop(capture_mode_bar_widget_.GetNativeWindow());
+    parent->StackChildAtTop(capture_mode_bar_widget_->GetNativeWindow());
   }
 
   OnLocatedEvent(event, /*is_touch=*/false);
@@ -453,7 +452,7 @@ gfx::Rect CaptureModeSession::GetSelectedWindowBounds() const {
 
 void CaptureModeSession::RefreshStackingOrder(aura::Window* parent_container) {
   DCHECK(parent_container);
-  auto* capture_mode_bar_layer = capture_mode_bar_widget_.GetLayer();
+  auto* capture_mode_bar_layer = capture_mode_bar_widget_->GetLayer();
   auto* overlay_layer = layer();
   auto* parent_container_layer = parent_container->layer();
 
@@ -555,7 +554,7 @@ void CaptureModeSession::OnLocatedEvent(ui::LocatedEvent* event,
   wm::ConvertPointToScreen(event_target, &screen_location);
 
   const bool is_event_on_capture_bar =
-      capture_mode_bar_widget_.GetWindowBoundsInScreen().Contains(
+      capture_mode_bar_widget_->GetWindowBoundsInScreen().Contains(
           screen_location);
 
   if (capture_source == CaptureModeSource::kWindow) {
@@ -868,9 +867,9 @@ void CaptureModeSession::UpdateDimensionsLabelWidget(bool is_resizing) {
     // When moving to a new display, the dimensions label gets created/moved
     // onto the new display on press, while the capture bar gets moved on
     // release. In this case, we do not have to stack the dimensions label.
-    if (parent == capture_mode_bar_widget_.GetNativeWindow()->parent()) {
+    if (parent == capture_mode_bar_widget_->GetNativeWindow()->parent()) {
       parent->StackChildBelow(dimensions_label_widget_->GetNativeWindow(),
-                              capture_mode_bar_widget_.GetNativeWindow());
+                              capture_mode_bar_widget_->GetNativeWindow());
     }
   }
 
@@ -1132,7 +1131,7 @@ void CaptureModeSession::MaybeChangeRoot(aura::Window* new_root) {
   // capture, the capture bar will move at a later time, when the mouse is
   // released.
   if (controller_->source() != CaptureModeSource::kRegion) {
-    capture_mode_bar_widget_.SetBounds(
+    capture_mode_bar_widget_->SetBounds(
         CaptureModeBarView::GetBounds(current_root_));
   }
 

ash/capture_mode/capture_mode_session.h

@@ -19,6 +19,7 @@
 #include "ui/events/event.h"
 #include "ui/events/event_handler.h"
 #include "ui/views/controls/button/button.h"
+#include "ui/views/widget/unique_widget_ptr.h"
 #include "ui/views/widget/widget.h"
 
 namespace gfx {
@@ -192,13 +193,14 @@ class ASH_EXPORT CaptureModeSession : public ui::LayerOwner,
   // change if the user warps the cursor to another display in some situations.
   aura::Window* current_root_;
 
-  views::Widget capture_mode_bar_widget_;
+  views::UniqueWidgetPtr capture_mode_bar_widget_ =
+      std::make_unique<views::Widget>();
 
   // The content view of the above widget and owned by its views hierarchy.
   CaptureModeBarView* capture_mode_bar_view_ = nullptr;
 
   // Widget which displays capture region size during a region capture session.
-  std::unique_ptr<views::Widget> dimensions_label_widget_;
+  views::UniqueWidgetPtr dimensions_label_widget_;
 
   // Widget that shows an optional icon and a message in the middle of the
   // screen or in the middle of the capture region and prompts the user what to
@@ -206,7 +208,7 @@ class ASH_EXPORT CaptureModeSession : public ui::LayerOwner,
   // and source and can be empty in some cases. And in video capture mode, when
   // starting capturing, the widget will transform into a 3-second countdown
   // timer.
-  std::unique_ptr<views::Widget> capture_label_widget_;
+  views::UniqueWidgetPtr capture_label_widget_;
 
   // Magnifier glass used during a region capture session.
   MagnifierGlass magnifier_glass_;

ash/capture_mode/capture_mode_unittests.cc

@@ -25,6 +25,7 @@
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "ash/wm/window_state.h"
 #include "base/run_loop.h"
+#include "base/scoped_observation.h"
 #include "base/test/bind.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
@@ -35,6 +36,7 @@
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/vector2d.h"
 #include "ui/views/view.h"
+#include "ui/views/widget/widget_observer.h"
 
 namespace ash {
 
@@ -90,6 +92,10 @@ class CaptureModeSessionTestApi {
     return session_->capture_mode_bar_view_;
   }
 
+  views::Widget* capture_mode_bar_widget() const {
+    return session_->capture_mode_bar_widget_.get();
+  }
+
   views::Widget* capture_label_widget() const {
     return session_->capture_label_widget_.get();
   }
@@ -125,6 +131,12 @@ class CaptureModeTest : public AshTestBase {
     return CaptureModeSessionTestApi(session).capture_mode_bar_view();
   }
 
+  views::Widget* GetCaptureModeBarWidget() const {
+    auto* session = CaptureModeController::Get()->capture_mode_session();
+    DCHECK(session);
+    return CaptureModeSessionTestApi(session).capture_mode_bar_widget();
+  }
+
   CaptureModeToggleButton* GetImageToggleButton() const {
     auto* controller = CaptureModeController::Get();
     DCHECK(controller->IsActive());
@@ -255,6 +267,29 @@ class CaptureModeTest : public AshTestBase {
   base::test::ScopedFeatureList scoped_feature_list_;
 };
 
+class CaptureSessionWidgetObserver : public views::WidgetObserver {
+ public:
+  explicit CaptureSessionWidgetObserver(views::Widget* widget) {
+    DCHECK(widget);
+    observer_.Observe(widget);
+  }
+  CaptureSessionWidgetObserver(const CaptureSessionWidgetObserver&) = delete;
+  CaptureSessionWidgetObserver& operator=(const CaptureSessionWidgetObserver&) =
+      delete;
+  ~CaptureSessionWidgetObserver() override = default;
+
+  bool GetWidgetDestroyed() const { return !observer_.IsObserving(); }
+
+  // views::WidgetObserver
+  void OnWidgetClosing(views::Widget* widget) override {
+    DCHECK(observer_.IsObservingSource(widget));
+    observer_.RemoveObservation();
+  }
+
+ private:
+  base::ScopedObservation<views::Widget, views::WidgetObserver> observer_{this};
+};
+
 TEST_F(CaptureModeTest, StartStop) {
   auto* controller = CaptureModeController::Get();
   controller->Start(CaptureModeEntryType::kQuickSettings);
@@ -266,6 +301,20 @@ TEST_F(CaptureModeTest, StartStop) {
   EXPECT_FALSE(controller->IsActive());
 }
 
+TEST_F(CaptureModeTest, CheckWidgetClosed) {
+  auto* controller = CaptureModeController::Get();
+  controller->Start(CaptureModeEntryType::kQuickSettings);
+  EXPECT_TRUE(controller->IsActive());
+  EXPECT_TRUE(GetCaptureModeBarWidget());
+  CaptureSessionWidgetObserver observer(GetCaptureModeBarWidget());
+  EXPECT_FALSE(observer.GetWidgetDestroyed());
+  controller->Stop();
+  EXPECT_FALSE(controller->IsActive());
+  EXPECT_FALSE(controller->capture_mode_session());
+  // The Widget should have been destroyed by now.
+  EXPECT_TRUE(observer.GetWidgetDestroyed());
+}
+
 TEST_F(CaptureModeTest, StartWithMostRecentTypeAndSource) {
   auto* controller = CaptureModeController::Get();
   controller->SetSource(CaptureModeSource::kFullscreen);