Update CompositorTestSuite mocking of Time

LayerWithRealCompositorTest.ReportMetrics was racy due to the fact that
LayerAnimationElement and SchedulerStateMachine's timing and task queues cannot
be mocked out.

This left the test vulnerable to differences in timing on bots.

The test had a certain expectation of the order of events, which is not actually
a guarantee. An animation's frame could be generated before it entered its wait
loop. Similarly a animation could be delayed in executing for so long that not
enough frames were generated to actually create a report.

This updates the test to mock out time, so that it can reliably control the order of events.

The test has also been moved to LayerAnimatorTest.ReportMetrics. LayerWithRealCompositorTest
utilizes a TestCompositorHost in order to control the Compositor throughout the tests.
This was done by having CompositorTestSuite always create a ScopedTaskEnvironment.
Unfortunately this causes a thread race with base::TimeTicks::Now, as this thread starts
enqueuing work before an individual test can mock out time.

This change also updates CompositorTestSuite to not create the ScopedTaskEnvironment.
Instead each test suite which desires to mock out the time of task runners, must explicitly
create one. Thus allowing tests which need not a full Compositor thread, can instead
be able to mock time separately.

TBR=sadrul@chromium.org
TEST=LayerWithRealCompositorTest.ReportMetrics
LayerAnimatorTest.ReportMetrics

Bug: 709080
Change-Id: Ia12cdcdf2b6c67c8abc21399eae66cdb1d0d1eb1
Reviewed-on: https://chromium-review.googlesource.com/1157234
Commit-Queue: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587940}

ui/compositor/compositor_unittest.cc

@@ -7,6 +7,7 @@
 #include "base/macros.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/test/scoped_task_environment.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/viz/common/frame_sinks/begin_frame_args.h"
