Updated ownership semantics in TouchSelectionControllerImpl

Removed the need for set_owned_by_client() in EditingHandleView.

Fixed ownership semantics for the Widget holding EditingHandleViews
to eliminate the use of WIDGET_OWNS_NATIVE_WIDGET. Currently this
ownership model is aimed primarily at enabling easier testing and is
in the process of being removed.

Bug: 1044687
Change-Id: I51861ca2d9f2a505d86f92fe208868a562f585b0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2218502
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Reviewed-by: Mohsen Izadi <mohsen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774690}

ui/views/touchui/touch_selection_controller_impl.cc

@@ -71,21 +71,6 @@ constexpr int kSelectionHandleBarMinHeight = 5;
 // boundaries.
 constexpr int kSelectionHandleBarBottomAllowance = 3;
 
-// Creates a widget to host SelectionHandleView.
-views::Widget* CreateTouchSelectionPopupWidget(
-    gfx::NativeView parent,
-    views::WidgetDelegate* widget_delegate) {
-  views::Widget* widget = new views::Widget;
-  views::Widget::InitParams params(views::Widget::InitParams::TYPE_POPUP);
-  params.opacity = views::Widget::InitParams::WindowOpacity::kTranslucent;
-  params.shadow_type = views::Widget::InitParams::ShadowType::kNone;
-  params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
-  params.parent = parent;
-  params.delegate = widget_delegate;
-  widget->Init(std::move(params));
-  return widget;
-}
-
 gfx::Image* GetCenterHandleImage() {
   static gfx::Image* handle_image = nullptr;
   if (!handle_image) {
@@ -209,8 +194,7 @@ namespace views {
 using EditingHandleView = TouchSelectionControllerImpl::EditingHandleView;
 
 // A View that displays the text selection handle.
-class TouchSelectionControllerImpl::EditingHandleView
-    : public WidgetDelegateView {
+class TouchSelectionControllerImpl::EditingHandleView : public View {
  public:
   EditingHandleView(TouchSelectionControllerImpl* controller,
                     gfx::NativeView parent,
@@ -218,26 +202,31 @@ class TouchSelectionControllerImpl::EditingHandleView
       : controller_(controller),
         image_(GetCenterHandleImage()),
         is_cursor_handle_(is_cursor_handle),
-        draw_invisible_(false) {
-    widget_.reset(CreateTouchSelectionPopupWidget(parent, this));
-
-    targeter_ = new aura::WindowTargeter();
-    aura::Window* window = widget_->GetNativeWindow();
-    window->SetEventTargeter(std::unique_ptr<aura::WindowTargeter>(targeter_));
+        draw_invisible_(false),
+        widget_(new views::Widget) {
+    // Create a widget to host EditingHandleView.
+    views::Widget::InitParams params(views::Widget::InitParams::TYPE_POPUP);
+    params.opacity = views::Widget::InitParams::WindowOpacity::kTranslucent;
+    params.shadow_type = views::Widget::InitParams::ShadowType::kNone;
+    params.parent = parent;
+    widget_->Init(std::move(params));
 
-    // We are owned by the TouchSelectionControllerImpl.
-    set_owned_by_client();
+    widget_->GetNativeWindow()->SetEventTargeter(
+        std::make_unique<aura::WindowTargeter>());
+    widget_->SetContentsView(this);
   }
 
-  ~EditingHandleView() override { SetWidgetVisible(false); }
+  EditingHandleView(const EditingHandleView&) = delete;
+  EditingHandleView& operator=(const EditingHandleView&) = delete;
+  ~EditingHandleView() override = default;
 
-  gfx::SelectionBound::Type selection_bound_type() {
-    return selection_bound_.type();
+  void CloseHandleWidget() {
+    SetWidgetVisible(false);
+    widget_->CloseNow();
   }
 
-  // WidgetDelegateView:
-  void DeleteDelegate() override {
-    // We are owned and deleted by TouchSelectionControllerImpl.
+  gfx::SelectionBound::Type selection_bound_type() {
+    return selection_bound_.type();
   }
 
   // View:
@@ -340,7 +329,10 @@ class TouchSelectionControllerImpl::EditingHandleView
     const gfx::Insets insets(
         selection_bound_.GetHeight() + kSelectionHandleVerticalVisualOffset, 0,
         0, 0);
-    targeter_->SetInsets(insets, insets);
+
+    // Shifts the hit-test target below the apparent bounds to make dragging
+    // easier.
+    widget_->GetNativeWindow()->targeter()->SetInsets(insets, insets);
   }
 
   void SetDrawInvisible(bool draw_invisible) {
@@ -351,15 +343,8 @@ class TouchSelectionControllerImpl::EditingHandleView
   }
 
  private:
-  std::unique_ptr<Widget> widget_;
   TouchSelectionControllerImpl* controller_;
 
-  // A WindowTargeter that shifts the hit-test target below the apparent bounds
-  // to make dragging easier. The |widget_|'s NativeWindow takes ownership over
-  // the |targeter_| but since the |widget_|'s lifetime is known to this class,
-  // it can safely access the |targeter_|.
-  aura::WindowTargeter* targeter_;
-
   // In local coordinates
   gfx::SelectionBound selection_bound_;
   gfx::Image* image_;
@@ -379,7 +364,8 @@ class TouchSelectionControllerImpl::EditingHandleView
   // handle.
   bool draw_invisible_;
 
-  DISALLOW_COPY_AND_ASSIGN(EditingHandleView);
+  // Owning widget.
+  Widget* widget_ = nullptr;
 };
 
 TouchSelectionControllerImpl::TouchSelectionControllerImpl(
@@ -412,6 +398,10 @@ TouchSelectionControllerImpl::~TouchSelectionControllerImpl() {
   aura::Env::GetInstance()->RemoveEventObserver(this);
   if (client_widget_)
     client_widget_->RemoveObserver(this);
+  // Close the owning Widgets to clean up the EditingHandleViews.
+  selection_handle_1_->CloseHandleWidget();
+  selection_handle_2_->CloseHandleWidget();
+  cursor_handle_->CloseHandleWidget();
 }
 
 void TouchSelectionControllerImpl::SelectionChanged() {
@@ -466,11 +456,11 @@ void TouchSelectionControllerImpl::SelectionChanged() {
     // TODO(varunjain): Fix this: crbug.com/269003
     dragging_handle_->SetDrawInvisible(!ShouldShowHandleFor(focus));
 
-    if (dragging_handle_ != cursor_handle_.get()) {
+    if (dragging_handle_ != cursor_handle_) {
       // The non-dragging-handle might have recently become visible.
-      EditingHandleView* non_dragging_handle = selection_handle_1_.get();
-      if (dragging_handle_ == selection_handle_1_.get()) {
-        non_dragging_handle = selection_handle_2_.get();
+      EditingHandleView* non_dragging_handle = selection_handle_1_;
+      if (dragging_handle_ == selection_handle_1_) {
+        non_dragging_handle = selection_handle_2_;
         // if handle 1 is being dragged, it is corresponding to the end of
         // selection and the other handle to the start of selection.
         selection_bound_1_ = screen_bound_focus;
@@ -488,15 +478,13 @@ void TouchSelectionControllerImpl::SelectionChanged() {
         screen_bound_anchor.edge_end() == screen_bound_focus.edge_end()) {
       selection_handle_1_->SetWidgetVisible(false);
       selection_handle_2_->SetWidgetVisible(false);
-      SetHandleBound(cursor_handle_.get(), anchor, screen_bound_anchor_clipped);
+      SetHandleBound(cursor_handle_, anchor, screen_bound_anchor_clipped);
       return;
     }
 
     cursor_handle_->SetWidgetVisible(false);
-    SetHandleBound(selection_handle_1_.get(), anchor,
-                   screen_bound_anchor_clipped);
-    SetHandleBound(selection_handle_2_.get(), focus,
-                   screen_bound_focus_clipped);
+    SetHandleBound(selection_handle_1_, anchor, screen_bound_anchor_clipped);
+    SetHandleBound(selection_handle_2_, focus, screen_bound_focus_clipped);
   }
 }
 
@@ -522,14 +510,14 @@ void TouchSelectionControllerImpl::SelectionHandleDragged(
   gfx::Point drag_pos_in_client = drag_pos;
   ConvertPointToClientView(dragging_handle_, &drag_pos_in_client);
 
-  if (dragging_handle_ == cursor_handle_.get()) {
+  if (dragging_handle_ == cursor_handle_) {
     client_view_->MoveCaretTo(drag_pos_in_client);
     return;
   }
 
   // Find the stationary selection handle.
-  gfx::SelectionBound anchor_bound =
-      selection_handle_1_.get() == dragging_handle_ ? selection_bound_2_
+  gfx::SelectionBound anchor_bound = selection_handle_1_ == dragging_handle_
+                                         ? selection_bound_2_
                                          : selection_bound_1_;
 
   // Find selection end points in client_view's coordinate system.
@@ -729,12 +717,12 @@ gfx::Rect TouchSelectionControllerImpl::GetExpectedHandleBounds(
   return GetSelectionWidgetBounds(bound);
 }
 
-WidgetDelegateView* TouchSelectionControllerImpl::GetHandle1View() {
-  return selection_handle_1_.get();
+View* TouchSelectionControllerImpl::GetHandle1View() {
+  return selection_handle_1_;
 }
 
-WidgetDelegateView* TouchSelectionControllerImpl::GetHandle2View() {
-  return selection_handle_2_.get();
+View* TouchSelectionControllerImpl::GetHandle2View() {
+  return selection_handle_2_;
 }
 
 }  // namespace views

ui/views/touchui/touch_selection_controller_impl.h

@@ -19,7 +19,6 @@
 #include "ui/views/widget/widget_observer.h"
 
 namespace views {
-class WidgetDelegateView;
 
 // Touch specific implementation of TouchEditingControllerDeprecated.
 // Responsible for displaying selection handles and menu elements relevant in a
@@ -34,6 +33,9 @@ class VIEWS_EXPORT TouchSelectionControllerImpl
 
   // Use ui::TouchEditingControllerFactory::Create() instead.
   explicit TouchSelectionControllerImpl(ui::TouchEditable* client_view);
+  TouchSelectionControllerImpl(const TouchSelectionControllerImpl&) = delete;
+  TouchSelectionControllerImpl& operator=(const TouchSelectionControllerImpl&) =
+      delete;
   ~TouchSelectionControllerImpl() override;
 
   // ui::TouchEditingControllerDeprecated:
@@ -106,14 +108,16 @@ class VIEWS_EXPORT TouchSelectionControllerImpl
   bool IsSelectionHandle2Visible();
   bool IsCursorHandleVisible();
   gfx::Rect GetExpectedHandleBounds(const gfx::SelectionBound& bound);
-  WidgetDelegateView* GetHandle1View();
-  WidgetDelegateView* GetHandle2View();
+  View* GetHandle1View();
+  View* GetHandle2View();
 
   ui::TouchEditable* client_view_;
   Widget* client_widget_ = nullptr;
-  std::unique_ptr<EditingHandleView> selection_handle_1_;
-  std::unique_ptr<EditingHandleView> selection_handle_2_;
-  std::unique_ptr<EditingHandleView> cursor_handle_;
+  // Non-owning pointers to EditingHandleViews. These views are owned by their
+  // Widget and cleaned up when their Widget closes.
+  EditingHandleView* selection_handle_1_;
+  EditingHandleView* selection_handle_2_;
+  EditingHandleView* cursor_handle_;
   bool command_executed_ = false;
   base::TimeTicks selection_start_time_;
 
@@ -136,8 +140,6 @@ class VIEWS_EXPORT TouchSelectionControllerImpl
   // Selection bounds, clipped to client view's boundaries.
   gfx::SelectionBound selection_bound_1_clipped_;
   gfx::SelectionBound selection_bound_2_clipped_;
-
-  DISALLOW_COPY_AND_ASSIGN(TouchSelectionControllerImpl);
 };
 
 }  // namespace views

ui/views/touchui/touch_selection_controller_impl_unittest.cc

@@ -147,7 +147,7 @@ class TouchSelectionControllerImplTest : public ViewsTestBase {
 
   void SimulateSelectionHandleDrag(gfx::Vector2d v, int selection_handle) {
     TouchSelectionControllerImpl* controller = GetSelectionController();
-    views::WidgetDelegateView* handle = nullptr;
+    views::View* handle = nullptr;
     if (selection_handle == 1)
       handle = controller->GetHandle1View();
     else