Fix many shell tests

flow/frame_timings.cc

@@ -255,7 +255,9 @@ const char* FrameTimingsRecorder::GetFrameNumberTraceArg() const {
 }
 
 void FrameTimingsRecorder::AssertInState(State state) const {
-  FML_DCHECK(state_ == state);
+  FML_DCHECK(state_ == state)
+      << "Expected state " << static_cast<uint32_t>(state) << ", actual state "
+      << static_cast<uint32_t>(state_);
 }
 
 }  // namespace flutter

shell/common/animator.cc

@@ -60,6 +60,7 @@ void Animator::EnqueueTraceFlowId(uint64_t trace_flow_id) {
 
 void Animator::BeginFrame(
     std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder) {
+  FML_CHECK(frame_timings_recorder_ == nullptr);
   TRACE_EVENT_ASYNC_END0("flutter", "Frame Request Pending",
                          frame_request_number_);
   frame_request_number_++;
@@ -112,21 +113,34 @@ void Animator::BeginFrame(
   dart_frame_deadline_ = frame_target_time.ToEpochDelta();
   uint64_t frame_number = frame_timings_recorder_->GetFrameNumber();
   delegate_.OnAnimatorBeginFrame(frame_target_time, frame_number);
+}
 
-  // Commit the pending continuation.
-  PipelineProduceResult result =
-      producer_continuation_.Complete(std::make_unique<FrameItem>(
-          std::move(layer_trees_tasks_), std::move(frame_timings_recorder_)));
-
-  if (!result.success) {
-    FML_DLOG(INFO) << "Failed to commit to the pipeline";
-  } else if (!result.is_first_item) {
-    // Do nothing. It has been successfully pushed to the pipeline but not as
-    // the first item. Eventually the 'Rasterizer' will consume it, so we don't
-    // need to notify the delegate.
-  } else {
-    delegate_.OnAnimatorDraw(layer_tree_pipeline_);
+void Animator::EndFrame() {
+  FML_CHECK(frame_timings_recorder_ != nullptr);
+  if (!layer_trees_tasks_.empty()) {
+    // The build is completed in OnAnimatorBeginFrame.
+    frame_timings_recorder_->RecordBuildEnd(fml::TimePoint::Now());
+
+    delegate_.OnAnimatorUpdateLatestFrameTargetTime(
+        frame_timings_recorder_->GetVsyncTargetTime());
+
+    // Commit the pending continuation.
+    PipelineProduceResult result =
+        producer_continuation_.Complete(std::make_unique<FrameItem>(
+            std::move(layer_trees_tasks_), std::move(frame_timings_recorder_)));
+
+    if (!result.success) {
+      FML_DLOG(INFO) << "Failed to commit to the pipeline";
+    } else if (!result.is_first_item) {
+      // Do nothing. It has been successfully pushed to the pipeline but not as
+      // the first item. Eventually the 'Rasterizer' will consume it, so we
+      // don't need to notify the delegate.
+    } else {
+      delegate_.OnAnimatorDraw(layer_tree_pipeline_);
+    }
   }
+  FML_DCHECK(layer_trees_tasks_.empty());
+  frame_timings_recorder_ = nullptr;  // Ensure it's cleared.
 
   if (!frame_scheduled_ && has_rendered_) {
     // Wait a tad more than 3 60hz frames before reporting a big idle period.
@@ -165,10 +179,6 @@ void Animator::Render(int64_t view_id,
   TRACE_EVENT_WITH_FRAME_NUMBER(frame_timings_recorder_, "flutter",
                                 "Animator::Render", /*flow_id_count=*/0,
                                 /*flow_ids=*/nullptr);
-  frame_timings_recorder_->RecordBuildEnd(fml::TimePoint::Now());
-
-  delegate_.OnAnimatorUpdateLatestFrameTargetTime(
-      frame_timings_recorder_->GetVsyncTargetTime());
 
   layer_trees_tasks_.push_back(std::make_unique<LayerTreeTask>(
       view_id, std::move(layer_tree), device_pixel_ratio));
@@ -243,6 +253,7 @@ void Animator::AwaitVSync() {
             self->DrawLastLayerTrees(std::move(frame_timings_recorder));
           } else {
             self->BeginFrame(std::move(frame_timings_recorder));
+            self->EndFrame();
           }
         }
       });

shell/common/animator.h

@@ -84,6 +84,7 @@ class Animator final {
 
  private:
   void BeginFrame(std::unique_ptr<FrameTimingsRecorder> frame_timings_recorder);
+  void EndFrame();
 
   bool CanReuseLastLayerTrees();
 

shell/common/animator_unittests.cc

@@ -160,20 +160,30 @@ TEST_F(ShellTest, AnimatorDoesNotNotifyIdleBeforeRender) {
   latch.Wait();
   ASSERT_FALSE(delegate.notify_idle_called_);
 
+  fml::AutoResetWaitableEvent render_latch;
   // Validate it has not notified idle and try to render.
   task_runners.GetUITaskRunner()->PostDelayedTask(
       [&] {
         ASSERT_FALSE(delegate.notify_idle_called_);
-        auto layer_tree = std::make_unique<LayerTree>(LayerTree::Config(),
-                                                      SkISize::Make(600, 800));
-        animator->Render(kImplicitViewId, std::move(layer_tree), 1.0);
+        EXPECT_CALL(delegate, OnAnimatorBeginFrame).WillOnce([&] {
+          auto layer_tree = std::make_unique<LayerTree>(
+              LayerTree::Config(), SkISize::Make(600, 800));
+          animator->Render(kImplicitViewId, std::move(layer_tree), 1.0);
+          render_latch.Signal();
+        });
+        // Request a frame that builds a layer tree and renders a frame.
+        // When the frame is rendered, render_latch will be signaled.
+        animator->RequestFrame(true);
         task_runners.GetPlatformTaskRunner()->PostTask(flush_vsync_task);
       },
       // See kNotifyIdleTaskWaitTime in animator.cc.
       fml::TimeDelta::FromMilliseconds(60));
   latch.Wait();
+  render_latch.Wait();
 
-  // Still hasn't notified idle because there has been no frame request.
+  // A frame has been rendered, and the next frame request will notify idle.
+  // But at the moment there isn't another frame request, therefore it still
+  // hasn't notified idle.
   task_runners.GetUITaskRunner()->PostTask([&] {
     ASSERT_FALSE(delegate.notify_idle_called_);
     // False to avoid getting cals to BeginFrame that will request more frames
@@ -226,11 +236,6 @@ TEST_F(ShellTest, AnimatorDoesNotNotifyDelegateIfPipelineIsNotEmpty) {
   });
 
   fml::AutoResetWaitableEvent begin_frame_latch;
-  EXPECT_CALL(delegate, OnAnimatorBeginFrame)
-      .WillRepeatedly(
-          [&](fml::TimePoint frame_target_time, uint64_t frame_number) {
-            begin_frame_latch.Signal();
-          });
   // It must always be called when the method 'Animator::Render' is called,
   // regardless of whether the pipeline is empty or not.
   EXPECT_CALL(delegate, OnAnimatorUpdateLatestFrameTargetTime).Times(2);
@@ -241,16 +246,16 @@ TEST_F(ShellTest, AnimatorDoesNotNotifyDelegateIfPipelineIsNotEmpty) {
 
   for (int i = 0; i < 2; i++) {
     task_runners.GetUITaskRunner()->PostTask([&] {
+      EXPECT_CALL(delegate, OnAnimatorBeginFrame).WillOnce([&] {
+        auto layer_tree = std::make_unique<LayerTree>(LayerTree::Config(),
+                                                      SkISize::Make(600, 800));
+        animator->Render(kImplicitViewId, std::move(layer_tree), 1.0);
+        begin_frame_latch.Signal();
+      });
       animator->RequestFrame();
       task_runners.GetPlatformTaskRunner()->PostTask(flush_vsync_task);
     });
     begin_frame_latch.Wait();
-
-    PostTaskSync(task_runners.GetUITaskRunner(), [&] {
-      auto layer_tree = std::make_unique<LayerTree>(LayerTree::Config(),
-                                                    SkISize::Make(600, 800));
-      animator->Render(kImplicitViewId, std::move(layer_tree), 1.0);
-    });
   }
 
   PostTaskSync(task_runners.GetUITaskRunner(), [&] { animator.reset(); });

shell/common/shell_test.cc

@@ -154,6 +154,7 @@ void ShellTest::SetViewportMetrics(Shell* shell, double width, double height) {
               std::make_unique<FrameTimingsRecorder>();
           recorder->RecordVsync(frame_begin_time, frame_end_time);
           engine->animator_->BeginFrame(std::move(recorder));
+          engine->animator_->EndFrame();
         }
         latch.Signal();
       });
@@ -181,7 +182,8 @@ void ShellTest::PumpOneFrame(Shell* shell,
 
 void ShellTest::PumpOneFrame(Shell* shell,
                              const flutter::ViewportMetrics& viewport_metrics,
-                             LayerTreeBuilder builder) {
+                             LayerTreeBuilder builder,
+                             bool render_implicit_view) {
   // Set viewport to nonempty, and call Animator::BeginFrame to make the layer
   // tree pipeline nonempty. Without either of this, the layer tree below
   // won't be rasterized.
@@ -204,21 +206,25 @@ void ShellTest::PumpOneFrame(Shell* shell,
   // Call |Render| to rasterize a layer tree and trigger |OnFrameRasterized|
   fml::WeakPtr<RuntimeDelegate> runtime_delegate = shell->weak_engine_;
   shell->GetTaskRunners().GetUITaskRunner()->PostTask(
-      [&latch, runtime_delegate, &builder, viewport_metrics]() {
-        SkMatrix identity;
-        identity.setIdentity();
-        auto root_layer = std::make_shared<TransformLayer>(identity);
-        auto layer_tree = std::make_unique<LayerTree>(
-            LayerTree::Config{.root_layer = root_layer},
-            SkISize::Make(viewport_metrics.physical_width,
-                          viewport_metrics.physical_height));
-        float device_pixel_ratio =
-            static_cast<float>(viewport_metrics.device_pixel_ratio);
-        if (builder) {
-          builder(root_layer);
+      [&latch, engine = shell->weak_engine_, runtime_delegate, &builder,
+       viewport_metrics, render_implicit_view]() {
+        if (render_implicit_view) {
+          SkMatrix identity;
+          identity.setIdentity();
+          auto root_layer = std::make_shared<TransformLayer>(identity);
+          auto layer_tree = std::make_unique<LayerTree>(
+              LayerTree::Config{.root_layer = root_layer},
+              SkISize::Make(viewport_metrics.physical_width,
+                            viewport_metrics.physical_height));
+          float device_pixel_ratio =
+              static_cast<float>(viewport_metrics.device_pixel_ratio);
+          if (builder) {
+            builder(root_layer);
+          }
+          runtime_delegate->Render(kImplicitViewId, std::move(layer_tree),
+                                   device_pixel_ratio);
         }
-        runtime_delegate->Render(kImplicitViewId, std::move(layer_tree),
-                                 device_pixel_ratio);
+        engine->animator_->EndFrame();
         latch.Signal();
       });
   latch.Wait();

shell/common/shell_test.h

@@ -85,9 +85,11 @@ class ShellTest : public FixtureTest {
                            double width = 1,
                            double height = 1,
                            LayerTreeBuilder = {});
+  // TODO(dkwingsmt): Refactor builder and render_implicit_view
   static void PumpOneFrame(Shell* shell,
                            const flutter::ViewportMetrics& viewport_metrics,
-                           LayerTreeBuilder = {});
+                           LayerTreeBuilder = {},
+                           bool render_implicit_view = true);
   static void DispatchFakePointerData(Shell* shell);
   static void DispatchPointerData(Shell* shell,
                                   std::unique_ptr<PointerDataPacket> packet);

shell/common/shell_unittests.cc

@@ -1059,6 +1059,9 @@ TEST_F(ShellTest, OnPlatformViewDestroyDisablesThreadMerger) {
   PumpOneFrame(shell.get(), 100, 100, builder);
 
   auto result = shell->WaitForFirstFrame(fml::TimeDelta::Max());
+  // Wait for the rasterizer to process the frame. WaitForFirstFrame only waits
+  // for the Animator, but end_frame_callback is called by the Rasterizer.
+  PostSync(shell->GetTaskRunners().GetRasterTaskRunner(), [] {});
   ASSERT_TRUE(result.ok()) << "Result: " << static_cast<int>(result.code())
                            << ": " << result.message();
 
@@ -1562,8 +1565,9 @@ TEST_F(ShellTest, WaitForFirstFrameZeroSizeFrame) {
   RunEngine(shell.get(), std::move(configuration));
   PumpOneFrame(shell.get(), {1.0, 0.0, 0.0, 22, 0});
   fml::Status result = shell->WaitForFirstFrame(fml::TimeDelta::Zero());
-  ASSERT_FALSE(result.ok());
-  ASSERT_EQ(result.code(), fml::StatusCode::kDeadlineExceeded);
+  EXPECT_FALSE(result.ok());
+  EXPECT_EQ(result.message(), "timeout");
+  EXPECT_EQ(result.code(), fml::StatusCode::kDeadlineExceeded);
 
   DestroyShell(std::move(shell));
 }
@@ -2616,7 +2620,7 @@ TEST_F(ShellTest, OnServiceProtocolRenderFrameWithRasterStatsWorks) {
   configuration.SetEntrypoint("scene_with_red_box");
 
   RunEngine(shell.get(), std::move(configuration));
-  PumpOneFrame(shell.get());
+  PumpOneFrame(shell.get(), {1.0, 1, 1, 22, 0}, {}, false);
 
   ServiceProtocol::Handler::ServiceProtocolMap empty_params;
   rapidjson::Document document;