@@ -80,6 +81,12 @@ class CompositorTestWithMockedTime : public CompositorTest {
 
 // For tests that run on a real MessageLoop with real time.
 class CompositorTestWithMessageLoop : public CompositorTest {
+ public:
+  CompositorTestWithMessageLoop()
+      : scoped_task_environment_(
+            base::test::ScopedTaskEnvironment::MainThreadType::UI) {}
+  ~CompositorTestWithMessageLoop() override = default;
+
  protected:
   scoped_refptr<base::SingleThreadTaskRunner> CreateTaskRunner() override {
     task_runner_ = base::ThreadTaskRunnerHandle::Get();
@@ -89,6 +96,7 @@ class CompositorTestWithMessageLoop : public CompositorTest {
   base::SequencedTaskRunner* task_runner() { return task_runner_.get(); }
 
  private:
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 };
 

ui/compositor/layer_animator_unittest.cc

@@ -10,7 +10,10 @@
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/scoped_mock_clock_override.h"
+#include "base/test/scoped_task_environment.h"
 #include "base/time/time.h"
 #include "cc/trees/layer_tree_host.h"
 #include "cc/trees/mutator_host.h"
@@ -3250,6 +3253,8 @@ TEST(LayerAnimatorTest, AnimatorRemovedFromCollectionWhenLayerIsDestroyed) {
 }
 
 TEST(LayerAnimatorTest, LayerMovedBetweenCompositorsDuringAnimation) {
+  base::test::ScopedTaskEnvironment scoped_task_environment_(
+      base::test::ScopedTaskEnvironment::MainThreadType::UI);
   bool enable_pixel_output = false;
   ui::ContextFactory* context_factory = nullptr;
   ui::ContextFactoryPrivate* context_factory_private = nullptr;
@@ -3314,6 +3319,8 @@ TEST(LayerAnimatorTest, LayerMovedBetweenCompositorsDuringAnimation) {
 }
 
 TEST(LayerAnimatorTest, ThreadedAnimationSurvivesIfLayerRemovedAdded) {
+  base::test::ScopedTaskEnvironment scoped_task_environment_(
+      base::test::ScopedTaskEnvironment::MainThreadType::UI);
   bool enable_pixel_output = false;
   ui::ContextFactory* context_factory = nullptr;
   ui::ContextFactoryPrivate* context_factory_private = nullptr;
@@ -3355,6 +3362,115 @@ TEST(LayerAnimatorTest, ThreadedAnimationSurvivesIfLayerRemovedAdded) {
   TerminateContextFactoryForTests();
 }
 
+// A simple AnimationMetricsReporter class that remembers smoothness metric
+// when animation completes.
+class TestMetricsReporter : public ui::AnimationMetricsReporter {
+ public:
+  TestMetricsReporter() {}
+  ~TestMetricsReporter() override {}
+
+  bool report_called() { return report_called_; }
+  int value() const { return value_; }
+
+ protected:
+  void Report(int value) override {
+    value_ = value;
+    report_called_ = true;
+  }
+
+ private:
+  bool report_called_ = false;
+  int value_ = -1;
+
+  DISALLOW_COPY_AND_ASSIGN(TestMetricsReporter);
+};
+
+// Starts an animation and tests that incrementing compositor frame count can
+// be used to report animation smoothness metrics. This verifies that when an
+// animation is smooth (frame count matches refresh rate) that we receive a
+// smoothness value of 100.
+TEST(LayerAnimatorTest, ReportMetricsSmooth) {
+  base::ScopedMockClockOverride mock_clock_;
+  const base::TimeDelta kAnimationDuration =
+      base::TimeDelta::FromMilliseconds(100);
+
+  std::unique_ptr<Layer> root(new Layer(LAYER_SOLID_COLOR));
+  TestLayerAnimationDelegate delegate;
+  scoped_refptr<LayerAnimator> animator(CreateImplicitTestAnimator(&delegate));
+
+  std::unique_ptr<ui::LayerAnimationElement> animation_element =
+      ui::LayerAnimationElement::CreateColorElement(SK_ColorRED,
+                                                    kAnimationDuration);
+  ui::LayerAnimationSequence* animation_sequence =
+      new ui::LayerAnimationSequence(std::move(animation_element));
+  TestMetricsReporter reporter;
+
+  animation_sequence->SetAnimationMetricsReporter(&reporter);
+  animator->StartAnimation(animation_sequence);
+  // Advances the frame count, simulating having produced 6 frames, and received
+  // an ack for each one. This implicitly simulates calling LayerAnimator::Step
+  // with an increased clock 6 times.
+  delegate.SetFrameNumber(6);
+
+  // Advancing the clock does not implicitly advance frame count. It allows the
+  // next call of LayerAnimator::Step to detect that the animation is finished,
+  // thus triggering the processing of metrics reporting.
+  mock_clock_.Advance(kAnimationDuration);
+  animator->Step(mock_clock_.NowTicks());
+
+  CHECK(reporter.report_called());
+  // With the clock being controlled, the animations should be smooth regardless
+  // of the test bots. Smoothness is based on a calculated duration of the
+  // animation when compared to the requested duration, as an integer
+  // percentage. With 100 being 100%. When the number of frames match the
+  // refresh rate, reporter should be notified of a smoothness of 100.
+  EXPECT_EQ(reporter.value(), 100);
+}
+
+// Starts an animation and tests that incrementing compositor frame count can
+// be used to report animation smoothness metrics. This verifies that when an
+// animation is not smooth (frame count doesn't match refresh rate) that we
+// receive a smoothness value that is not 100.
+TEST(LayerAnimatorTest, ReportMetricsNotSmooth) {
+  base::ScopedMockClockOverride mock_clock_;
+  const base::TimeDelta kAnimationDuration =
+      base::TimeDelta::FromMilliseconds(100);
+
+  std::unique_ptr<Layer> root(new Layer(LAYER_SOLID_COLOR));
+  TestLayerAnimationDelegate delegate;
+  scoped_refptr<LayerAnimator> animator(CreateImplicitTestAnimator(&delegate));
+
+  std::unique_ptr<ui::LayerAnimationElement> animation_element =
+      ui::LayerAnimationElement::CreateColorElement(SK_ColorRED,
+                                                    kAnimationDuration);
+  ui::LayerAnimationSequence* animation_sequence =
+      new ui::LayerAnimationSequence(std::move(animation_element));
+  TestMetricsReporter reporter;
+
+  animation_sequence->SetAnimationMetricsReporter(&reporter);
+  animator->StartAnimation(animation_sequence);
+  // Advances the frame count, simulating having produced 1 frame, and received
+  // an ack for it. This implicitly simulates calling LayerAnimator::Step with
+  // an increased clock 1 time.
+  delegate.SetFrameNumber(1);
+
+  // Advancing the clock does not implicitly advance frame count. It allows the
+  // next call of LayerAnimator::Step to detect that the animation is finished,
+  // thus triggering the processing of metrics reporting.
+  mock_clock_.Advance(kAnimationDuration);
+  animator->Step(mock_clock_.NowTicks());
+
+  CHECK(reporter.report_called());
+  // With the clock being controlled, we have direct control of the smoothness
+  // regardless of the test bots. Smoothness is based on a calculated duration
+  // of the animation when compared to the requested duration, as an integer
+  // percentage. With 100 being 100%. With a single frame being generated over
+  // 100ms, the reporter should be notified of a smoothness matching the
+  // interval between refreshes.
+  int expected = base::Time::kMillisecondsPerSecond / delegate.GetRefreshRate();
+  EXPECT_EQ(reporter.value(), expected);
+}
+
 class LayerOwnerAnimationObserver : public LayerAnimationObserver {
  public:
   explicit LayerOwnerAnimationObserver(LayerAnimator* animator)

ui/compositor/layer_unittest.cc

@@ -20,6 +20,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_task_environment.h"
 #include "base/trace_event/trace_event.h"
 #include "build/build_config.h"
 #include "cc/animation/animation_events.h"
@@ -129,7 +130,9 @@ class DrawFadedStringLayerDelegate : public LayerDelegate {
 
 class LayerWithRealCompositorTest : public testing::Test {
  public:
-  LayerWithRealCompositorTest() {
+  LayerWithRealCompositorTest()
+      : scoped_task_environment_(
+            base::test::ScopedTaskEnvironment::MainThreadType::UI) {
     if (base::PathService::Get(gfx::DIR_TEST_DATA, &test_data_directory_)) {
       test_data_directory_ = test_data_directory_.AppendASCII("compositor");
     } else {
@@ -271,6 +274,7 @@ class LayerWithRealCompositorTest : public testing::Test {
     std::unique_ptr<base::RunLoop> run_loop_;
   };
 
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
   std::unique_ptr<TestCompositorHost> compositor_host_;
 
   // The root directory for test files.
@@ -489,7 +493,9 @@ TEST_F(LayerWithRealCompositorTest, Hierarchy) {
 
 class LayerWithDelegateTest : public testing::Test {
  public:
-  LayerWithDelegateTest() {}
+  LayerWithDelegateTest()
+      : scoped_task_environment_(
+            base::test::ScopedTaskEnvironment::MainThreadType::UI) {}
   ~LayerWithDelegateTest() override {}
 
   // Overridden from testing::Test:
@@ -556,6 +562,7 @@ class LayerWithDelegateTest : public testing::Test {
   }
 
  private:
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
   std::unique_ptr<TestCompositorHost> compositor_host_;
 
   DISALLOW_COPY_AND_ASSIGN(LayerWithDelegateTest);
@@ -2675,53 +2682,6 @@ TEST_F(LayerWithRealCompositorTest, CompositorAnimationObserverTest) {
   EXPECT_TRUE(animation_observer.shutdown());
 }
 
-// A simple AnimationMetricsReporter class that remembers smoothness metric
-// when animation completes.
-class TestMetricsReporter : public ui::AnimationMetricsReporter {
- public:
-  TestMetricsReporter() {}
-  ~TestMetricsReporter() override {}
-
-  bool report_called() { return report_called_; }
-  int value() const { return value_; }
-
- protected:
-  void Report(int value) override {
-    value_ = value;
-    report_called_ = true;
-  }
-
- private:
-  bool report_called_ = false;
-  int value_ = -1;
-
-  DISALLOW_COPY_AND_ASSIGN(TestMetricsReporter);
-};
-
-// Starts an animation and tests that incrementing compositor frame count can
-// be used to report animation smoothness metrics.
-// Flaky test crbug.com/709080
-TEST_F(LayerWithRealCompositorTest, DISABLED_ReportMetrics) {
-  std::unique_ptr<Layer> root(CreateLayer(LAYER_SOLID_COLOR));
-  GetCompositor()->SetRootLayer(root.get());
-  LayerAnimator* animator = root->GetAnimator();
-  std::unique_ptr<ui::LayerAnimationElement> animation_element =
-      ui::LayerAnimationElement::CreateColorElement(
-          SK_ColorRED, base::TimeDelta::FromMilliseconds(100));
-  ui::LayerAnimationSequence* animation_sequence =
-      new ui::LayerAnimationSequence(std::move(animation_element));
-  TestMetricsReporter reporter;
-  animation_sequence->SetAnimationMetricsReporter(&reporter);
-  animator->StartAnimation(animation_sequence);
-  while (!reporter.report_called())
-    WaitForSwap();
-  ResetCompositor();
-  // Even though most of the time 100% smooth animations are expected, on the
-  // test bots this cannot be guaranteed. Therefore simply check that some
-  // value was reported.
-  EXPECT_GT(reporter.value(), 0);
-}
-
 TEST(LayerDebugInfoTest, LayerNameDoesNotClobber) {
   Layer layer(LAYER_NOT_DRAWN);
   layer.set_name("foo");

ui/compositor/test/test_layer_animation_delegate.cc

@@ -49,6 +49,10 @@ void TestLayerAnimationDelegate::ExpectLastPropertyChangeReason(
   last_property_change_reason_is_set_ = false;
 }
 
+void TestLayerAnimationDelegate::SetFrameNumber(int frame_number) {
+  frame_number_ = frame_number;
+}
+
 void TestLayerAnimationDelegate::SetBoundsFromAnimation(
     const gfx::Rect& bounds,
     PropertyChangeReason reason) {
@@ -159,7 +163,7 @@ TestLayerAnimationDelegate::GetThreadedAnimationDelegate() {
 }
 
 int TestLayerAnimationDelegate::GetFrameNumber() const {
-  return 0;
+  return frame_number_;
 }
 
 float TestLayerAnimationDelegate::GetRefreshRate() const {

ui/compositor/test/test_layer_animation_delegate.h

@@ -40,6 +40,11 @@ class TestLayerAnimationDelegate : public LayerAnimationDelegate {
   // PropertyChangeReason.
   void ExpectLastPropertyChangeReason(PropertyChangeReason reason);
 
+  // Sets the current frame number to be returned by GetFrameNumber. This can be
+  // used to simulate receiving acks of frame submission, in order to test
+  // advancing of animations.
+  void SetFrameNumber(int frame_number);
+
   // Implementation of LayerAnimationDelegate
   void SetBoundsFromAnimation(const gfx::Rect& bounds,
                               PropertyChangeReason reason) override;
@@ -88,6 +93,7 @@ class TestLayerAnimationDelegate : public LayerAnimationDelegate {
   float grayscale_;
   SkColor color_;
   scoped_refptr<cc::Layer> cc_layer_;
+  int frame_number_ = 0;
 
   // Allow copy and assign.
 };

ui/compositor/test/test_suite.cc

@@ -5,7 +5,6 @@
 #include "ui/compositor/test/test_suite.h"
 
 #include "base/command_line.h"
-#include "base/test/scoped_task_environment.h"
 #include "build/build_config.h"
 #include "ui/compositor/compositor.h"
 #include "ui/compositor/compositor_switches.h"
@@ -50,16 +49,6 @@ void CompositorTestSuite::Initialize() {
 #if defined(OS_WIN)
   display::win::SetDefaultDeviceScaleFactor(1.0f);
 #endif
-
-  scoped_task_environment_ =
-      std::make_unique<base::test::ScopedTaskEnvironment>(
-          base::test::ScopedTaskEnvironment::MainThreadType::UI);
-}
-
-void CompositorTestSuite::Shutdown() {
-  scoped_task_environment_.reset();
-
-  base::TestSuite::Shutdown();
 }
 
 }  // namespace test

ui/compositor/test/test_suite.h

@@ -5,18 +5,10 @@
 #ifndef UI_COMPOSITOR_TEST_TEST_SUITE_H_
 #define UI_COMPOSITOR_TEST_TEST_SUITE_H_
 
-#include <memory>
-
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/test/test_suite.h"
 
-namespace base {
-namespace test {
-class ScopedTaskEnvironment;
-}
-}
-
 namespace ui {
 namespace test {
 
@@ -28,11 +20,8 @@ class CompositorTestSuite : public base::TestSuite {
  protected:
   // Overridden from base::TestSuite:
   void Initialize() override;
-  void Shutdown() override;
 
  private:
-  std::unique_ptr<base::test::ScopedTaskEnvironment> scoped_task_environment_;
-
   DISALLOW_COPY_AND_ASSIGN(CompositorTestSuite);
 };
 

ui/snapshot/snapshot_aura_unittest.cc

@@ -10,6 +10,7 @@
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/win/windows_version.h"
 #include "build/build_config.h"
@@ -98,6 +99,10 @@ class SnapshotAuraTest : public testing::Test {
   void SetUp() override {
     testing::Test::SetUp();
 
+    scoped_task_environment_ =
+        std::make_unique<base::test::ScopedTaskEnvironment>(
+            base::test::ScopedTaskEnvironment::MainThreadType::UI);
+
     // The ContextFactory must exist before any Compositors are created.
     // Snapshot test tests real drawing and readback, so needs pixel output.
     bool enable_pixel_output = true;
@@ -118,6 +123,7 @@ class SnapshotAuraTest : public testing::Test {
     helper_->RunAllPendingInMessageLoop();
     helper_->TearDown();
     ui::TerminateContextFactoryForTests();
+    scoped_task_environment_.reset();
     testing::Test::TearDown();
   }
 
@@ -178,6 +184,7 @@ class SnapshotAuraTest : public testing::Test {
     bool completed_;
   };
 
+  std::unique_ptr<base::test::ScopedTaskEnvironment> scoped_task_environment_;
   std::unique_ptr<aura::test::AuraTestHelper> helper_;
   std::unique_ptr<aura::Window> test_window_;
   std::unique_ptr<TestPaintingWindowDelegate> delegate_;