Revert "Reland "wm: Improve cross fade animation.""

This reverts commit 4370f074.

Reason for revert: Causing 
HomeToOverviewNudgeControllerTest.NudgeHiddenDuringShowAnimation to
fail.

https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-rel/38470

https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8878645531766485392/+/steps/ash_unittests/0/logs/Deterministic_failure:_HomeToOverviewNudgeControllerTest.NudgeHiddenDuringShowAnimation__status_CRASH_/0

Original change's description:
> Reland "wm: Improve cross fade animation."
> 
> This is a reland of 3abaa7f0.
> 
> Changes: Now that the observer observes the new layer, on window
> destroying or removing from root window we should stop animating the
> new window as well.
> 
> Original change's description:
> > 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/+/2218557
> > Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> > Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#773397}
> 
> Bug: 868170, 1087460, 1088169
> Change-Id: I3a2f6af334ca0cd325e49107e812824f7a39340e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2224990
> Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#773921}

TBR=oshima@chromium.org,sammiequon@chromium.org

Change-Id: I043c684a88133cf6210c5b2d53ecd3d31583bc2a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 868170, 1087460, 1088169
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2226004Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774021}

ash/wm/window_animations.cc

@@ -19,6 +19,7 @@
 #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"
@@ -73,6 +74,9 @@ 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) {
@@ -100,8 +104,7 @@ 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::AnimationMetricsReporter {
+                          public ui::ImplicitAnimationObserver {
  public:
   // Observes |window| for destruction, but does not take ownership.
   // Takes ownership of |layer_owner| and its child layers.
@@ -127,27 +130,9 @@ 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();
-    DCHECK(window_);
-    window_->layer()->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;
+  void StopAnimating() { layer_owner_->root()->GetAnimator()->StopAnimating(); }
 
   aura::Window* window_;  // not owned
   std::unique_ptr<ui::LayerTreeOwner> layer_owner_;
@@ -179,7 +164,6 @@ class CrossFadeUpdateTransformObserver
   void OnLayerAnimationStarted(ui::LayerAnimationSequence* sequence) override {
     DCHECK(!layer_owner_->root()->GetAnimator()->IsAnimatingProperty(
         ui::LayerAnimationElement::TRANSFORM));
-    CrossFadeObserver::OnLayerAnimationStarted(sequence);
   }
 
   // ui::CompositorAnimationObserver:
@@ -261,10 +245,18 @@ 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.
@@ -315,20 +307,9 @@ 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,7 +14,6 @@
 #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"
@@ -173,37 +172,6 @@ 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,7 +352,6 @@ 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);