Android embedding refactor PR1: JNI Extraction to FlutterJNI.java #7098

matthew-carroll · 2018-12-05T21:05:56Z

PR Context:
The Android embedding refactor is being decomposed, re-reviewed, and merged as experimental into master. This is the 1st such PR to begin decomposing and merging.

The code in each PR is extracted from a staging branch that has accumulated months of work up to this point: https://github.com/flutter/engine/compare/21008_rewrite-android-embedding-to-reduce-coupling

What this PR does:
This PR attempts to start with the smallest change possible in a core area that will allow for expansion in future PRs. Therefore, this PR introduces FlutterJNI.java as an extraction of JNI calls from FlutterView.java and FlutterNativeView.java. This extraction is a big part of what will allow development of a new embedding without further effecting the existing embedding.

This PR is one of the few PRs that alters existing code, but the modifications are relatively narrow in scope.

Reminder of approach:
Though somewhat dated, the approach towards this refactor is available to Google devs here:
https://docs.google.com/document/d/1TaDV1ZAwBeVBkBA2rtBbt1BRr96WSFqEv6MNAHdgmGY/edit?usp=sharing

matthew-carroll · 2018-12-05T21:07:56Z

Source tree is unfrozen

amirh

Reviewed everything but platform_view_android_jni.cc which I'm less comfortable with.
Overall looks good, left a few nits.

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

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

amirh · 2018-12-06T19:04:32Z

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

+
+  private FlutterRenderer.RenderSurface renderSurface;
+  private PlatformMessageHandler platformMessageHandler;
+  private final Set<EngineLifecycleListener> engineLifecycleListeners = new CopyOnWriteArraySet<>();


Are we expected to modify this from different threads?
If so would be nice to note in e.g addEngineLifecycleListener that this is thread safe.

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

shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java

amirh · 2018-12-06T19:34:11Z

shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java

+  }
+  //------ END TextureRegistry IMPLEMENTATION ----
+
+  // TODO(mattcarroll): describe the native behavior that this invokes


Same comment for all of this, I think there are good chances for these TODO comments to be forgotten, so I'd rather either solving the documentation TODOs right now, or linking Github issues for each of them.

I missed these comments when I added GitHub issue links to the TODOs. I will try to remember to address these TODO comments in the next PR that I'm preparing.

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

shell/platform/android/platform_view_android_jni.cc

amirh

Note that it would make it easier for me to followup on the review if you add new commits on top of the original one (this way I can more easily see the delta since I last reviewed).

shell/platform/android/io/flutter/embedding/engine/renderer/FlutterRenderer.java

amirh · 2018-12-07T22:37:14Z

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

+  }
+
+  public void addEngineLifecycleListener(@NonNull EngineLifecycleListener engineLifecycleListener) {
+    engineLifecycleListeners.add(engineLifecycleListener);


I'd avoid assumptions regarding what IDE is used (as you know some people just use text editors).
I'd think that in this case an assertion could potentially save some debugging time (if this ends throwing an NPE later you'll need to figure what set a null value here).

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

matthew-carroll · 2018-12-11T01:07:46Z

@amirh just so you know, it looks like when you comment on specific commits, I lose the ability to reply. For example, I don't see an option to reply to your latest 3 comments above this.

matthew-carroll · 2018-12-11T22:28:01Z

@chinmaygarde @jason-simmons @amirh @dnfield - I have a question on naming for you. I'm considering renaming FlutterEngine to FlutterApplication. I'm not sure if that's a great rename or not, but I thought it might make more sense, semantically. Do any of you have any opinions on that?

jason-simmons · 2018-12-11T22:35:37Z

Currently we use FlutterApplcation as the name of an android.app.Application subclass that initializes Flutter on startup. I'd prefer to keep that and use FlutterEngine for the class that wraps the Engine object in the Flutter runtime.

matthew-carroll · 2018-12-11T23:08:40Z

@jason-simmons Makes sense. I'll leave it as-is.

tvolkert · 2018-12-14T18:16:51Z

Can you sync and resolve conflicts? I tend to be gun-shy of reviewing code that may be out-of-date 😄

googlebot · 2018-12-19T03:34:13Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

googlebot · 2018-12-19T03:34:14Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

matthew-carroll · 2018-12-19T03:34:54Z

fix my bad author history on this PR

matthew-carroll · 2018-12-19T03:36:59Z

@tvolkert I've merged in the latest from master so you should be good to go

matthew-carroll · 2018-12-19T03:37:37Z

@sbaranov this is the PR I mentioned where I merged in your recent changes to the bundle paths. Please take a look when you get a chance to make sure that I didn't break your changes.

