Deduplicate plugin registration logic and make error logs visible - take 2 #25395
Conversation
shell/platform/android/io/flutter/embedding/android/FlutterActivity.java
Outdated
Show resolved
Hide resolved
shell/platform/android/io/flutter/embedding/android/FlutterFragmentActivity.java
Outdated
Show resolved
Hide resolved
| */ | ||
| @Override | ||
| public void configureFlutterEngine(@NonNull FlutterEngine flutterEngine) { | ||
| if (flutterFragment.isFlutterEngineInjected()) { |
There was a problem hiding this comment.
This is the key bit that is different from the previous PR. It seems like there was a long-standing bug where even if the engine says don't auto-register, the activity registers it anyway, which makes it hard to test in while avoiding annoying registration errors.
Comically, this change makes everything harder to test since only a non-injected engine has the auto-registering behavior but a non-injected engine is harder to prevent real JNI calls on. Add more plumbing to make a default constructed engine testable.
| } | ||
|
|
||
| @NonNull | ||
| public FlutterJNI.Factory flutterJNIFactory() { |
There was a problem hiding this comment.
nit: isn't it idiomatic in java to name this getFlutterJNIFactory()?
| // If the FlutterEngine was explicitly built and injected into this FlutterActivity, the | ||
| // builder should explicitly decide whether to automatically register plugins via the | ||
| // FlutterEngine's construction parameter or via the AndroidManifest metadata. | ||
| return; |
There was a problem hiding this comment.
Hmm, should we report this? Potentially with an exception or a return value?
There was a problem hiding this comment.
both the engine registration and the activity registration paths are called 99% of the time. We should just ignore the second call.
| * A factory for creating {@code FlutterJNI} instances. Useful for FlutterJNI injections during | ||
| * tests. | ||
| */ | ||
| public static class Factory { |
There was a problem hiding this comment.
nit: The pattern should have the Factory being an interface. It's not a huge deal here, but helps if anyone tries to add methods.
There was a problem hiding this comment.
it creates a bit more indirection to be able to instantiate one if it was an interface though
xster
added
the
waiting for tree to go green
5 participants
Was reverted in #25393
Fixes flutter/flutter#74646
Fixes flutter/flutter#79663