Obtain compositor from widget in CompositorAnimationRunner

* It observes widget's destruction instead of observing
 compositor's in CompositorChecker. This allow CAR to continue running
 even if the widget's compositor changes.
* Re-enable on chromeos

Bug: 1019536
Test: Covered by unittests.
Change-Id: Ia3a3201408b93d70b98339e79b7e9ffd6db22e56
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1890493Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712641}

ui/views/animation/animation_delegate_views.cc

@@ -55,12 +55,6 @@ void AnimationDelegateViews::AnimationContainerShuttingDown(
 }
 
 void AnimationDelegateViews::UpdateAnimationRunner() {
-#if defined(OS_CHROMEOS)
-  // TODO(crbug.com/969788): Re-enable this function with better ui::Compositor
-  // switching support.
-  return;
-#endif  // defined(OS_CHROMEOS)
-
   if (!container_)
     return;
 
@@ -74,8 +68,8 @@ void AnimationDelegateViews::UpdateAnimationRunner() {
   if (container_->has_custom_animation_runner())
     return;
 
-  container_->SetAnimationRunner(std::make_unique<CompositorAnimationRunner>(
-      view_->GetWidget()->GetCompositor()));
+  container_->SetAnimationRunner(
+      std::make_unique<CompositorAnimationRunner>(view_->GetWidget()));
 }
 
 }  // namespace views

ui/views/animation/compositor_animation_runner.cc

@@ -4,37 +4,22 @@
 
 #include "ui/views/animation/compositor_animation_runner.h"
 
