a round of comments and docs

Author: xster

shell/common/shell.h

@@ -218,6 +218,12 @@ class Shell final : public PlatformView::Delegate,
   ///             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 running state so RunEngine
+  ///             shouldn't be called again on the Shell. Once running, the
+  ///             second Shell is mostly independent from the original Shell
+  ///             and the original Shell doesn't need to keep running for the
+  ///             spawned Shell to keep functioning.
   /// @param[in]  run_configuration  A RunConfiguration used to run the Isolate
   ///             associated with this new Shell. It doesn't have to be the same
   ///             configuration as the current Shell but it needs to be in the

shell/common/shell_unittests.cc

@@ -2444,11 +2444,13 @@ TEST_F(ShellTest, Spawn) {
 
   fml::AutoResetWaitableEvent main_latch;
   std::string last_entry_point;
+  // Fulfill native function for the first Shell's entrypoint.
   AddNativeCallback(
       "SayHiFromFixturesAreFunctionalMain", CREATE_NATIVE_ENTRY([&](auto args) {
         last_entry_point = shell->GetEngine()->GetLastEntrypoint();
         main_latch.Signal();
       }));
+  // Fulfill native function for the second Shell's entrypoint.
   AddNativeCallback(
       // The Dart native function names aren't very consistent but this is just
       // the native function name of the second vm entrypoint in the fixture.
@@ -2457,6 +2459,7 @@ TEST_F(ShellTest, Spawn) {
   RunEngine(shell.get(), std::move(configuration));
   main_latch.Wait();
   ASSERT_TRUE(DartVMRef::IsInstanceRunning());
+  // Check first Shell ran the first entrypoint.
   ASSERT_EQ("fixturesAreFunctionalMain", last_entry_point);
 
   PostSync(
@@ -2478,6 +2481,7 @@ TEST_F(ShellTest, Spawn) {
         ASSERT_TRUE(ValidateShell(spawn.get()));
 
         PostSync(spawner->GetTaskRunners().GetUITaskRunner(), [&spawn] {
+          // Check second shell ran the second entrypoint.
           ASSERT_EQ("testCanLaunchSecondaryIsolate",
                     spawn->GetEngine()->GetLastEntrypoint());
         });

shell/platform/android/android_shell_holder.cc

@@ -9,7 +9,6 @@
 #include <pthread.h>
 #include <sys/resource.h>
 #include <sys/time.h>
-#include <cstddef>
 #include <memory>
 #include <optional>
 
@@ -150,7 +149,9 @@ AndroidShellHolder::AndroidShellHolder(
       shell_(std::move(shell)) {
   FML_DCHECK(jni_facade);
   FML_DCHECK(shell_);
+  FML_DCHECK(shell_->IsSetup());
   FML_DCHECK(platform_view_);
+  FML_DCHECK(thread_host_);
   is_valid_ = shell_ != nullptr;
 }
 
@@ -171,10 +172,16 @@ std::unique_ptr<AndroidShellHolder> AndroidShellHolder::Spawn(
     std::shared_ptr<PlatformViewAndroidJNI> jni_facade,
     std::string entrypoint,
     std::string libraryUrl) const {
-  FML_DCHECK(shell_) << "A new Shell can only be spawned if the current Shell "
-                        "is properly constructed";
+  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.
   fml::WeakPtr<PlatformViewAndroid> weak_platform_view;
+
+  // Take out the old AndroidContext to reuse inside the PlatformViewAndroid
+  // of the new Shell.
   PlatformViewAndroid* android_platform_view = platform_view_.get();
   // There's some indirection with platform_view_ being a weak pointer but
   // we just checked that the shell_ exists above and a valid shell is the
@@ -192,8 +199,7 @@ std::unique_ptr<AndroidShellHolder> AndroidShellHolder::Spawn(
             shell,                   // delegate
             shell.GetTaskRunners(),  // task runners
             jni_facade,              // JNI interop
-            shell.GetSettings()
-                .enable_software_rendering  // use software rendering
+            android_context          // Android context
         );
         weak_platform_view = platform_view_android->GetWeakPtr();
         shell.OnDisplayUpdates(DisplayUpdateType::kStartup,
@@ -205,9 +211,12 @@ std::unique_ptr<AndroidShellHolder> AndroidShellHolder::Spawn(
     return std::make_unique<Rasterizer>(shell);
   };
 
-  FML_DLOG(ERROR) << "Spawned run";
+  // TODO(xster): could be worth tracing this to investigate whether
+  // the IsolateConfiguration could be cached somewhere.
   auto config = BuildRunConfiguration(asset_manager_, entrypoint, libraryUrl);
   if (!config) {
+    // If the RunConfiguration was null, the kernel blob wasn't readable.
+    // Fail the whole thing.
     return nullptr;
   }
 

shell/platform/android/android_shell_holder.h

@@ -7,7 +7,7 @@
 
 #include <memory>
 
-#include "assets/asset_manager.h"
+#include "flutter/assets/asset_manager.h"
 #include "flutter/fml/macros.h"
 #include "flutter/fml/unique_fd.h"
 #include "flutter/lib/ui/window/viewport_metrics.h"
@@ -49,11 +49,16 @@ class AndroidShellHolder {
   ///             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>);
+                     fml::WeakPtr<PlatformViewAndroid> platform_view);
 
   ~AndroidShellHolder();
 
@@ -80,6 +85,9 @@ class AndroidShellHolder {
   ///             makes, the JNI instance holding this AndroidShellHolder should
   ///             be created first to supply the jni_facade callback.
   ///
+  /// @param[in]  jni_facade this argument should be the JNI callback facade of
+  ///             a new JNI instance meant to hold this AndroidShellHolder.
+  ///
   std::unique_ptr<AndroidShellHolder> Spawn(
       std::shared_ptr<PlatformViewAndroidJNI> jni_facade,
       std::string entrypoint,
@@ -99,6 +107,7 @@ class AndroidShellHolder {
   void UpdateAssetManager(fml::RefPtr<flutter::AssetManager> asset_manager);
 
   void NotifyLowMemoryWarning();
+  ;
 
  private:
   const flutter::Settings settings_;

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

@@ -370,9 +370,7 @@ private boolean isAttachedToJni() {
    *     AOT or snapshot.
    * @return a new {@link FlutterEngine}.
    */
-  // This method is package private because it's non-ideal. The method on FlutterEngine should
-  // ideally just create a second engine and a call to its DartExecutor should then run a
-  // DartEntrypoint.
+  @NonNull
   /*package*/ FlutterEngine spawn(@NonNull DartEntrypoint dartEntrypoint) {
     if (!isAttachedToJni()) {
       throw new IllegalStateException(

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

@@ -106,7 +106,7 @@ public class FlutterJNI {
    * <p>This must be called before any other native methods, and can be overridden by tests to avoid
    * loading native libraries.
    *
-   * <p>This method should only be called once.
+   * <p>This method should only be called once across all FlutterJNI instances.
    */
   public void loadLibrary() {
     if (loadLibraryCalled = true) {
@@ -124,7 +124,7 @@ public void loadLibrary() {
    * singleton owned by Skia. Note that, the first call to SkFontMgr::RefDefault() will take
    * noticeable time, but later calls will return a reference to the preexisting font manager.
    *
-   * <p>This method should only be called once.
+   * <p>This method should only be called once across all FlutterJNI instances.
    */
   public void prefetchDefaultFontManager() {
     if (prefetchDefaultFontManagerCalled = true) {
@@ -332,6 +332,7 @@ public long performNativeAttach(@NonNull FlutterJNI flutterJNI, boolean isBackgr
    * on the spawned FlutterJNI instance.
    */
   @UiThread
+  @NonNull
   public FlutterJNI spawn(
       @Nullable String entrypointFunctionName, @Nullable String pathToEntrypointFunction) {
     ensureRunningOnMainThread();

shell/platform/android/platform_view_android.cc

@@ -74,16 +74,19 @@ PlatformViewAndroid::PlatformViewAndroid(
         fml::MakeRefCounted<AndroidEnvironmentGL>());
 #endif  // SHELL_ENABLE_VULKAN
   }
-  FML_CHECK(android_context_ && android_context_->IsValid())
-      << "Could not create an Android context.";
-
-  surface_factory_ = std::make_shared<AndroidSurfaceFactoryImpl>(
-      *android_context_, jni_facade);
+  InitSurface();
+}
 
-  android_surface_ = surface_factory_->CreateSurface();
-  FML_CHECK(android_surface_ && android_surface_->IsValid())
-      << "Could not create an OpenGL, Vulkan or Software surface to setup "
-         "rendering.";
+PlatformViewAndroid::PlatformViewAndroid(
+    PlatformView::Delegate& delegate,
+    flutter::TaskRunners task_runners,
+    std::shared_ptr<PlatformViewAndroidJNI> jni_facade,
+    std::shared_ptr<flutter::AndroidContext> android_context)
+    : PlatformView(delegate, std::move(task_runners)),
+      jni_facade_(jni_facade),
+      android_context_(android_context),
+      platform_view_android_delegate_(jni_facade) {
+  InitSurface();
 }
 
 PlatformViewAndroid::PlatformViewAndroid(
@@ -96,6 +99,19 @@ PlatformViewAndroid::PlatformViewAndroid(
 
 PlatformViewAndroid::~PlatformViewAndroid() = default;
 
+void PlatformViewAndroid::InitSurface() {
+  FML_CHECK(android_context_ && android_context_->IsValid())
+      << "Could not create an Android context.";
+
+  surface_factory_ = std::make_shared<AndroidSurfaceFactoryImpl>(
+      *android_context_, jni_facade_);
+
+  android_surface_ = surface_factory_->CreateSurface();
+  FML_CHECK(android_surface_ && android_surface_->IsValid())
+      << "Could not create an OpenGL, Vulkan or Software surface to setup "
+         "rendering.";
+}
+
 void PlatformViewAndroid::NotifyCreated(
     fml::RefPtr<AndroidNativeWindow> native_window) {
   if (android_surface_) {

shell/platform/android/platform_view_android.h

@@ -53,6 +53,16 @@ class PlatformViewAndroid final : public PlatformView {
                       std::shared_ptr<PlatformViewAndroidJNI> jni_facade,
                       bool use_software_rendering);
 
+  //----------------------------------------------------------------------------
+  /// @brief      Creates a new PlatformViewAndroid but using an existing
+  ///             Android GPU context to create new surfaces. This maximizes
+  ///             resource sharing between 2 PlatformViewAndroids of 2 Shells.
+  ///
+  PlatformViewAndroid(PlatformView::Delegate& delegate,
+                      flutter::TaskRunners task_runners,
+                      std::shared_ptr<PlatformViewAndroidJNI> jni_facade,
+                      std::shared_ptr<flutter::AndroidContext> android_context);
+
   ~PlatformViewAndroid() override;
 
   void NotifyCreated(fml::RefPtr<AndroidNativeWindow> native_window);
@@ -159,6 +169,8 @@ class PlatformViewAndroid final : public PlatformView {
   // |PlatformView|
   void RequestDartDeferredLibrary(intptr_t loading_unit_id) override;
 
+  void InitSurface();
+
   void InstallFirstFrameCallback();
 
   void FireFirstFrameCallback();