matthew-carroll · 2018-12-19T03:38:10Z

@amirh can you take a 2nd pass on this PR? I believe I've addressed all the comments that seemed directly actionable.

matthew-carroll · 2018-12-19T03:39:10Z

@jason-simmons would you mind taking a look through this PR, especially the native and JNI pieces? I think your LGTM will be important for the embedding refactor PRs.

matthew-carroll · 2018-12-19T03:39:45Z

@cbracken can you take a pass through this PR for an LGTM?

matthew-carroll · 2018-12-21T01:00:30Z

FYI - I'm going to do some force pushes as I attempt to correct my author history.

…to track its own native ID, removed comment from platform_view_android_jni.cc

…xture() private.

cbracken · 2018-12-21T01:21:14Z

@cbracken you're talking about a newline after the final brace?

I'm talking about an end-of-file newline.

Is there a reason behind such a policy? Does that enable something I'm unaware of?

That's how lines in a file are defined in the POSIX standard:
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_206

Lots of tools deal with this just fine, but best not to tempt fate.

As one example, C++ standards prior to C++11 require trailing newlines. From section 2.1/2:

If a source file that is not empty does not end in a new-line character, or ends in a new-line character immediately preceded by a backslash character, the behavior is undefined.

…ODOs.

googlebot · 2018-12-21T01:21:50Z

CLAs look good, thanks!

googlebot · 2018-12-21T01:21:51Z

CLAs look good, thanks!

