Remove set_owned_by_client() from AutoclickScrollPositionHandler.

It's safe for the Widget to own the underlying View via
SetContentsView().  This was not immediately obvious; much of this CL is
adjacent refactoring and cleanup that simplifies the ownership/lifetime.

Bug: 1044687
Change-Id: I76cbc402edb7195c42cbae679a89839384d0615a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2210914
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Katie Dektar <katie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#771122}

ash/autoclick/autoclick_controller.cc

@@ -52,8 +52,7 @@ base::TimeDelta CalculateStartGestureDelay(base::TimeDelta total_delay) {
 }
 
 views::Widget::InitParams CreateAutoclickOverlayWidgetParams(
-    aura::Window* target) {
-  aura::Window* root_window = target->GetRootWindow();
+    aura::Window* root_window) {
   views::Widget::InitParams params;
   params.type = views::Widget::InitParams::TYPE_WINDOW_FRAMELESS;
   params.accept_events = false;
@@ -128,7 +127,7 @@ void AutoclickController::SetEnabled(bool enabled,
     menu_bubble_controller_->ShowBubble(event_type_, menu_position_);
     if (event_type_ == AutoclickEventType::kScroll) {
       InitializeScrollLocation();
-      UpdateScrollPosition(scroll_location_);
+      UpdateScrollPosition();
     }
     enabled_ = enabled;
   } else {
@@ -190,7 +189,7 @@ void AutoclickController::SetAutoclickEventType(AutoclickEventType type) {
 
   if (type == AutoclickEventType::kScroll) {
     InitializeScrollLocation();
-    UpdateScrollPosition(scroll_location_);
+    UpdateScrollPosition();
   } else {
     over_scroll_button_ = false;
     HideScrollPosition();
@@ -290,32 +289,12 @@ void AutoclickController::UpdateAutoclickMenuBoundsIfNeeded() {
     menu_bubble_controller_->SetPosition(menu_position_);
 }
 
-void AutoclickController::CreateAutoclickRingWidget(
-    const gfx::Point& point_in_screen) {
-  aura::Window* target = window_util::GetRootWindowAt(point_in_screen);
-  SetTapDownTarget(target);
-  ring_widget_.reset(new views::Widget);
-  ring_widget_->Init(CreateAutoclickOverlayWidgetParams(target));
-  ring_widget_->SetOpacity(1.f);
-}
-
-void AutoclickController::CreateAutoclickScrollPositionWidget(
-    const gfx::Point& point_in_screen) {
-  aura::Window* target = window_util::GetRootWindowAt(point_in_screen);
-  SetTapDownTarget(target);
-  scroll_position_widget_.reset(new views::Widget);
-  scroll_position_widget_->Init(CreateAutoclickOverlayWidgetParams(target));
-}
-
 void AutoclickController::UpdateAutoclickWidgetPosition(
-    views::Widget* widget,
-    const gfx::Point& point_in_screen) {
-  aura::Window* target = window_util::GetRootWindowAt(point_in_screen);
-  SetTapDownTarget(target);
-  aura::Window* root_window = target->GetRootWindow();
-  if (widget->GetNativeView()->GetRootWindow() != root_window) {
+    gfx::NativeView native_view,
+    aura::Window* root_window) {
+  if (native_view->GetRootWindow() != root_window) {
     views::Widget::ReparentNativeView(
-        widget->GetNativeView(),
+        native_view,
         Shell::GetContainer(root_window, kShellWindowId_OverlayContainer));
   }
 }
@@ -356,7 +335,7 @@ void AutoclickController::DoAutoclickAction() {
     } else {
       scroll_location_ = gesture_anchor_location_;
       is_initial_scroll_location_ = false;
-      UpdateScrollPosition(scroll_location_);
+      UpdateScrollPosition();
       Shell::Get()
           ->accessibility_controller()
           ->RequestAutoclickScrollableBoundsForPoint(scroll_location_);
@@ -500,11 +479,17 @@ void AutoclickController::InitClickTimers() {
                           base::Unretained(this)));
 }
 
-void AutoclickController::UpdateRingWidget(const gfx::Point& point_in_screen) {
-  if (!ring_widget_) {
-    CreateAutoclickRingWidget(point_in_screen);
+void AutoclickController::UpdateRingWidget() {
+  aura::Window* const target =
+      window_util::GetRootWindowAt(last_mouse_location_);
+  SetTapDownTarget(target);
+  aura::Window* const root_window = target->GetRootWindow();
+  if (ring_widget_) {
+    UpdateAutoclickWidgetPosition(ring_widget_->GetNativeView(), root_window);
   } else {
-    UpdateAutoclickWidgetPosition(ring_widget_.get(), point_in_screen);
+    ring_widget_ = std::make_unique<views::Widget>(
+        CreateAutoclickOverlayWidgetParams(root_window));
+    ring_widget_->SetOpacity(1.0f);
   }
 }
 
@@ -522,29 +507,28 @@ void AutoclickController::InitializeScrollLocation() {
       ->RequestAutoclickScrollableBoundsForPoint(scroll_location_);
 }
 
-void AutoclickController::UpdateScrollPosition(
-    const gfx::Point& point_in_screen) {
+void AutoclickController::UpdateScrollPosition() {
   if (!enabled_)
     return;
-  if (!scroll_position_widget_) {
-    CreateAutoclickScrollPositionWidget(point_in_screen);
+
+  aura::Window* const target = window_util::GetRootWindowAt(scroll_location_);
+  SetTapDownTarget(target);
+  aura::Window* const root_window = target->GetRootWindow();
+  if (autoclick_scroll_position_handler_) {
+    UpdateAutoclickWidgetPosition(
+        autoclick_scroll_position_handler_->GetNativeView(), root_window);
+  } else {
     autoclick_scroll_position_handler_ =
         std::make_unique<AutoclickScrollPositionHandler>(
-            scroll_location_, scroll_position_widget_.get());
-  } else {
-    UpdateAutoclickWidgetPosition(scroll_position_widget_.get(),
-                                  point_in_screen);
-    autoclick_scroll_position_handler_->SetCenter(
-        scroll_location_, scroll_position_widget_.get());
+            std::make_unique<views::Widget>(
+                CreateAutoclickOverlayWidgetParams(root_window)));
   }
+  autoclick_scroll_position_handler_->SetScrollPointCenterInScreen(
+      scroll_location_);
 }
 
 void AutoclickController::HideScrollPosition() {
-  // Hide the scroll position UI if it exists.
-  if (autoclick_scroll_position_handler_)
-    autoclick_scroll_position_handler_.reset();
-  if (scroll_position_widget_)
-    scroll_position_widget_.reset();
+  autoclick_scroll_position_handler_.reset();
 
   // TODO(katie): Clear any Autoclick scroll focus rings here.
 }
@@ -611,7 +595,7 @@ void AutoclickController::OnMouseEvent(ui::MouseEvent* event) {
         drag_event_rewriter_->IsEnabled()))) {
     mouse_event_flags_ = event->flags();
     // Update the point even if the animation is not currently being shown.
-    UpdateRingWidget(last_mouse_location_);
+    UpdateRingWidget();
 
     // The distance between the mouse location and the anchor location
     // must exceed a certain threshold to initiate a new autoclick countdown.

ash/autoclick/autoclick_controller.h

@@ -13,6 +13,7 @@
 #include "ui/aura/window_observer.h"
 #include "ui/events/event_handler.h"
 #include "ui/gfx/geometry/point.h"
+#include "ui/gfx/native_widget_types.h"
 
 namespace base {
 class RetainingOneShotTimer;
@@ -119,19 +120,17 @@ class ASH_EXPORT AutoclickController
 
  private:
   void SetTapDownTarget(aura::Window* target);
-  void CreateAutoclickRingWidget(const gfx::Point& point_in_screen);
-  void CreateAutoclickScrollPositionWidget(const gfx::Point& point_in_screen);
-  void UpdateAutoclickWidgetPosition(views::Widget* widget,
-                                     const gfx::Point& point_in_screen);
+  void UpdateAutoclickWidgetPosition(gfx::NativeView native_view,
+                                     aura::Window* root_window);
   void DoAutoclickAction();
   void StartAutoclickGesture();
   void CancelAutoclickAction();
   void OnActionCompleted(AutoclickEventType event_type);
   void InitClickTimers();
-  void UpdateRingWidget(const gfx::Point& mouse_location);
+  void UpdateRingWidget();
   void UpdateRingSize();
   void InitializeScrollLocation();
-  void UpdateScrollPosition(const gfx::Point& point_in_screen);
+  void UpdateScrollPosition();
   void HideScrollPosition();
   void RecordUserAction(AutoclickEventType event_type) const;
   bool DragInProgress() const;
@@ -200,8 +199,6 @@ class ASH_EXPORT AutoclickController
 
   // The widget containing the autoclick ring.
   std::unique_ptr<views::Widget> ring_widget_;
-  // The widget containing the autoclick scroll position indiciator.
-  std::unique_ptr<views::Widget> scroll_position_widget_;
   base::TimeDelta delay_;
   // The timer that counts down from the beginning of a gesture until a click.
   std::unique_ptr<base::RetainingOneShotTimer> autoclick_timer_;

ash/autoclick/autoclick_scroll_position_handler.cc

@@ -6,126 +6,82 @@
 
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "third_party/skia/include/core/SkColor.h"
-#include "third_party/skia/include/core/SkPath.h"
-#include "third_party/skia/include/core/SkRect.h"
-#include "ui/aura/window.h"
-#include "ui/compositor/layer.h"
-#include "ui/gfx/animation/linear_animation.h"
+#include "ui/gfx/animation/tween.h"
 #include "ui/gfx/canvas.h"
+#include "ui/gfx/color_palette.h"
+#include "ui/gfx/geometry/point.h"
+#include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/size.h"
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/paint_vector_icon.h"
-#include "ui/gfx/transform.h"
+#include "ui/views/controls/image_view.h"
+#include "ui/views/layout/fill_layout.h"
 #include "ui/views/view.h"
+#include "ui/views/widget/widget.h"
 
 namespace ash {
 
-namespace {
-constexpr int kScrollBackgroundSizeInDips = 32;
-constexpr int kScrollIconSizeInDips = 24;
-constexpr SkColor kIconBackgroundColor = SkColorSetARGB(255, 128, 134, 139);
-constexpr base::TimeDelta kAnimationTime =
-    base::TimeDelta::FromMilliseconds(500);
-constexpr float kFadedOpacity = 0.5;
-}  // namespace
-
-// View of the AutoclickScrollPositionHandler. Draws the actual contents and
-// maintains the views::Widget that the animation is shown in.
 class AutoclickScrollPositionView : public views::View {
  public:
-  AutoclickScrollPositionView(const gfx::Point& event_location,
-                              views::Widget* widget)
-      : views::View(), widget_(widget) {
-    image_ = gfx::CreateVectorIcon(kAutoclickScrollIcon, kScrollIconSizeInDips,
-                                   SK_ColorWHITE);
-    widget_->SetContentsView(this);
-    SetSize(
-        gfx::Size(kScrollBackgroundSizeInDips, kScrollBackgroundSizeInDips));
-
-    // Owned by the AutoclickScrollPositionHandler.
-    set_owned_by_client();
-
-    SetLocation(event_location);
-  }
-
+  AutoclickScrollPositionView();
+  AutoclickScrollPositionView(const AutoclickScrollPositionView&) = delete;
+  AutoclickScrollPositionView& operator=(const AutoclickScrollPositionView&) =
+      delete;
   ~AutoclickScrollPositionView() override = default;
 
-  void SetLocation(const gfx::Point& new_event_location) {
-    gfx::Point point = new_event_location;
-    widget_->SetBounds(gfx::Rect(point.x() - kScrollBackgroundSizeInDips / 2,
-                                 point.y() - kScrollBackgroundSizeInDips / 2,
-                                 kScrollBackgroundSizeInDips,
-                                 kScrollBackgroundSizeInDips));
-    widget_->Show();
-    widget_->SetOpacity(1.0);
-    SchedulePaint();
-  }
-
-  void UpdateForAnimationStep(gfx::Animation* animation) {
-    widget_->SetOpacity(animation->CurrentValueBetween(1.0, kFadedOpacity));
-  }
-
- private:
-  void OnPaint(gfx::Canvas* canvas) override {
-    gfx::Point center(kScrollBackgroundSizeInDips / 2,
-                      kScrollBackgroundSizeInDips / 2);
-    canvas->Save();
-
-    cc::PaintFlags flags;
-    flags.setAntiAlias(true);
-
-    // Draw the grey background.
-    flags.setStyle(cc::PaintFlags::kFill_Style);
-    flags.setColor(kIconBackgroundColor);
-    canvas->DrawCircle(center, kScrollBackgroundSizeInDips / 2, flags);
-
-    // Draw the icon on top.
-    canvas->DrawImageInt(image_, center.x() - kScrollIconSizeInDips / 2,
-                         center.y() - kScrollIconSizeInDips / 2);
+ protected:
+  // views::View:
+  void OnPaintBackground(gfx::Canvas* canvas) override;
+};
 
-    canvas->Restore();
-  }
+AutoclickScrollPositionView::AutoclickScrollPositionView() {
+  SetLayoutManager(std::make_unique<views::FillLayout>());
+  AddChildView(std::make_unique<views::ImageView>())
+      ->SetImage(
+          gfx::CreateVectorIcon(kAutoclickScrollIcon, 24, SK_ColorWHITE));
+}
 
-  views::Widget* widget_;
-  gfx::ImageSkia image_;
+void AutoclickScrollPositionView::OnPaintBackground(gfx::Canvas* canvas) {
+  cc::PaintFlags flags;
+  flags.setAntiAlias(true);
+  flags.setColor(gfx::kGoogleGrey600);
+  canvas->DrawCircle(GetLocalBounds().CenterPoint(), width() / 2, flags);
+}
 
-  DISALLOW_COPY_AND_ASSIGN(AutoclickScrollPositionView);
-};
+// static
+constexpr base::TimeDelta AutoclickScrollPositionHandler::kOpaqueTime;
+constexpr base::TimeDelta AutoclickScrollPositionHandler::kFadeTime;
 
 AutoclickScrollPositionHandler::AutoclickScrollPositionHandler(
-    const gfx::Point& center_point_in_screen,
-    views::Widget* widget)
-    : gfx::LinearAnimation(nullptr) {
-  view_ = std::make_unique<AutoclickScrollPositionView>(center_point_in_screen,
-                                                        widget);
-  SetDuration(kAnimationTime);
-  animation_state_ = AnimationState::kWait;
-  Start();
+    std::unique_ptr<views::Widget> widget)
+    : widget_(std::move(widget)) {
+  widget_->SetContentsView(std::make_unique<AutoclickScrollPositionView>());
 }
 
-AutoclickScrollPositionHandler::~AutoclickScrollPositionHandler() {
-  view_.reset();
-}
+AutoclickScrollPositionHandler::~AutoclickScrollPositionHandler() = default;
 
-void AutoclickScrollPositionHandler::SetCenter(
-    const gfx::Point& center_point_in_screen,
-    views::Widget* widget) {
-  view_->SetLocation(center_point_in_screen);
-  animation_state_ = AnimationState::kWait;
-  Start();
+gfx::NativeView AutoclickScrollPositionHandler::GetNativeView() {
+  return widget_->GetNativeView();
 }
 
-void AutoclickScrollPositionHandler::AnimateToState(double state) {
-  if (animation_state_ == AnimationState::kFade)
-    view_->UpdateForAnimationStep(this);
+void AutoclickScrollPositionHandler::SetScrollPointCenterInScreen(
+    const gfx::Point& scroll_point_center) {
+  constexpr int kBackgroundSizeDips = 32;
+  gfx::Rect bounds(gfx::Size(kBackgroundSizeDips, kBackgroundSizeDips));
+  bounds.set_origin(scroll_point_center -
+                    bounds.CenterPoint().OffsetFromOrigin());
+  widget_->SetBounds(bounds);
+  widget_->Show();
+  widget_->SetOpacity(1.0f);
+
+  timer_.Reset();
 }
 
-void AutoclickScrollPositionHandler::AnimationStopped() {
-  if (animation_state_ == AnimationState::kWait) {
-    animation_state_ = AnimationState::kFade;
-    Start();
-  } else if (animation_state_ == AnimationState::kFade) {
-    animation_state_ = AnimationState::kDone;
-  }
+void AutoclickScrollPositionHandler::AnimationProgressed(
+    const gfx::Animation* animation) {
+  constexpr float kSteadyStateOpacity = 0.5f;
+  widget_->SetOpacity(gfx::Tween::FloatValueBetween(
+      animation_.GetCurrentValue(), 1.0f, kSteadyStateOpacity));
 }
 
 }  // namespace ash

ash/autoclick/autoclick_scroll_position_handler.h

@@ -7,43 +7,54 @@
 
 #include <memory>
 
-#include "base/macros.h"
+#include "base/location.h"
+#include "base/time/time.h"
+#include "base/timer/timer.h"
+#include "ui/gfx/animation/animation_delegate.h"
 #include "ui/gfx/animation/linear_animation.h"
-#include "ui/gfx/geometry/point.h"
-#include "ui/views/widget/widget.h"
+#include "ui/gfx/native_widget_types.h"
 
-namespace ash {
+namespace gfx {
+class Point;
+}  // namespace gfx
+
+namespace views {
+class Widget;
+}  // namespace views
 
-class AutoclickScrollPositionView;
+namespace ash {
 
 // AutoclickScrollPositionHandler displays the position at which the next scroll
 // event will occur, giving users a sense of which part of the screen will
 // receive scroll events. It will display at full opacity for a short time, then
 // partially fade out to keep from blocking content.
-class AutoclickScrollPositionHandler : public gfx::LinearAnimation {
+class AutoclickScrollPositionHandler : public gfx::AnimationDelegate {
  public:
-  AutoclickScrollPositionHandler(const gfx::Point& center_point_in_screen,
-                                 views::Widget* widget);
+  explicit AutoclickScrollPositionHandler(
+      std::unique_ptr<views::Widget> widget);
+  AutoclickScrollPositionHandler(const AutoclickScrollPositionHandler&) =
+      delete;
+  AutoclickScrollPositionHandler& operator=(
+      const AutoclickScrollPositionHandler&) = delete;
   ~AutoclickScrollPositionHandler() override;
 
-  void SetCenter(const gfx::Point& center_point_in_screen,
-                 views::Widget* widget);
+  gfx::NativeView GetNativeView();
 
- private:
-  enum AnimationState {
-    kWait,
-    kFade,
-    kDone,
-  };
+  void SetScrollPointCenterInScreen(const gfx::Point& scroll_point_center);
 
-  // Overridden from gfx::LinearAnimation.
-  void AnimateToState(double state) override;
-  void AnimationStopped() override;
-
-  std::unique_ptr<AutoclickScrollPositionView> view_;
-  AnimationState animation_state_ = kDone;
-
-  DISALLOW_COPY_AND_ASSIGN(AutoclickScrollPositionHandler);
+ private:
+  static constexpr auto kOpaqueTime = base::TimeDelta::FromMilliseconds(500);
+  static constexpr auto kFadeTime = base::TimeDelta::FromMilliseconds(500);
+
+  // gfx::AnimationDelegate:
+  void AnimationProgressed(const gfx::Animation* animation) override;
+
+  std::unique_ptr<views::Widget> widget_;
+  base::DelayTimer timer_{FROM_HERE, kOpaqueTime,
+                          static_cast<gfx::Animation*>(&animation_),
+                          &gfx::Animation::Start};
+  gfx::LinearAnimation animation_{
+      kFadeTime, gfx::LinearAnimation::kDefaultFrameRate, this};
 };
 
 }  // namespace ash