From d28f3aff1b878ef66229bcc31a9cc514d52848bf Mon Sep 17 00:00:00 2001 From: Chris Bracken Date: Tue, 3 Jun 2025 22:44:27 -0700 Subject: [PATCH] Clean up EmbedderExternalViewEmbedder creation (#169962) When creating an EmbedderExternalViewController, we were returning a std::pair of the newly created instance and a bool, where the bool indicated whether creation was successful or not. When true, creation was unsuccessful and we should halt the engine launch. When false, creation was successful and we should continue the engine launch. Instead, we now return fml::StatusOr. When not StatusOr::ok(), we halt engine launch, otherwise we continue. No tests changed since this is a minor cleanup with no semantic change. ## Pre-launch Checklist - [X] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [X] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [X] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [X] I wondered why the hell that first baillout was returning {nullptr, false} too. - [X] I signed the [CLA]. - [X] I listed at least one issue that this PR fixes in the description above. - [X] I updated/added relevant documentation (doc comments with `///`). - [X] I added new tests to check the change I am making, or this PR is [test-exempt]. - [X] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [X] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --- .../shell/platform/embedder/embedder.cc | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/engine/src/flutter/shell/platform/embedder/embedder.cc b/engine/src/flutter/shell/platform/embedder/embedder.cc index aebf1fedb99..d849e7dca12 100644 --- a/engine/src/flutter/shell/platform/embedder/embedder.cc +++ b/engine/src/flutter/shell/platform/embedder/embedder.cc @@ -16,6 +16,7 @@ #include "flutter/fml/closure.h" #include "flutter/fml/make_copyable.h" #include "flutter/fml/native_library.h" +#include "flutter/fml/status_or.h" #include "flutter/fml/thread.h" #include "third_party/dart/runtime/bin/elf_loader.h" #include "third_party/dart/runtime/include/dart_native_api.h" @@ -1529,12 +1530,14 @@ CreateEmbedderRenderTarget( return render_target; } -static std::pair, - bool /* halt engine launch if true */> +/// Creates an EmbedderExternalViewEmbedder. +/// +/// When a non-OK status is returned, engine startup should be halted. +static fml::StatusOr> InferExternalViewEmbedderFromArgs(const FlutterCompositor* compositor, bool enable_impeller) { if (compositor == nullptr) { - return {nullptr, false}; + return std::unique_ptr{nullptr}; } auto c_create_callback = @@ -1550,15 +1553,15 @@ InferExternalViewEmbedderFromArgs(const FlutterCompositor* compositor, // Make sure the required callbacks are present if (!c_create_callback || !c_collect_callback) { - FML_LOG(ERROR) << "Required compositor callbacks absent."; - return {nullptr, true}; + return fml::Status(fml::StatusCode::kInvalidArgument, + "Required compositor callbacks absent."); } // Either the present view or the present layers callback must be provided. if ((!c_present_view_callback && !c_present_callback) || (c_present_view_callback && c_present_callback)) { - FML_LOG(ERROR) << "Either present_layers_callback or present_view_callback " - "must be provided but not both."; - return {nullptr, true}; + return fml::Status(fml::StatusCode::kInvalidArgument, + "Either present_layers_callback or " + "present_view_callback must be provided but not both."); } FlutterCompositor captured_compositor = *compositor; @@ -1601,10 +1604,9 @@ InferExternalViewEmbedderFromArgs(const FlutterCompositor* compositor, }; } - return {std::make_unique( - avoid_backing_store_cache, create_render_target_callback, - present_callback), - false}; + return std::make_unique( + avoid_backing_store_cache, create_render_target_callback, + present_callback); } // Translates embedder metrics to engine metrics, or returns a string on error. @@ -2258,7 +2260,8 @@ FlutterEngineResult FlutterEngineInitialize(size_t version, auto external_view_embedder_result = InferExternalViewEmbedderFromArgs( SAFE_ACCESS(args, compositor, nullptr), settings.enable_impeller); - if (external_view_embedder_result.second) { + if (!external_view_embedder_result.ok()) { + FML_LOG(ERROR) << external_view_embedder_result.status().message(); return LOG_EMBEDDER_ERROR(kInvalidArguments, "Compositor arguments were invalid."); } @@ -2276,7 +2279,8 @@ FlutterEngineResult FlutterEngineInitialize(size_t version, auto on_create_platform_view = InferPlatformViewCreationCallback( config, user_data, platform_dispatch_table, - std::move(external_view_embedder_result.first), settings.enable_impeller); + std::move(external_view_embedder_result.value()), + settings.enable_impeller); if (!on_create_platform_view) { return LOG_EMBEDDER_ERROR(