…java (flutter/engine#7098)

googlebot added the cla: yes label Dec 5, 2018

matthew-carroll force-pushed the android_embedding_refactor_pr1_jni_extraction branch from 7c9b9b0 to 89e5be1 Compare December 5, 2018 21:09

matthew-carroll requested review from amirh, jonahwilliams, tvolkert, cbracken, mklim, jason-simmons and chinmaygarde December 5, 2018 21:17

matthew-carroll force-pushed the android_embedding_refactor_pr1_jni_extraction branch from 89e5be1 to aa32be8 Compare December 5, 2018 21:20

matthew-carroll requested a review from xster December 6, 2018 13:59

matthew-carroll force-pushed the android_embedding_refactor_pr1_jni_extraction branch from aa32be8 to d4c8006 Compare December 6, 2018 14:01

amirh reviewed Dec 6, 2018

View reviewed changes

amirh reviewed Dec 7, 2018

View reviewed changes

googlebot added cla: no and removed cla: yes labels Dec 19, 2018

matthew-carroll added 6 commits December 20, 2018 20:15

Android embedding refactor PR1: JNI Extraction to FlutterJNI.java

26b7e87

Added JavaDoc to FlutterEngine and FlutterJNI, refactored FlutterJNI …

2e2c657

…to track its own native ID, removed comment from platform_view_android_jni.cc

Removed comment saying we should make FlutterRenderer#createSurfaceTe…

22adcc7

…xture() private.

Added JavaDoc for FlutterJNI#setPlatformMessageHandler(...)

9ca9b80

Added JavaDoc for FlutterJNI#setRenderSurface(...)

1e28b72

Merge branch 'master' into android_embedding_refactor_pr1_jni_extraction

65b378b

matthew-carroll added 3 commits December 20, 2018 20:21

Specified @UiThread for public JNI methods, added GitHub issues for T…

785be70

…ODOs.

PR Updates: added and clarified JavaDocs.

691ad35

PR Updates: Added blank lines at end of files.

cb00a5e

matthew-carroll force-pushed the android_embedding_refactor_pr1_jni_extraction branch from dda02e2 to cb00a5e Compare December 21, 2018 01:21

googlebot added cla: yes and removed cla: no labels Dec 21, 2018

PR Updates: Added TODO for cbracken to expand javadoc.

42bc546

matthew-carroll merged commit 6b85ed3 into flutter:master Dec 21, 2018

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 21, 2018

6b85ed3 Android embedding refactor PR1: JNI Extraction to FlutterJNI.…

4f49347

…java (flutter/engine#7098)

engine-flutter-autoroll mentioned this pull request Dec 21, 2018

Roll engine 54a3577c0139..37af0cd7b8d5 (3 commits) flutter/flutter#25665

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 21, 2018

6b85ed3 Android embedding refactor PR1: JNI Extraction to FlutterJNI.…

8fc91ec

…java (flutter/engine#7098)

engine-flutter-autoroll mentioned this pull request Dec 21, 2018

Roll engine 54a3577c0139..367bb9bdf5d5 (4 commits) flutter/flutter#25668

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 21, 2018

6b85ed3 Android embedding refactor PR1: JNI Extraction to FlutterJNI.…

23ddbbd

…java (flutter/engine#7098)

engine-flutter-autoroll mentioned this pull request Dec 21, 2018

Roll engine 54a3577c0139..8532868d8bf1 (5 commits) flutter/flutter#25677

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 21, 2018

6b85ed3 Android embedding refactor PR1: JNI Extraction to FlutterJNI.…

44ccbb5

…java (flutter/engine#7098)

engine-flutter-autoroll mentioned this pull request Dec 21, 2018

Roll engine 54a3577c0139..a56467ed74d9 (6 commits) flutter/flutter#25680

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 22, 2018

6b85ed3 Android embedding refactor PR1: JNI Extraction to FlutterJNI.…

14f2971

…java (flutter/engine#7098)

engine-flutter-autoroll mentioned this pull request Dec 22, 2018

Roll engine 54a3577c0139..526b4bc35754 (7 commits) flutter/flutter#25684

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 22, 2018

6b85ed3 Android embedding refactor PR1: JNI Extraction to FlutterJNI.…

7e62627

…java (flutter/engine#7098)

engine-flutter-autoroll mentioned this pull request Dec 22, 2018

Roll engine 54a3577c0139..215ca1560088 (8 commits) flutter/flutter#25686

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android embedding refactor PR1: JNI Extraction to FlutterJNI.java #7098

Android embedding refactor PR1: JNI Extraction to FlutterJNI.java #7098

matthew-carroll commented Dec 5, 2018 •

edited

Loading

matthew-carroll commented Dec 5, 2018 •

edited

Loading

amirh left a comment

amirh Dec 6, 2018

amirh Dec 6, 2018

amirh Dec 21, 2018

matthew-carroll Dec 22, 2018

amirh left a comment

amirh Dec 7, 2018

matthew-carroll commented Dec 11, 2018

matthew-carroll commented Dec 11, 2018

jason-simmons commented Dec 11, 2018

matthew-carroll commented Dec 11, 2018

tvolkert commented Dec 14, 2018

googlebot commented Dec 19, 2018

googlebot commented Dec 19, 2018

matthew-carroll commented Dec 19, 2018 •

edited

Loading

matthew-carroll commented Dec 19, 2018

matthew-carroll commented Dec 19, 2018

matthew-carroll commented Dec 19, 2018

matthew-carroll commented Dec 19, 2018

matthew-carroll commented Dec 19, 2018

matthew-carroll commented Dec 21, 2018

cbracken commented Dec 21, 2018 •

edited

Loading

googlebot commented Dec 21, 2018

googlebot commented Dec 21, 2018

Android embedding refactor PR1: JNI Extraction to FlutterJNI.java #7098

Android embedding refactor PR1: JNI Extraction to FlutterJNI.java #7098

Conversation

matthew-carroll commented Dec 5, 2018 • edited Loading

matthew-carroll commented Dec 5, 2018 • edited Loading

amirh left a comment

Choose a reason for hiding this comment

amirh Dec 6, 2018

Choose a reason for hiding this comment

amirh Dec 6, 2018

Choose a reason for hiding this comment

amirh Dec 21, 2018

Choose a reason for hiding this comment

matthew-carroll Dec 22, 2018

Choose a reason for hiding this comment

amirh left a comment

Choose a reason for hiding this comment

amirh Dec 7, 2018

Choose a reason for hiding this comment

matthew-carroll commented Dec 11, 2018

matthew-carroll commented Dec 11, 2018

jason-simmons commented Dec 11, 2018

matthew-carroll commented Dec 11, 2018

tvolkert commented Dec 14, 2018

googlebot commented Dec 19, 2018

googlebot commented Dec 19, 2018

matthew-carroll commented Dec 19, 2018 • edited Loading

matthew-carroll commented Dec 19, 2018

matthew-carroll commented Dec 19, 2018

matthew-carroll commented Dec 19, 2018

matthew-carroll commented Dec 19, 2018

matthew-carroll commented Dec 19, 2018

matthew-carroll commented Dec 21, 2018

cbracken commented Dec 21, 2018 • edited Loading

googlebot commented Dec 21, 2018

googlebot commented Dec 21, 2018

matthew-carroll commented Dec 5, 2018 •

edited

Loading

matthew-carroll commented Dec 5, 2018 •

edited

Loading

matthew-carroll commented Dec 19, 2018 •

edited

Loading

cbracken commented Dec 21, 2018 •

edited

Loading