reviews

Author: xster

shell/platform/android/android_shell_holder.cc

@@ -71,8 +71,8 @@ AndroidShellHolder::AndroidShellHolder(
           );
         }
         weak_platform_view = platform_view_android->GetWeakPtr();
-        shell.OnDisplayUpdates(DisplayUpdateType::kStartup,
-                               {Display(jni_facade->GetDisplayRefreshRate())});
+        auto display = Display(jni_facade->GetDisplayRefreshRate());
+        shell.OnDisplayUpdates(DisplayUpdateType::kStartup, {display});
         return platform_view_android;
       };
 
@@ -170,14 +170,17 @@ const flutter::Settings& AndroidShellHolder::GetSettings() const {
 
 std::unique_ptr<AndroidShellHolder> AndroidShellHolder::Spawn(
     std::shared_ptr<PlatformViewAndroidJNI> jni_facade,
-    std::string entrypoint,
-    std::string libraryUrl) const {
+    const std::string& entrypoint,
+    const std::string& libraryUrl) const {
   FML_DCHECK(shell_ && shell_->IsSetup())
       << "A new Shell can only be spawned "
          "if the current Shell is properly constructed";
 
   // Pull out the new PlatformViewAndroid from the new Shell to feed to it to
   // the new AndroidShellHolder.
+  //
+  // It's a weak pointer because it's owned by the Shell (which we're also)
+  // making below. And the AndroidShellHolder then owns the Shell.
   fml::WeakPtr<PlatformViewAndroid> weak_platform_view;
 
   // Take out the old AndroidContext to reuse inside the PlatformViewAndroid
@@ -202,8 +205,8 @@ std::unique_ptr<AndroidShellHolder> AndroidShellHolder::Spawn(
             android_context          // Android context
         );
         weak_platform_view = platform_view_android->GetWeakPtr();
-        shell.OnDisplayUpdates(DisplayUpdateType::kStartup,
-                               {Display(jni_facade->GetDisplayRefreshRate())});
+        auto display = Display(jni_facade->GetDisplayRefreshRate());
+        shell.OnDisplayUpdates(DisplayUpdateType::kStartup, {display});
         return platform_view_android;
       };
 
@@ -223,14 +226,14 @@ std::unique_ptr<AndroidShellHolder> AndroidShellHolder::Spawn(
   std::unique_ptr<flutter::Shell> shell = shell_->Spawn(
       std::move(config.value()), on_create_platform_view, on_create_rasterizer);
 
-  return std::make_unique<AndroidShellHolder>(GetSettings(), jni_facade,
-                                              thread_host_, std::move(shell),
-                                              weak_platform_view);
+  return std::unique_ptr<AndroidShellHolder>(
+      new AndroidShellHolder(GetSettings(), jni_facade, thread_host_,
+                             std::move(shell), weak_platform_view));
 }
 
 void AndroidShellHolder::Launch(std::shared_ptr<AssetManager> asset_manager,
-                                std::string entrypoint,
-                                std::string libraryUrl) {
+                                const std::string& entrypoint,
+                                const std::string& libraryUrl) {
   if (!IsValid()) {
     return;
   }
@@ -264,8 +267,8 @@ void AndroidShellHolder::NotifyLowMemoryWarning() {
 
 std::optional<RunConfiguration> AndroidShellHolder::BuildRunConfiguration(
     std::shared_ptr<flutter::AssetManager> asset_manager,
-    std::string entrypoint,
-    std::string libraryUrl) const {
+    const std::string& entrypoint,
+    const std::string& libraryUrl) const {
   std::unique_ptr<IsolateConfiguration> isolate_configuration;
   if (flutter::DartVM::IsRunningPrecompiledCode()) {
     isolate_configuration = IsolateConfiguration::CreateForAppSnapshot();

shell/platform/android/android_shell_holder.h

@@ -23,12 +23,12 @@ namespace flutter {
 //----------------------------------------------------------------------------
 /// @brief      This is the Android owner of the core engine Shell.
 ///
-///             This is the top orchestrator class on the C++ side for the
+/// @details    This is the top orchestrator class on the C++ side for the
 ///             Android embedding. It corresponds to a FlutterEngine on the
-///             Java side. This class is in C++ because the core Shell is in
-///             C++ and an Android specific orchestrator needs to exist to
-///             compose it with other Android specific components such as
-///             the PlatformViewAndroid. This composition of many to one
+///             Java side. This class is in C++ because the Shell is in
+///             C++ and an Android orchestrator needs to exist to
+///             compose it with other Android specific C++ components such as
+///             the PlatformViewAndroid. This composition of many-to-one
 ///             C++ components would be difficult to do through JNI whereas
 ///             a FlutterEngine and AndroidShellHolder has a 1:1 relationship.
 ///
@@ -43,23 +43,6 @@ class AndroidShellHolder {
                      std::shared_ptr<PlatformViewAndroidJNI> jni_facade,
                      bool is_background_view);
 
-  //----------------------------------------------------------------------------
-  /// @brief      Constructor with its components injected.
-  ///
-  ///             This is similar to the standard constructor, except its
-  ///             members were constructed elsewhere and injected.
-  ///
-  ///             All injected components must be non-null and valid.
-  ///
-  ///             Used when constructing the Shell from the inside out when
-  ///             spawning from an existing Shell.
-  ///
-  AndroidShellHolder(flutter::Settings settings,
-                     std::shared_ptr<PlatformViewAndroidJNI> jni_facade,
-                     std::shared_ptr<ThreadHost> thread_host,
-                     std::unique_ptr<Shell> shell,
-                     fml::WeakPtr<PlatformViewAndroid> platform_view);
-
   ~AndroidShellHolder();
 
   bool IsValid() const;
@@ -68,13 +51,15 @@ class AndroidShellHolder {
   /// @brief      This is a factory for a derived AndroidShellHolder from an
   ///             existing AndroidShellHolder.
   ///
-  ///             The new and existing AndroidShellHolder and underlying
-  ///             Shells Creates one Shell from another Shell where the created
+  /// @details    Creates one Shell from another Shell where the created
   ///             Shell takes the opportunity to share any internal components
   ///             it can. This results is a Shell that has a smaller startup
   ///             time cost and a smaller memory footprint than an Shell created
   ///             with a Create function.
   ///
+  ///             The new Shell is returned in a new AndroidShellHolder
+  ///             instance.
+  ///
   ///             The new Shell's flutter::Settings cannot be changed from that
   ///             of the initial Shell. The RunConfiguration subcomponent can
   ///             be changed however in the spawned Shell to run a different
@@ -88,14 +73,17 @@ class AndroidShellHolder {
   /// @param[in]  jni_facade this argument should be the JNI callback facade of
   ///             a new JNI instance meant to hold this AndroidShellHolder.
   ///
+  /// @returns    A new AndroidShellHolder containing a new Shell. Returns
+  ///             nullptr when a new Shell can't be created.
+  ///
   std::unique_ptr<AndroidShellHolder> Spawn(
       std::shared_ptr<PlatformViewAndroidJNI> jni_facade,
-      std::string entrypoint,
-      std::string libraryUrl) const;
+      const std::string& entrypoint,
+      const std::string& libraryUrl) const;
 
   void Launch(std::shared_ptr<AssetManager> asset_manager,
-              std::string entrypoint,
-              std::string libraryUrl);
+              const std::string& entrypoint,
+              const std::string& libraryUrl);
 
   const flutter::Settings& GetSettings() const;
 
@@ -118,11 +106,27 @@ class AndroidShellHolder {
   uint64_t next_pointer_flow_id_ = 0;
   std::shared_ptr<AssetManager> asset_manager_;
 
+  //----------------------------------------------------------------------------
+  /// @brief      Constructor with its components injected.
+  ///
+  /// @details    This is similar to the standard constructor, except its
+  ///             members were constructed elsewhere and injected.
+  ///
+  ///             All injected components must be non-null and valid.
+  ///
+  ///             Used when constructing the Shell from the inside out when
+  ///             spawning from an existing Shell.
+  ///
+  AndroidShellHolder(flutter::Settings settings,
+                     std::shared_ptr<PlatformViewAndroidJNI> jni_facade,
+                     std::shared_ptr<ThreadHost> thread_host,
+                     std::unique_ptr<Shell> shell,
+                     fml::WeakPtr<PlatformViewAndroid> platform_view);
   static void ThreadDestructCallback(void* value);
   std::optional<RunConfiguration> BuildRunConfiguration(
       std::shared_ptr<flutter::AssetManager> asset_manager,
-      std::string entrypoint,
-      std::string libraryUrl) const;
+      const std::string& entrypoint,
+      const std::string& libraryUrl) const;
 
   FML_DISALLOW_COPY_AND_ASSIGN(AndroidShellHolder);
 };

shell/platform/android/io/flutter/embedding/engine/FlutterEngine.java

@@ -76,7 +76,6 @@
 public class FlutterEngine {
   private static final String TAG = "FlutterEngine";
 
-  @NonNull private final Context context;
   @NonNull private final FlutterJNI flutterJNI;
   @NonNull private final FlutterRenderer renderer;
   @NonNull private final DartExecutor dartExecutor;
@@ -118,7 +117,7 @@ public void onPreEngineRestart() {
         }
 
         @Override
-        public void onEngineDestroy() {
+        public void onEngineWillDestroy() {
           // This inner implementation doesn't do anything since FlutterEngine sent this
           // notification in the first place. It's meant for external listeners.
         }
@@ -277,7 +276,6 @@ public FlutterEngine(
       @Nullable String[] dartVmArgs,
       boolean automaticallyRegisterPlugins,
       boolean waitForRestorationData) {
-    this.context = context;
     AssetManager assetManager;
     try {
       assetManager = context.createPackageContext(context.getPackageName(), 0).getAssets();
@@ -365,13 +363,17 @@ private boolean isAttachedToJni() {
    * Create a second {@link FlutterEngine} based on this current one by sharing as much resources
    * together as possible to minimize startup latency and memory cost.
    *
+   * @param context is a Context used to create the {@link FlutterEngine}. Could be the same Context
+   *     as the current engine or a different one. Generally, only an application Context is needed
+   *     for the {@link FlutterEngine} and its dependencies.
    * @param dartEntrypoint specifies the {@link DartEntrypoint} the new engine should run. It
    *     doesn't need to be the same entrypoint as the current engine but must be built in the same
    *     AOT or snapshot.
    * @return a new {@link FlutterEngine}.
    */
   @NonNull
-  /*package*/ FlutterEngine spawn(@NonNull DartEntrypoint dartEntrypoint) {
+  /*package*/ FlutterEngine spawn(
+      @NonNull Context context, @NonNull DartEntrypoint dartEntrypoint) {
     if (!isAttachedToJni()) {
       throw new IllegalStateException(
           "Spawn can only be called on a fully constructed FlutterEngine");
@@ -380,7 +382,11 @@ private boolean isAttachedToJni() {
     FlutterJNI newFlutterJNI =
         flutterJNI.spawn(
             dartEntrypoint.dartEntrypointFunctionName, dartEntrypoint.dartEntrypointLibrary);
-    return new FlutterEngine(context, null, newFlutterJNI);
+    return new FlutterEngine(
+        context, // Context.
+        null, // FlutterLoader. A null value passed here causes the constructor to get it from the
+        // FlutterInjector.
+        newFlutterJNI); // FlutterJNI.
   }
 
   /**
@@ -422,7 +428,7 @@ private void registerPlugins() {
   public void destroy() {
     Log.v(TAG, "Destroying.");
     for (EngineLifecycleListener listener : engineLifecycleListeners) {
-      listener.onEngineDestroy();
+      listener.onEngineWillDestroy();
     }
     // The order that these things are destroyed is important.
     pluginRegistry.destroy();
@@ -614,6 +620,6 @@ public interface EngineLifecycleListener {
      *
      * <p>For the duration of the call, the Flutter engine is still valid.
      */
-    void onEngineDestroy();
+    void onEngineWillDestroy();
   }
 }

shell/platform/android/io/flutter/embedding/engine/FlutterEngineGroup.java

@@ -16,8 +16,8 @@
  * This class is experimental. Please do not ship production code using it.
  *
  * <p>Represents a collection of {@link io.flutter.embedding.engine.FlutterEngine}s who share
- * resources among each other to allow them to be created faster and with less memory than calling
- * the {@link io.flutter.embedding.engine.FlutterEngine}'s constructor multiple times.
+ * resources to allow them to be created faster and with less memory than calling the {@link
+ * io.flutter.embedding.engine.FlutterEngine}'s constructor multiple times.
  *
  * <p>When creating or recreating the first {@link io.flutter.embedding.engine.FlutterEngine} in the
  * FlutterEngineGroup, the behavior is the same as creating a {@link
@@ -31,19 +31,18 @@
  */
 public class FlutterEngineGroup {
 
-  public FlutterEngineGroup(@NonNull Context context) {
-    this.context = context;
-  }
-
-  private final Context context;
   /* package */ @VisibleForTesting final List<FlutterEngine> activeEngines = new ArrayList<>();
 
-  public FlutterEngine createAndRunDefaultEngine() {
-    return createAndRunEngine(null);
+  public FlutterEngine createAndRunDefaultEngine(@NonNull Context context) {
+    return createAndRunEngine(context, null);
   }
 
-  public FlutterEngine createAndRunEngine(@Nullable DartEntrypoint dartEntrypoint) {
+  public FlutterEngine createAndRunEngine(
+      @NonNull Context context, @Nullable DartEntrypoint dartEntrypoint) {
     FlutterEngine engine = null;
+    // This is done up here because an engine needs to be created first in order to be able to use
+    // DartEntrypoint.createDefault. The engine creation initializes the FlutterLoader so
+    // DartEntrypoint known where to find the assets for the AOT or kernel code.
     if (activeEngines.size() == 0) {
       engine = createEngine(context);
     }
@@ -55,7 +54,7 @@ public FlutterEngine createAndRunEngine(@Nullable DartEntrypoint dartEntrypoint)
     if (activeEngines.size() == 0) {
       engine.getDartExecutor().executeDartEntrypoint(dartEntrypoint);
     } else {
-      engine = activeEngines.get(0).spawn(dartEntrypoint);
+      engine = activeEngines.get(0).spawn(context, dartEntrypoint);
     }
 
     activeEngines.add(engine);
@@ -70,7 +69,7 @@ public void onPreEngineRestart() {
           }
 
           @Override
-          public void onEngineDestroy() {
+          public void onEngineWillDestroy() {
             activeEngines.remove(engineToCleanUpOnDestroy);
           }
         });