-namespace views {
-
-////////////////////////////////////////////////////////////////////////////////
-// CompositorAnimationRunner::CompositorChecker
-//
-CompositorAnimationRunner::CompositorChecker::CompositorChecker(
-    CompositorAnimationRunner* runner)
-    : runner_(runner) {
-  scoped_observer_.Add(runner_->compositor_);
-}
-
-CompositorAnimationRunner::CompositorChecker::~CompositorChecker() = default;
+#include "ui/views/widget/widget.h"
 
-void CompositorAnimationRunner::CompositorChecker::OnCompositingShuttingDown(
-    ui::Compositor* compositor) {
-  runner_->OnCompositingShuttingDown(compositor);
-  DCHECK(!compositor->HasAnimationObserver(runner_));
-  scoped_observer_.Remove(compositor);
-}
+namespace views {
 
 ////////////////////////////////////////////////////////////////////////////////
 // CompositorAnimationRunner
 //
-CompositorAnimationRunner::CompositorAnimationRunner(ui::Compositor* compositor)
-    : compositor_(compositor) {
-  DCHECK(compositor_);
+CompositorAnimationRunner::CompositorAnimationRunner(Widget* widget)
+    : widget_(widget) {
+  widget_->AddObserver(this);
 }
 
 CompositorAnimationRunner::~CompositorAnimationRunner() {
   // Make sure we're not observing |compositor_|.
-  Stop();
+  if (widget_)
+    OnWidgetDestroying(widget_);
   DCHECK(!compositor_ || !compositor_->HasAnimationObserver(this));
 }
 
@@ -43,6 +28,7 @@ void CompositorAnimationRunner::Stop() {
     compositor_->RemoveAnimationObserver(this);
 
   min_interval_ = base::TimeDelta::Max();
+  compositor_ = nullptr;
 }
 
 void CompositorAnimationRunner::OnAnimationStep(base::TimeTicks timestamp) {
@@ -56,13 +42,30 @@ void CompositorAnimationRunner::OnAnimationStep(base::TimeTicks timestamp) {
 void CompositorAnimationRunner::OnCompositingShuttingDown(
     ui::Compositor* compositor) {
   Stop();
-  compositor_ = nullptr;
+}
+
+void CompositorAnimationRunner::OnWidgetDestroying(Widget* widget) {
+  Stop();
+  widget_->RemoveObserver(this);
+  widget_ = nullptr;
 }
 
 void CompositorAnimationRunner::OnStart(base::TimeDelta min_interval,
                                         base::TimeDelta elapsed) {
-  if (!compositor_)
+  if (!widget_)
+    return;
+
+  ui::Compositor* current_compositor = widget_->GetCompositor();
+  if (!current_compositor) {
+    Stop();
     return;
+  }
+
+  if (current_compositor != compositor_) {
+    if (compositor_ && compositor_->HasAnimationObserver(this))
+      compositor_->RemoveAnimationObserver(this);
+    compositor_ = current_compositor;
+  }
 
   last_tick_ = base::TimeTicks::Now() - elapsed;
   min_interval_ = min_interval;

ui/views/animation/compositor_animation_runner.h

@@ -11,14 +11,19 @@
 #include "ui/compositor/compositor_animation_observer.h"
 #include "ui/compositor/compositor_observer.h"
 #include "ui/gfx/animation/animation_container.h"
+#include "ui/views/widget/widget_observer.h"
 
 namespace views {
+class Widget;
 
 // An animation runner based on ui::Compositor.
 class CompositorAnimationRunner : public gfx::AnimationRunner,
-                                  public ui::CompositorAnimationObserver {
+                                  public ui::CompositorAnimationObserver,
+                                  public WidgetObserver {
  public:
-  explicit CompositorAnimationRunner(ui::Compositor* compositor);
+  explicit CompositorAnimationRunner(Widget* widget);
+  CompositorAnimationRunner(CompositorAnimationRunner&) = delete;
+  CompositorAnimationRunner& operator=(CompositorAnimationRunner&) = delete;
   ~CompositorAnimationRunner() override;
 
   // gfx::AnimationRunner:
@@ -28,37 +33,25 @@ class CompositorAnimationRunner : public gfx::AnimationRunner,
   void OnAnimationStep(base::TimeTicks timestamp) override;
   void OnCompositingShuttingDown(ui::Compositor* compositor) override;
 
+  // WidgetObserver:
+  void OnWidgetDestroying(Widget* widget) override;
+
  protected:
   // gfx::AnimationRunner:
   void OnStart(base::TimeDelta min_interval, base::TimeDelta elapsed) override;
 
  private:
-  // This observes Compositor's destruction and helps CompositorAnimationRunner
-  // to stop animation. we need this because CompositorAnimationRunner observes
-  // the Compositor only when it's animating.
-  class CompositorChecker : public ui::CompositorObserver {
-   public:
-    explicit CompositorChecker(CompositorAnimationRunner* runner);
-    CompositorChecker(const CompositorChecker& other) = delete;
-    CompositorChecker& operator=(const CompositorChecker& other) = delete;
-    ~CompositorChecker() override;
+  // When |widget_| is nullptr, it means the widget has been destroyed and
+  // |compositor_| must also be nullptr.
+  Widget* widget_;
 
-    // ui::CompositorObserver:
-    void OnCompositingShuttingDown(ui::Compositor* compositor) override;
-
-   private:
-    CompositorAnimationRunner* runner_;
-    ScopedObserver<ui::Compositor, ui::CompositorObserver> scoped_observer_{
-        this};
-  };
-
-  // When |compositor_| is nullptr, it means compositor has been shut down.
-  ui::Compositor* compositor_;
+  // When |compositor_| is nullptr, it means either the animation is not
+  // running, or the compositor or |widget_| associated with the compositor_ has
+  // been destroyed during animation.
+  ui::Compositor* compositor_ = nullptr;
 
   base::TimeDelta min_interval_ = base::TimeDelta::Max();
   base::TimeTicks last_tick_;
-
-  CompositorChecker checker_{this};
 };
 
 }  // namespace views

ui/views/animation/compositor_animation_runner_unittest.cc

@@ -12,22 +12,16 @@
 
 namespace views {
 namespace test {
+namespace {
+constexpr base::TimeDelta kDuration = base::TimeDelta::FromMilliseconds(100);
+}
 
 using CompositorAnimationRunnerTest = WidgetTest;
 
-// TODO(crbug.com/969788): Re-enable CompositorAnimationRunner with better
-// ui::Compositor switching support.
-#if defined(OS_CHROMEOS)
-#define MAYBE_BasicCoverageTest DISABLED_BasicCoverageTest
-#else
-#define MAYBE_BasicCoverageTest BasicCoverageTest
-#endif
-TEST_F(CompositorAnimationRunnerTest, MAYBE_BasicCoverageTest) {
+TEST_F(CompositorAnimationRunnerTest, BasicCoverageTest) {
   WidgetAutoclosePtr widget(CreateTopLevelPlatformWidget());
   widget->Show();
 
-  constexpr base::TimeDelta kDuration = base::TimeDelta::FromMilliseconds(100);
-
   AnimationDelegateViews delegate(widget->GetContentsView());
   gfx::LinearAnimation animation(
       kDuration, gfx::LinearAnimation::kDefaultFrameRate, &delegate);
@@ -50,5 +44,67 @@ TEST_F(CompositorAnimationRunnerTest, MAYBE_BasicCoverageTest) {
   run_loop.Run();
 }
 
+// No DesktopAura on ChromeOS.
+// Each widget on MACOSX has its own ui::Compositor.
+#if !defined(OS_CHROMEOS) && !(OS_MACOSX)
+using CompositorAnimationRunnerDesktopTest = DesktopWidgetTest;
+
+TEST_F(CompositorAnimationRunnerDesktopTest, SwitchCompositor) {
+  WidgetAutoclosePtr widget1(CreateTopLevelNativeWidget());
+  widget1->Show();
+
+  WidgetAutoclosePtr widget2(CreateTopLevelNativeWidget());
+  widget2->Show();
+
+  ASSERT_NE(widget1->GetCompositor(), widget2->GetCompositor());
+
+  Widget* child = CreateChildNativeWidgetWithParent(widget1.get());
+  child->Show();
+  AnimationDelegateViews delegate(child->GetContentsView());
+  gfx::LinearAnimation animation(
+      kDuration, gfx::LinearAnimation::kDefaultFrameRate, &delegate);
+
+  base::RepeatingTimer interval_timer;
+
+  animation.Start();
+  EXPECT_TRUE(animation.is_animating());
+  EXPECT_TRUE(delegate.container()->has_custom_animation_runner());
+  {
+    base::RunLoop run_loop;
+    interval_timer.Start(FROM_HERE, kDuration,
+                         base::BindLambdaForTesting([&]() {
+                           if (animation.is_animating())
+                             return;
+                           interval_timer.Stop();
+                           run_loop.Quit();
+                         }));
+    run_loop.Run();
+  }
+
+  EXPECT_FALSE(animation.is_animating());
+
+  Widget::ReparentNativeView(child->GetNativeView(), widget2->GetNativeView());
+  widget1.reset();
+
+  animation.Start();
+  EXPECT_TRUE(animation.is_animating());
+  EXPECT_TRUE(delegate.container()->has_custom_animation_runner());
+
+  {
+    base::RunLoop run_loop;
+    interval_timer.Start(FROM_HERE, kDuration,
+                         base::BindLambdaForTesting([&]() {
+                           if (animation.is_animating())
+                             return;
+
+                           interval_timer.Stop();
+                           run_loop.Quit();
+                         }));
+
+    run_loop.Run();
+  }
+}
+#endif
+
 }  // namespace test
 }  // namespace views

ui/views/animation/installable_ink_drop.cc

@@ -76,8 +76,7 @@ InstallableInkDrop::InstallableInkDrop(View* view)
     // Using CompositorAnimationRunner keeps our animation updates in sync with
     // compositor frames and avoids jank.
     animation_container_->SetAnimationRunner(
-        std::make_unique<CompositorAnimationRunner>(
-            view_->GetWidget()->GetCompositor()));
+        std::make_unique<CompositorAnimationRunner>(view_->GetWidget()));
   }
 }