Updated background execution implementation for Android #5640

bkonyi · 2018-06-28T23:15:46Z

This change depends on changes made in PR #5539. Also, clang-format had some fun formatting a lot more than I had changed, so I apologize for the numerous white space changes!

Fixes the Android portion of #3671 and is required to fix #17566.

jason-simmons · 2018-06-29T00:55:31Z

shell/platform/android/android_shell_holder.cc

    blink::Settings settings,
-    fml::jni::JavaObjectWeakGlobalRef java_object)
+    fml::jni::JavaObjectWeakGlobalRef java_object,
+    bool is_background_view)


Why does a headless shell need to be designated as a background view?

Would it be sufficient to create a standard shell and just not attach a surface?

This flag determines whether or not a surface is created for the platform view and what number of task runners are created (see here)

jason-simmons · 2018-06-29T01:01:12Z

shell/platform/android/io/flutter/view/FlutterNativeView.java


-    public void runFromSource(final String assetsDirectory, final String main, final String packages) {
+    public void runFromBundle(String bundlePath, String snapshotOverride, String entrypoint,
+            String libraryUrl, boolean reuseRuntimeController, FlutterIsolateStartedEvent event) {


We should create an object that holds the parameters needed to launch the Dart runtime instead of adding more arguments here. Some of the arguments of runFromBundle() added from past experiments are already obsolete (snapshotOverride, reuseRuntimeController). I'd like to make it easier to evolve this interface.

@chinmaygarde had a proposal for refactoring the host APIs along these lines

I think that's a good idea. Would you happen to know if there's a doc for this proposal?

I've gone ahead and added FlutterRunArguments to hold all the parameters required for entering an isolate in a new shell. Added deprecation message for all other runFromBundle definitions that don't use this new argument object.

jason-simmons · 2018-06-29T01:08:42Z

shell/platform/android/io/flutter/view/FlutterNativeView.java

-    private static native long nativeAttach(FlutterNativeView view);
+    private void onStarted(boolean success) {
+        if (startedEvent == null) return;
+        startedEvent.onStarted(success);


What is this event used for? Can this be replaced by a message sent by the app on a platform channel at a time of the app's choosing?

This will be called after the engine invokes _startMainIsolate, and the app code may not be fully initialized when this callback happens.

This event is used to notify that the either the new shell has started running or there was an error starting the shell. Other than having this event, I don't believe that the plugin can determine whether or not the new shell was started successfully.

Good point about this being called after _startMainIsolate is invoked when the app still may not be fully initialized, but we could totally use a platform channel to notify the plugin when the background callback determines that initialization is complete.

s-bauer · 2018-07-16T11:14:44Z

Any update on this? It's quite important!

bkonyi · 2018-07-16T15:59:44Z

@s-bauer yes, this is being worked on. The iOS changes needed to land first since this PR depended on some of the engine and dart:ui changes, but since those went in Friday I expect this change should go in soon after the review process is complete. Thanks for your patience!

zanderso · 2018-07-17T21:28:15Z

shell/platform/android/android_shell_holder.cc

+  fxl::RefPtr<fml::TaskRunner> gpu_runner;
+  fxl::RefPtr<fml::TaskRunner> ui_runner;
+  fxl::RefPtr<fml::TaskRunner> io_runner;
+  if (!is_background_view) {


Think positive! Change to if (is_background_view) and swap if and else.

zanderso · 2018-07-17T21:45:18Z

tools/licenses/pubspec.lock

    version: "1.1.5"
 sdks:
-  dart: ">=1.21.0 <2.0.0"
+  dart: ">=1.21.0 <=2.0.0-edge.7f1f54edc2cbe5280217ad3c858dd3df515aa3eb"


That upper bound looks a bit strange.

Yeah, the license script likes to update that field to weird stuff. See here for what it is currently.

zanderso

lgtm, but also bug jason, chinamy, or chris for any more comments.

chinmaygarde

A few nits but I like the direction of this patch. Thanks!

chinmaygarde · 2018-07-19T21:10:40Z

shell/platform/android/platform_view_android.cc

         "rendering.";
 }

+PlatformViewAndroid::PlatformViewAndroid(


I would be more comfortable with this being a separate subclass of PlatformView. Though you have added a null checks to the surface on each call site, there are also other callbacks that make no sense in a background execution context. Stuff like the first frame callback and such.

We can iterate on this in a subsequent patch however. I don't think this is necessarily blocking.

That sounds like a plan!

chinmaygarde · 2018-07-19T21:20:16Z

shell/platform/android/android_shell_holder.cc

+                           "started and run.";
+          success = true;
        }
+        view->InvokeOnStartedCallback(success);


Attach a closure to the root_isolate_create_callback field of the settings object used to start this shell. This will fire the callback only if the isolate is correctly launched and also take care of re-firing the callback in case of cold-restart events (which the plugins also probably need to react to). You will lose the ability for the plugin to react to failed launches. But, then again, there isn't much it can do now either. So, we may have to devise a more robust error recovery/reporting mechanism. But till that time, lets use the existing hooks.

Be careful that the callback in the settings object is fire on the UI thread, you might have to post a task back to the platform thread to fire a message back to the platform.

On second thought, I don't think this is particularly useful if we have no way of notifying the plugin that we failed to start. I've gone ahead and removed this.

chinmaygarde · 2018-07-19T21:21:04Z

shell/platform/android/platform_view_android_jni.h


 void FlutterViewOnFirstFrame(JNIEnv* env, jobject obj);

+void FlutterViewOnStarted(JNIEnv* env, jobject obj, jboolean success);


"started" is a rather nebulous term. Can be make this more descriptive like OnRootIsolateDidStart or something?

bkonyi requested review from chinmaygarde, cbracken, zanderso and jason-simmons June 28, 2018 23:15

googlebot added the cla: yes label Jun 28, 2018

bkonyi mentioned this pull request Jun 28, 2018

Updated android_alarm_manager to work after engine refactor. flutter/plugins#642

Merged

jason-simmons reviewed Jun 29, 2018

View reviewed changes

bkonyi mentioned this pull request Jul 10, 2018

Flutter should provide an abstraction for background execution flutter/flutter#3671

Closed

bkonyi force-pushed the headless_plugin branch from 6f27b97 to 59f4c05 Compare July 11, 2018 19:37

bkonyi added 3 commits July 12, 2018 14:38

Updated background execution implementation for Android

1be3595

Reverted formatting on FlutterView.java

ca823d9

Updated to use DartCallbackCache to lookup callbacks

1e36a0d

bkonyi force-pushed the android_headless_plugins branch from 1d01869 to 1e36a0d Compare July 13, 2018 17:53

googlebot added cla: no and removed cla: yes labels Jul 13, 2018

flutter deleted a comment from googlebot Jul 13, 2018

googlebot added cla: yes and removed cla: no labels Jul 16, 2018

bkonyi added 5 commits July 16, 2018 10:57

Updated runFromBundle to take an args object; deprecated old signatures

67dc24a

whitespace

2dab42a

remove whitespace

f02d63a

Updated licenses

8cc0880

Reverted local changes to licenses.sh

fcc1c03

zoechi mentioned this pull request Jul 17, 2018

alarm_manager plugin stopped working (Could not launch engine in configuration) flutter/flutter#17566

Closed

zanderso reviewed Jul 17, 2018

View reviewed changes

Thinking positively

2449c41

zanderso approved these changes Jul 18, 2018

View reviewed changes

chinmaygarde reviewed Jul 19, 2018

View reviewed changes

bkonyi force-pushed the android_headless_plugins branch from 2449c41 to 3f88c42 Compare August 1, 2018 23:34

This comment has been minimized.

Sign in to view

googlebot added cla: no and removed cla: yes labels Aug 1, 2018

This comment has been minimized.

Sign in to view

bkonyi force-pushed the android_headless_plugins branch from 0f465bc to 2449c41 Compare August 2, 2018 00:24

This comment has been minimized.

Sign in to view

googlebot added cla: yes cla: no and removed cla: no cla: yes labels Aug 2, 2018

bkonyi force-pushed the android_headless_plugins branch from 352a2f2 to df599bb Compare August 2, 2018 00:35

Removed "onStarted" events + hooks

8868cdf

bkonyi force-pushed the android_headless_plugins branch from df599bb to 8868cdf Compare August 2, 2018 00:37

Updated licenses

109b34e

chinmaygarde approved these changes Aug 3, 2018

View reviewed changes

bkonyi merged commit 2ba3d6b into headless_plugin Aug 3, 2018

bkonyi deleted the android_headless_plugins branch August 3, 2018 23:51


		void FlutterViewOnFirstFrame(JNIEnv* env, jobject obj);

		void FlutterViewOnStarted(JNIEnv* env, jobject obj, jboolean success);

Updated background execution implementation for Android #5640

Updated background execution implementation for Android #5640

Uh oh!

Conversation

bkonyi commented Jun 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-bauer commented Jul 16, 2018

Uh oh!

bkonyi commented Jul 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Uh oh!