Revert "Revert skiping illegal render in Dart (flutter#49473)"

This reverts commit 4325486.

Author: dkwingsmt

lib/ui/platform_dispatcher.dart

@@ -308,9 +308,8 @@ class PlatformDispatcher {
     _invoke(onMetricsChanged, _onMetricsChangedZone);
   }
 
-  // A debug-only variable that stores the [FlutterView]s for which
-  // [FlutterView.render] has already been called during the current
-  // [onBeginFrame]/[onDrawFrame] callback sequence.
+  // The [FlutterView]s for which [FlutterView.render] has already been called
+  // during the current [onBeginFrame]/[onDrawFrame] callback sequence.
   //
   // It is null outside the scope of those callbacks indicating that calls to
   // [FlutterView.render] must be ignored. Furthermore, if a given [FlutterView]
@@ -320,16 +319,9 @@ class PlatformDispatcher {
   // Between [onBeginFrame] and [onDrawFrame] the properties value is
   // temporarily stored in `_renderedViewsBetweenCallbacks` so that it survives
   // the gap between the two callbacks.
-  //
-  // In release build, this variable is null, and therefore the calling rule is
-  // not enforced. This is because the check might hurt cold startup delay;
-  // see https://github.com/flutter/engine/pull/46919.
-  Set<FlutterView>? _debugRenderedViews;
-  // A debug-only variable that temporarily stores the `_renderedViews` value
-  // between `_beginFrame` and `_drawFrame`.
-  //
-  // In release build, this variable is null.
-  Set<FlutterView>? _debugRenderedViewsBetweenCallbacks;
+  Set<FlutterView>? _renderedViews;
+  // The `_renderedViews` value between `_beginFrame` and `_drawFrame`.
+  Set<FlutterView>? _renderedViewsBetweenCallbacks;
 
   /// A callback invoked when any view begins a frame.
   ///
@@ -351,26 +343,20 @@ class PlatformDispatcher {
 
   // Called from the engine, via hooks.dart
   void _beginFrame(int microseconds) {
-    assert(_debugRenderedViews == null);
-    assert(_debugRenderedViewsBetweenCallbacks == null);
-    assert(() {
-      _debugRenderedViews = <FlutterView>{};
-      return true;
-    }());
+    assert(_renderedViews == null);
+    assert(_renderedViewsBetweenCallbacks == null);
+    _renderedViews = <FlutterView>{};
 
     _invoke1<Duration>(
       onBeginFrame,
       _onBeginFrameZone,
       Duration(microseconds: microseconds),
     );
 
-    assert(_debugRenderedViews != null);
-    assert(_debugRenderedViewsBetweenCallbacks == null);
-    assert(() {
-      _debugRenderedViewsBetweenCallbacks = _debugRenderedViews;
-      _debugRenderedViews = null;
-      return true;
-    }());
+    assert(_renderedViews != null);
+    assert(_renderedViewsBetweenCallbacks == null);
+    _renderedViewsBetweenCallbacks = _renderedViews;
+    _renderedViews = null;
   }
 
   /// A callback that is invoked for each frame after [onBeginFrame] has
@@ -388,22 +374,16 @@ class PlatformDispatcher {
 
   // Called from the engine, via hooks.dart
   void _drawFrame() {
-    assert(_debugRenderedViews == null);
-    assert(_debugRenderedViewsBetweenCallbacks != null);
-    assert(() {
-      _debugRenderedViews = _debugRenderedViewsBetweenCallbacks;
-      _debugRenderedViewsBetweenCallbacks = null;
-      return true;
-    }());
+    assert(_renderedViews == null);
+    assert(_renderedViewsBetweenCallbacks != null);
+    _renderedViews = _renderedViewsBetweenCallbacks;
+    _renderedViewsBetweenCallbacks = null;
 
     _invoke(onDrawFrame, _onDrawFrameZone);
 
-    assert(_debugRenderedViews != null);
-    assert(_debugRenderedViewsBetweenCallbacks == null);
-    assert(() {
-      _debugRenderedViews = null;
-      return true;
-    }());
+    assert(_renderedViews != null);
+    assert(_renderedViewsBetweenCallbacks == null);
+    _renderedViews = null;
   }
 
   /// A callback that is invoked when pointer data is available.

lib/ui/window.dart

@@ -373,14 +373,8 @@ class FlutterView {
   ///   painting.
   void render(Scene scene, {Size? size}) {
     // Duplicated calls or calls outside of onBeginFrame/onDrawFrame (indicated
-    // by _debugRenderedViews being null) are ignored. See _debugRenderedViews.
-    // TODO(dkwingsmt): We should change this skip into an assertion.
-    // https://github.com/flutter/flutter/issues/137073
-    bool validRender = true;
-    assert(() {
-      validRender = platformDispatcher._debugRenderedViews?.add(this) ?? false;
-      return true;
-    }());
+    // by _renderedViews being null) are ignored. See _renderedViews.
+    final bool validRender = platformDispatcher._renderedViews?.add(this) ?? false;
     if (validRender) {
       _render(scene as _NativeScene, size?.width ?? physicalSize.width, size?.height ?? physicalSize.height);
     }

shell/common/animator.cc

@@ -143,15 +143,12 @@ void Animator::BeginFrame(
 
 void Animator::Render(std::unique_ptr<flutter::LayerTree> layer_tree,
                       float device_pixel_ratio) {
-  has_rendered_ = true;
-
-  if (!frame_timings_recorder_) {
-    // Framework can directly call render with a built scene.
-    frame_timings_recorder_ = std::make_unique<FrameTimingsRecorder>();
-    const fml::TimePoint placeholder_time = fml::TimePoint::Now();
-    frame_timings_recorder_->RecordVsync(placeholder_time, placeholder_time);
-    frame_timings_recorder_->RecordBuildStart(placeholder_time);
+  // Animator::Render should be called after BeginFrame, which is indicated by
+  // frame_timings_recorder_ being non-null. Otherwise, this call is ignored.
+  if (frame_timings_recorder_ == nullptr) {
+    return;
   }
+  has_rendered_ = true;
 
   TRACE_EVENT_WITH_FRAME_NUMBER(frame_timings_recorder_, "flutter",
                                 "Animator::Render", /*flow_id_count=*/0,

shell/common/animator.h

@@ -58,7 +58,8 @@ class Animator final {
   ///
   ///           This method must be called during a vsync callback, or
   ///           technically, between Animator::BeginFrame and Animator::EndFrame
-  ///           (both private methods). Otherwise, this call will be ignored.
+  ///           (both private methods). Otherwise, an assertion will be
+  ///           triggered.
   ///
   void Render(std::unique_ptr<flutter::LayerTree> layer_tree,
               float device_pixel_ratio);

shell/common/animator_unittests.cc

@@ -158,20 +158,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(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(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
@@ -239,16 +249,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(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(std::move(layer_tree), 1.0);
-    });
   }
 
   PostTaskSync(task_runners.GetUITaskRunner(), [&] { animator.reset(); });