wm: Improve cross fade animation.

*Destroy old layer when new layer animation is aborted.
*Only log histograms once after animation.

The old layer is owned by a observer which does not know when the new
layer is being aborted. Change this so that the observer observers the
new layer and destroys the old layer accordingly.

The pass animations reporter will be used for every LayerAnimationElement,
for cross fade, we animate opacity in addition to transform when the old
bounds is wider than the new bounds. This results in double counting,
which skews the smoothness numbers as animating two properties should be
slower.

Test: manual, ash_unittests WindowAnimationsTest.CrossFadeHistograms
Bug: 868170, 1087460
Change-Id: I3bc5b597e8109a3e87f432f6aa352ba94860f8ed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2218557Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773397}

ash/wm/window_animations.cc

@@ -19,7 +19,6 @@
 #include "ash/wm/workspace_controller.h"
 #include "base/check.h"
 #include "base/i18n/rtl.h"
-#include "base/no_destructor.h"
 #include "base/notreached.h"
 #include "base/stl_util.h"
 #include "base/time/time.h"
@@ -74,9 +73,6 @@ constexpr base::TimeDelta kZeroAnimationMs =
 constexpr char kCrossFadeSmoothness[] =
     "Ash.Window.AnimationSmoothness.CrossFade";
 
-base::NoDestructor<ui::HistogramPercentageMetricsReporter<kCrossFadeSmoothness>>
-    g_reporter_cross_fade;
-
 base::TimeDelta GetCrossFadeDuration(aura::Window* window,
                                      const gfx::RectF& old_bounds,
                                      const gfx::Rect& new_bounds) {
@@ -104,7 +100,8 @@ base::TimeDelta GetCrossFadeDuration(aura::Window* window,
 // the layer's animation completes, it deletes the layer and removes itself as
 // an observer.
 class CrossFadeObserver : public aura::WindowObserver,
-                          public ui::ImplicitAnimationObserver {
+                          public ui::ImplicitAnimationObserver,
+                          public ui::AnimationMetricsReporter {
  public:
   // Observes |window| for destruction, but does not take ownership.
   // Takes ownership of |layer_owner| and its child layers.
@@ -130,10 +127,24 @@ class CrossFadeObserver : public aura::WindowObserver,
   // ui::ImplicitAnimationObserver:
   void OnImplicitAnimationsCompleted() override { delete this; }
 
+  // ui::AnimationMetricsReporter:
+  void Report(int value) override {
+    if (reported_)
+      return;
+
+    UMA_HISTOGRAM_PERCENTAGE(kCrossFadeSmoothness, value);
+    reported_ = true;
+  }
+
  protected:
   // Triggers OnImplicitAnimationsCompleted() to be called and deletes us.
   void StopAnimating() { layer_owner_->root()->GetAnimator()->StopAnimating(); }
 
+  // Report() gets called for each LayerAnimationElement. For cross fade, its
+  // fairly common to animate both transform and opacity, so only report the
+  // metric once.
+  bool reported_ = false;
+
   aura::Window* window_;  // not owned
   std::unique_ptr<ui::LayerTreeOwner> layer_owner_;
 };
@@ -164,6 +175,7 @@ class CrossFadeUpdateTransformObserver
   void OnLayerAnimationStarted(ui::LayerAnimationSequence* sequence) override {
     DCHECK(!layer_owner_->root()->GetAnimator()->IsAnimatingProperty(
         ui::LayerAnimationElement::TRANSFORM));
+    CrossFadeObserver::OnLayerAnimationStarted(sequence);
   }
 
   // ui::CompositorAnimationObserver:
@@ -245,18 +257,10 @@ void CrossFadeAnimationInternal(
     old_layer->GetAnimator()->StopAnimating();
     old_layer->SetTransform(old_transform);
     ui::ScopedLayerAnimationSettings settings(old_layer->GetAnimator());
-    // Animation observer owns the old layer and deletes itself.
-    CrossFadeObserver* observer =
-        animate_old_layer_transform
-            ? new CrossFadeObserver(window, std::move(old_layer_owner))
-            : new CrossFadeUpdateTransformObserver(window,
-                                                   std::move(old_layer_owner));
-    settings.AddObserver(observer);
     settings.SetTransitionDuration(animation_duration);
     settings.SetTweenType(animation_tween_type);
-    // Only add reporter to |old_layer|.
-    settings.SetAnimationMetricsReporter(g_reporter_cross_fade.get());
     settings.DeferPaint();
+
     if (old_on_top) {
       // Only caching render surface when there is an opacity animation and
       // multiple layers.
@@ -307,9 +311,20 @@ void CrossFadeAnimationInternal(
     new_layer->SetOpacity(kWindowAnimation_HideOpacity);
   }
   {
+    // Animation observer owns the old layer and deletes itself. It should be
+    // attached to the new layer so that if the new layer animation gets
+    // aborted, we can delete the old layer.
+    CrossFadeObserver* observer =
+        animate_old_layer_transform
+            ? new CrossFadeObserver(window, std::move(old_layer_owner))
+            : new CrossFadeUpdateTransformObserver(window,
+                                                   std::move(old_layer_owner));
+
     // Animate the new layer to the identity transform, so the window goes to
     // its newly set bounds.
     ui::ScopedLayerAnimationSettings settings(new_layer->GetAnimator());
+    settings.AddObserver(observer);
+    settings.SetAnimationMetricsReporter(observer);
     settings.SetTransitionDuration(animation_duration);
     settings.SetTweenType(animation_tween_type);
     settings.DeferPaint();

ash/wm/window_animations_unittest.cc

@@ -14,6 +14,7 @@
 #include "ash/wm/wm_event.h"
 #include "ash/wm/workspace_controller.h"
 #include "base/command_line.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "base/time/time.h"
 #include "ui/aura/test/test_windows.h"
 #include "ui/aura/window.h"
@@ -172,6 +173,37 @@ TEST_F(WindowAnimationsTest, CrossFadeToBounds) {
                                        base::TimeDelta::FromSeconds(1));
 }
 
+TEST_F(WindowAnimationsTest, CrossFadeHistograms) {
+  constexpr char kHistogram[] = "Ash.Window.AnimationSmoothness.CrossFade";
+  ui::ScopedAnimationDurationScaleMode test_duration_mode(
+      ui::ScopedAnimationDurationScaleMode::NON_ZERO_DURATION);
+
+  base::HistogramTester histogram_tester;
+
+  std::unique_ptr<Window> window(CreateTestWindowInShellWithId(0));
+  window->SetBounds(gfx::Rect(5, 10, 320, 240));
+  window->Show();
+
+  Layer* old_layer = window->layer();
+  old_layer->GetAnimator()->StopAnimating();
+
+  auto* window_state = WindowState::Get(window.get());
+  // Cross fade to a larger size, as in a maximize animation. We should get one
+  // recorded histogram.
+  window_state->SetBoundsDirectCrossFade(gfx::Rect(640, 480));
+  old_layer->GetAnimator()->StopAnimating();
+  window->layer()->GetAnimator()->StopAnimating();
+  histogram_tester.ExpectTotalCount(kHistogram, 1);
+
+  // Cross fade to a smaller size, as in a restore animation. Tests that there
+  // is only one recorded histogram.
+  old_layer = window->layer();
+  window_state->SetBoundsDirectCrossFade(gfx::Rect(5, 10, 320, 240));
+  old_layer->GetAnimator()->StopAnimating();
+  window->layer()->GetAnimator()->StopAnimating();
+  histogram_tester.ExpectTotalCount(kHistogram, 2);
+}
+
 // Tests that when crossfading from a window which has a transform, the cross
 // fading animation should be ignored and the window should set to its desired
 // bounds directly.

ash/wm/window_state.h

@@ -352,6 +352,7 @@ class ASH_EXPORT WindowState : public aura::WindowObserver {
   friend class TabletModeWindowState;
   friend class ScopedBoundsChangeAnimation;
   FRIEND_TEST_ALL_PREFIXES(WindowAnimationsTest, CrossFadeToBounds);
+  FRIEND_TEST_ALL_PREFIXES(WindowAnimationsTest, CrossFadeHistograms);
   FRIEND_TEST_ALL_PREFIXES(WindowAnimationsTest,
                            CrossFadeToBoundsFromTransform);
   FRIEND_TEST_ALL_PREFIXES(WindowStateTest, PipWindowMaskRecreated);