Add completion callback to Shell::AddView

loic-sharma · loic-sharma · commit ecbeecb74b7b · 2024-03-25T15:37:11.000-07:00
diff --git a/lib/ui/window/platform_configuration.cc b/lib/ui/window/platform_configuration.cc
@@ -83,15 +83,18 @@ void PlatformConfiguration::DidCreateIsolate() {
                       Dart_GetField(library, tonic::ToDart("_reportTimings")));
 }
 
-void PlatformConfiguration::AddView(int64_t view_id,
+bool PlatformConfiguration::AddView(int64_t view_id,
                                     const ViewportMetrics& view_metrics) {
   auto [view_iterator, insertion_happened] =
       metrics_.emplace(view_id, view_metrics);
-  FML_DCHECK(insertion_happened);
+  if (!insertion_happened) {
+    FML_LOG(ERROR) << "View #" << view_id << " already exists.";
+    return false;
+  }
 
   std::shared_ptr<tonic::DartState> dart_state = add_view_.dart_state().lock();
   if (!dart_state) {
-    return;
+    return false;
   }
   tonic::DartState::Scope scope(dart_state);
   tonic::CheckAndHandleError(tonic::DartInvoke(
@@ -119,6 +122,7 @@ void PlatformConfiguration::AddView(int64_t view_id,
           tonic::ToDart(view_metrics.physical_display_features_state),
           tonic::ToDart(view_metrics.display_id),
       }));
+  return true;
 }
 
 bool PlatformConfiguration::RemoveView(int64_t view_id) {
diff --git a/lib/ui/window/platform_configuration.h b/lib/ui/window/platform_configuration.h
@@ -263,7 +263,7 @@ class PlatformConfigurationClient {
 ///             to and from the Dart code in Flutter's framework.
 ///
 ///             It handles communication between the engine and the framework,
-///             and owns the main window.
+///             and owns the views' metrics.
 ///
 ///             It communicates with the RuntimeController through the use of a
 ///             PlatformConfigurationClient interface, which the
@@ -315,7 +315,9 @@ class PlatformConfiguration final {
   /// @param[in]  view_id           The ID of the new view.
   /// @param[in]  viewport_metrics  The initial viewport metrics for the view.
   ///
-  void AddView(int64_t view_id, const ViewportMetrics& view_metrics);
+  /// @return     Whether the view was added.
+  ///
+  bool AddView(int64_t view_id, const ViewportMetrics& view_metrics);
 
   //----------------------------------------------------------------------------
   /// @brief      Notify the framework that a view is no longer available.
diff --git a/runtime/runtime_controller.cc b/runtime/runtime_controller.cc
@@ -121,12 +121,29 @@ bool RuntimeController::FlushRuntimeStateToIsolate() {
   FML_DCHECK(!has_flushed_runtime_state_)
       << "FlushRuntimeStateToIsolate is called more than once somehow.";
   has_flushed_runtime_state_ = true;
+
+  auto platform_configuration = GetPlatformConfigurationIfAvailable();
+  if (!platform_configuration) {
+    return false;
+  }
+
   for (auto const& [view_id, viewport_metrics] :
        platform_data_.viewport_metrics_for_views) {
-    if (!AddView(view_id, viewport_metrics)) {
+    bool added = platform_configuration->AddView(view_id, viewport_metrics);
+
+    // Callbacks will have been already invoked if the engine was restarted.
+    if (pending_add_view_callbacks_.find(view_id) !=
+        pending_add_view_callbacks_.end()) {
+      pending_add_view_callbacks_[view_id](added);
+      pending_add_view_callbacks_.erase(view_id);
+    }
+
+    if (!added) {
       return false;
     }
   }
+
+  FML_DCHECK(pending_add_view_callbacks_.empty());
   return SetLocales(platform_data_.locale_data) &&
          SetSemanticsEnabled(platform_data_.semantics_enabled) &&
          SetAccessibilityFeatures(
@@ -136,16 +153,22 @@ bool RuntimeController::FlushRuntimeStateToIsolate() {
          SetDisplays(platform_data_.displays);
 }
 
-bool RuntimeController::AddView(int64_t view_id,
-                                const ViewportMetrics& view_metrics) {
+void RuntimeController::AddView(int64_t view_id,
+                                const ViewportMetrics& view_metrics,
+                                AddViewCallback callback) {
   platform_data_.viewport_metrics_for_views[view_id] = view_metrics;
-  if (auto* platform_configuration = GetPlatformConfigurationIfAvailable()) {
-    platform_configuration->AddView(view_id, view_metrics);
 
-    return true;
+  // If the Dart isolate is not running, |FlushRuntimeStateToIsolate| will
+  // add the view when the isolate is started.
+  auto* platform_configuration = GetPlatformConfigurationIfAvailable();
+  if (!platform_configuration) {
+    FML_DCHECK(has_flushed_runtime_state_ == false);
+    pending_add_view_callbacks_[view_id] = std::move(callback);
+    return;
   }
 
-  return false;
+  bool added = platform_configuration->AddView(view_id, view_metrics);
+  callback(added);
 }
 
 bool RuntimeController::RemoveView(int64_t view_id) {
diff --git a/runtime/runtime_controller.h b/runtime/runtime_controller.h
@@ -50,6 +50,11 @@ class Window;
 ///
 class RuntimeController : public PlatformConfigurationClient {
  public:
+  /// A callback that's invoked after this `RuntimeController` attempts to
+  /// add a view to the Dart isolate. The `added` parameter is true if the add
+  /// operation succeeds.
+  using AddViewCallback = std::function<void(bool added)>;
+
   //----------------------------------------------------------------------------
   /// @brief      Creates a new instance of a runtime controller. This is
   ///             usually only done by the engine instance associated with the
@@ -176,10 +181,17 @@ class RuntimeController : public PlatformConfigurationClient {
   ///             including the implicit view. Adding a view that already exists
   ///             triggers an assertion.
   ///
+  ///             If the isolate is not running, the view will be saved and
+  ///             flushed to the isolate when it starts.
+  ///
   /// @param[in]  view_id           The ID of the new view.
   /// @param[in]  viewport_metrics  The initial viewport metrics for the view.
+  /// @param[in]  callback          Callback that will be invoked after the add
+  ///                               operation is attempted.
   ///
-  bool AddView(int64_t view_id, const ViewportMetrics& view_metrics);
+  void AddView(int64_t view_id,
+               const ViewportMetrics& view_metrics,
+               AddViewCallback callback);
 
   //----------------------------------------------------------------------------
   /// @brief      Notify the isolate that a view is no longer available.
@@ -191,6 +203,9 @@ class RuntimeController : public PlatformConfigurationClient {
   ///
   /// @param[in]  view_id  The ID of the view.
   ///
+  /// @return     If the remove view operation was forwarded to the running
+  ///             isolate.
+  ///
   bool RemoveView(int64_t view_id);
 
   //----------------------------------------------------------------------------
@@ -660,6 +675,11 @@ class RuntimeController : public PlatformConfigurationClient {
       std::shared_ptr<PlatformIsolateManager>(new PlatformIsolateManager());
   bool has_flushed_runtime_state_ = false;
 
+  // Callbacks when `AddView` was called before the Dart isolate is launched.
+  //
+  // These views will be added when `FlushRuntimeStateToIsolate` is called.
+  std::unordered_map<int64_t, AddViewCallback> pending_add_view_callbacks_;
+
   // Tracks the views that have been called `Render` during a frame.
   //
   // If all views that have been registered by `AddView` have been called
diff --git a/shell/common/engine.cc b/shell/common/engine.cc
@@ -297,8 +297,10 @@ tonic::DartErrorHandleType Engine::GetUIIsolateLastError() {
   return runtime_controller_->GetLastError();
 }
 
-void Engine::AddView(int64_t view_id, const ViewportMetrics& view_metrics) {
-  runtime_controller_->AddView(view_id, view_metrics);
+void Engine::AddView(int64_t view_id,
+                     const ViewportMetrics& view_metrics,
+                     std::function<void(bool added)> callback) {
+  runtime_controller_->AddView(view_id, view_metrics, std::move(callback));
 }
 
 bool Engine::RemoveView(int64_t view_id) {
diff --git a/shell/common/engine.h b/shell/common/engine.h
@@ -721,8 +721,12 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate {
   ///
   /// @param[in]  view_id           The ID of the new view.
   /// @param[in]  viewport_metrics  The initial viewport metrics for the view.
+  /// @param[in]  callback          Callback that will be invoked once
+  ///                               the engine attempts to add the view.
   ///
-  void AddView(int64_t view_id, const ViewportMetrics& view_metrics);
+  void AddView(int64_t view_id,
+               const ViewportMetrics& view_metrics,
+               std::function<void(bool added)> callback);
 
   //----------------------------------------------------------------------------
   /// @brief      Notify the Flutter application that a view is no
diff --git a/shell/common/engine_animator_unittests.cc b/shell/common/engine_animator_unittests.cc
@@ -313,8 +313,10 @@ TEST_F(EngineAnimatorTest, AnimatorAcceptsMultipleRenders) {
   engine_context->Run(std::move(configuration));
 
   engine_context->EngineTaskSync([](Engine& engine) {
-    engine.AddView(1, ViewportMetrics{1, 10, 10, 22, 0});
-    engine.AddView(2, ViewportMetrics{1, 10, 10, 22, 0});
+    engine.AddView(1, ViewportMetrics{1, 10, 10, 22, 0},
+                   [](bool added) { ASSERT_TRUE(added); });
+    engine.AddView(2, ViewportMetrics{1, 10, 10, 22, 0},
+                   [](bool added) { ASSERT_TRUE(added); });
   });
 
   native_latch.Wait();
@@ -368,8 +370,10 @@ TEST_F(EngineAnimatorTest, IgnoresOutOfFrameRenders) {
                                          std::move(animator));
 
   engine_context->EngineTaskSync([](Engine& engine) {
-    engine.AddView(1, ViewportMetrics{1, 10, 10, 22, 0});
-    engine.AddView(2, ViewportMetrics{1, 10, 10, 22, 0});
+    engine.AddView(1, ViewportMetrics{1, 10, 10, 22, 0},
+                   [](bool added) { ASSERT_TRUE(added); });
+    engine.AddView(2, ViewportMetrics{1, 10, 10, 22, 0},
+                   [](bool added) { ASSERT_TRUE(added); });
   });
 
   auto configuration = RunConfiguration::InferFromSettings(settings_);
@@ -444,7 +448,8 @@ TEST_F(EngineAnimatorTest, IgnoresDuplicateRenders) {
                                          std::move(animator));
 
   engine_context->EngineTaskSync([](Engine& engine) {
-    engine.AddView(kFlutterImplicitViewId, ViewportMetrics{1, 10, 10, 22, 0});
+    engine.AddView(kFlutterImplicitViewId, ViewportMetrics{1, 10, 10, 22, 0},
+                   [](bool added) { ASSERT_TRUE(added); });
   });
 
   auto configuration = RunConfiguration::InferFromSettings(settings_);
@@ -504,7 +509,8 @@ TEST_F(EngineAnimatorTest, AnimatorSubmitsImplicitViewBeforeDrawFrameEnds) {
                                          std::move(animator));
 
   engine_context->EngineTaskSync([](Engine& engine) {
-    engine.AddView(kFlutterImplicitViewId, ViewportMetrics{1.0, 10, 10, 1, 0});
+    engine.AddView(kFlutterImplicitViewId, ViewportMetrics{1.0, 10, 10, 1, 0},
+                   [](bool added) { ASSERT_TRUE(added); });
   });
 
   auto configuration = RunConfiguration::InferFromSettings(settings_);
@@ -568,7 +574,8 @@ TEST_F(EngineAnimatorTest, AnimatorSubmitWarmUpImplicitView) {
     engine.ScheduleFrame(true);
     // Add the implicit view so that the engine recognizes it and that its
     // metrics is not empty.
-    engine.AddView(kFlutterImplicitViewId, ViewportMetrics{1.0, 10, 10, 1, 0});
+    engine.AddView(kFlutterImplicitViewId, ViewportMetrics{1.0, 10, 10, 1, 0},
+                   [](bool added) { ASSERT_TRUE(added); });
   });
   continuation_ready_latch.Wait();
 
@@ -634,9 +641,12 @@ TEST_F(EngineAnimatorTest, AnimatorSubmitPartialViewsForWarmUp) {
     // Schedule a frame to make the animator create a continuation.
     engine.ScheduleFrame(true);
     // Add multiple views.
-    engine.AddView(0, ViewportMetrics{1, 10, 10, 22, 0});
-    engine.AddView(1, ViewportMetrics{1, 10, 10, 22, 0});
-    engine.AddView(2, ViewportMetrics{1, 10, 10, 22, 0});
+    engine.AddView(0, ViewportMetrics{1, 10, 10, 22, 0},
+                   [](bool added) { ASSERT_TRUE(added); });
+    engine.AddView(1, ViewportMetrics{1, 10, 10, 22, 0},
+                   [](bool added) { ASSERT_TRUE(added); });
+    engine.AddView(2, ViewportMetrics{1, 10, 10, 22, 0},
+                   [](bool added) { ASSERT_TRUE(added); });
   });
 
   continuation_ready_latch.Wait();
diff --git a/shell/common/shell.cc b/shell/common/shell.cc
@@ -754,7 +754,11 @@ bool Shell::Setup(std::unique_ptr<PlatformView> platform_view,
   weak_rasterizer_ = rasterizer_->GetWeakPtr();
   weak_platform_view_ = platform_view_->GetWeakPtr();
 
-  engine_->AddView(kFlutterImplicitViewId, ViewportMetrics{});
+  // Add the implicit view with empty metrics.
+  engine_->AddView(kFlutterImplicitViewId, ViewportMetrics{}, [](bool added) {
+    FML_DCHECK(added) << "Failed to add the implicit view";
+  });
+
   // Setup the time-consuming default font manager right after engine created.
   if (!settings_.prefetched_default_font_manager) {
     fml::TaskRunner::RunNowOrPostTask(task_runners_.GetUITaskRunner(),
@@ -2102,7 +2106,9 @@ bool Shell::OnServiceProtocolReloadAssetFonts(
   return true;
 }
 
-void Shell::AddView(int64_t view_id, const ViewportMetrics& viewport_metrics) {
+void Shell::AddView(int64_t view_id,
+                    const ViewportMetrics& viewport_metrics,
+                    AddViewCallback callback) {
   TRACE_EVENT0("flutter", "Shell::AddView");
   FML_DCHECK(is_set_up_);
   FML_DCHECK(task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread());
@@ -2112,10 +2118,11 @@ void Shell::AddView(int64_t view_id, const ViewportMetrics& viewport_metrics) {
 
   task_runners_.GetUITaskRunner()->PostTask([engine = engine_->GetWeakPtr(),  //
                                              viewport_metrics,                //
-                                             view_id                          //
+                                             view_id,                         //
+                                             callback = std::move(callback)   //
   ] {
     if (engine) {
-      engine->AddView(view_id, viewport_metrics);
+      engine->AddView(view_id, viewport_metrics, callback);
     }
   });
 }
diff --git a/shell/common/shell.h b/shell/common/shell.h
@@ -133,6 +133,7 @@ class Shell final : public PlatformView::Delegate,
       const std::shared_ptr<fml::SyncSwitch>& gpu_disabled_switch,
       impeller::RuntimeStageBackend runtime_stage_type)>
       EngineCreateCallback;
+  using AddViewCallback = std::function<void(bool added)>;
   using RemoveViewCallback = std::function<void(bool removed)>;
 
   //----------------------------------------------------------------------------
@@ -317,8 +318,12 @@ class Shell final : public PlatformView::Delegate,
   ///
   /// @param[in]  view_id           The view ID of the new view.
   /// @param[in]  viewport_metrics  The initial viewport metrics for the view.
+  /// @param[in]  callback          The callback that's invoked once the engine
+  ///                               has attempted to add the view.
   ///
-  void AddView(int64_t view_id, const ViewportMetrics& viewport_metrics);
+  void AddView(int64_t view_id,
+               const ViewportMetrics& viewport_metrics,
+               AddViewCallback callback);
 
   /// @brief  Deallocates resources for a non-implicit view.
   ///
diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc

Original file line number	Diff line number	Diff line change
`@@ -297,8 +297,10 @@ tonic::DartErrorHandleType Engine::GetUIIsolateLastError() {`
`297`	`297`	`return runtime_controller_->GetLastError();`
`298`	`298`	`}`
`299`	`299`
`300`		`-void Engine::AddView(int64_t view_id, const ViewportMetrics& view_metrics) {`
`301`		`- runtime_controller_->AddView(view_id, view_metrics);`
	`300`	`+void Engine::AddView(int64_t view_id,`
	`301`	`+ const ViewportMetrics& view_metrics,`
	`302`	`+ std::function<void(bool added)> callback) {`
	`303`	`+ runtime_controller_->AddView(view_id, view_metrics, std::move(callback));`
`302`	`304`	`}`
`303`	`305`
`304`	`306`	`bool Engine::RemoveView(int64_t view_id) {`