Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
testss
  • Loading branch information
dnfield committed Sep 18, 2020
commit 5bebd55cc9f6f5fd9a92c55e4f929db0eefda0c9
6 changes: 3 additions & 3 deletions shell/platform/android/flutter_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ void FlutterMain::Init(JNIEnv* env,
jstring appStoragePath,
jstring engineCachesPath,
jlong initTimeMillis,
jlong oldGenHeapSizeBytes) {
jint oldGenHeapSizeMegaBytes) {
std::vector<std::string> args;
args.push_back("flutter");
for (auto& arg : fml::jni::StringArrayToVector(env, jargs)) {
Expand Down Expand Up @@ -121,7 +121,7 @@ void FlutterMain::Init(JNIEnv* env,
make_mapping_callback(kPlatformStrongDill, kPlatformStrongDillSize);
#endif // FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG

settings.old_gen_heap_size = static_cast<int64_t>(oldGenHeapSizeBytes);
settings.old_gen_heap_size = static_cast<int64_t>(oldGenHeapSizeMegaBytes);

// Not thread safe. Will be removed when FlutterMain is refactored to no
// longer be a singleton.
Expand Down Expand Up @@ -169,7 +169,7 @@ bool FlutterMain::Register(JNIEnv* env) {
{
.name = "nativeInit",
.signature = "(Landroid/content/Context;[Ljava/lang/String;Ljava/"
"lang/String;Ljava/lang/String;Ljava/lang/String;J;J)V",
"lang/String;Ljava/lang/String;Ljava/lang/String;J;I)V",
.fnPtr = reinterpret_cast<void*>(&Init),
},
{
Expand Down
3 changes: 2 additions & 1 deletion shell/platform/android/flutter_main.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ class FlutterMain {
jstring kernelPath,
jstring appStoragePath,
jstring engineCachesPath,
jlong initTimeMillis);
jlong initTimeMillis,
jint oldGenHeapSizeMegaBytes);

void SetupObservatoryUriCallback(JNIEnv* env);

Expand Down
30 changes: 2 additions & 28 deletions shell/platform/android/io/flutter/FlutterInjector.java
Original file line number Diff line number Diff line change
Expand Up @@ -62,23 +62,12 @@ public static void reset() {
instance = null;
}

private FlutterInjector(boolean shouldLoadNative, @NonNull FlutterLoader flutterLoader) {
this.shouldLoadNative = shouldLoadNative;
private FlutterInjector(@NonNull FlutterLoader flutterLoader) {
this.flutterLoader = flutterLoader;
}

private boolean shouldLoadNative;
private FlutterLoader flutterLoader;

/**
* Returns whether the Flutter Android engine embedding should load the native C++ engine.
*
* <p>Useful for testing since JVM tests via Robolectric can't load native libraries.
*/
public boolean shouldLoadNative() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this is a breaking change. I still haven't merged the website PR for the last time this broke so I'll hold off until we resolve this PR. flutter/website#4561

return shouldLoadNative;
}

/** Returns the {@link FlutterLoader} instance to use for the Flutter Android engine embedding. */
@NonNull
public FlutterLoader flutterLoader() {
Expand All @@ -92,20 +81,6 @@ public FlutterLoader flutterLoader() {
* <p>Non-overriden values have reasonable defaults.
*/
public static final class Builder {

private boolean shouldLoadNative = true;
/**
* Sets whether the Flutter Android engine embedding should load the native C++ engine.
*
* <p>Useful for testing since JVM tests via Robolectric can't load native libraries.
*
* <p>Defaults to true.
*/
public Builder setShouldLoadNative(boolean shouldLoadNative) {
this.shouldLoadNative = shouldLoadNative;
return this;
}

private FlutterLoader flutterLoader;
/**
* Sets a {@link FlutterLoader} override.
Expand All @@ -130,8 +105,7 @@ private void fillDefaults() {
public FlutterInjector build() {
fillDefaults();

System.out.println("should load native is " + shouldLoadNative);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♀️ thanks

return new FlutterInjector(shouldLoadNative, flutterLoader);
return new FlutterInjector(flutterLoader);
}
}
}
64 changes: 55 additions & 9 deletions shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,58 @@
*/
@Keep
public class FlutterJNI {
public static class FlutterJNILoader {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making all the static private SG. But rather than making this class 2 tiered which is a bit more confusing, why not just let classes that want to mock FlutterJNI just inject a FlutterJNI mock directly so all methods can be mocked?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It limits the scope of what FlutterLoader gets, making it a little easier to quickly see exactly what it expects to use and how these things are related.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IOW, writing tests for this won't leave me wondering which of the many methods on FlutterJNI I'm supposed to mock.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jason might have the historical background. I believe we made all the methods in the FlutterJNI class grouped in one class instead of being sliced and diced per consumer based on least privilege principle for a reason (probably to have a single place to look at the interop schema). If so, I strongly believe we should have a single pattern in the codebase to be consistent. Either keep everything in one class (and remove statics which I support) or break it all up and parcel views of the JNI to its consumer based on what the consumer needs.

Otherwise, it becomes arbitrary why I could mock the FlutterLoader's JNI loader but on some JNI calls, it still crashes.

I think the cleanest way for your intended change is still to make all the statics normal instance methods for the public APIs and inject the whole FlutterJNI in tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual native methods are still on Flutter JNI, which still keeps it easier for us to write the JNI bindings.

I'm not really clear on why we would want to keep all of the Java facing methods on the same class. Flutter JNI will stick around as this sort of God-object, and everyone you give it to will be able to call all sorts of native methods that may not make sense.

I don't work on this code very much, so I'm not going to put up a huge fight about this. If, seeing what I'm saying, you still believe these methods should all be exposed on the Java side via the same one object, I can easily refactor this PR to do that - although if I were working on this more I would more strongly object :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked offline. I'll move out this part of the change here, we can track what to do with FlutterJNI overall in flutter/flutter#66131

I've also made the static methods public again, but marked them as deprecated with pointers to the instance methods.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping me when it's ready again

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be ready now :) The test failure is an infra flake.

/**
* Loads the libflutter.so library.
*
* <p>This must be called before any other native methods, and can be overridden by tests to
* avoid loading native libraries.
*/
public void loadLibrary() {
System.loadLibrary("flutter");
}

/**
* Prefetch the default font manager provided by SkFontMgr::RefDefault() which is a process-wide
* 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.
*/
public void prefetchDefaultFontManager() {
FlutterJNI.nativePrefetchDefaultFontManager();
}

/**
* Perform one time initialization of the Dart VM and Flutter engine.
*
* <p>This method must be called only once.
*
* @param context The application context.
* @param args Arguments to the Dart VM/Flutter engine.
* @param bundlePath For JIT runtimes, the path to the Dart kernel file for the application.
* @param appStoragePath The path to the application data directory.
* @param engineCachesPath The path to the application cache directory.
* @param initTimeMillis The time, in milliseconds, taken for initialization.
* @param oldGenHeapSizeMegaBytes The maximum size for the old gen heap size.
*/
public void nativeInit(
@NonNull Context context,
@NonNull String[] args,
@Nullable String bundlePath,
@NonNull String appStoragePath,
@NonNull String engineCachesPath,
long initTimeMillis,
int oldGenHeapSizeMegaBytes) {
FlutterJNI.nativeInit(
context,
args,
bundlePath,
appStoragePath,
engineCachesPath,
initTimeMillis,
oldGenHeapSizeMegaBytes);
}
}

private static final String TAG = "FlutterJNI";

@Nullable private static AsyncWaitForVsyncDelegate asyncWaitForVsyncDelegate;
Expand All @@ -104,22 +156,16 @@ public class FlutterJNI {
// This is set from native code via JNI.
@Nullable private static String observatoryUri;

// TODO(mattcarroll): add javadocs
public static native void nativeInit(
private static native void nativeInit(
@NonNull Context context,
@NonNull String[] args,
@Nullable String bundlePath,
@NonNull String appStoragePath,
@NonNull String engineCachesPath,
long initTimeMillis,
long oldGenHeapSizeMegaBytes);
int oldGenHeapSizeMegaBytes);

/**
* Prefetch the default font manager provided by SkFontMgr::RefDefault() which is a process-wide
* 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.
*/
public static native void nativePrefetchDefaultFontManager();
private static native void nativePrefetchDefaultFontManager();

private native boolean nativeGetIsSoftwareRenderingEnabled();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

package io.flutter.embedding.engine.loader;

import android.app.ActivityManager;
import android.content.Context;
import android.content.pm.PackageManager;
import android.content.res.AssetManager;
Expand All @@ -15,7 +16,6 @@
import androidx.annotation.NonNull;
import androidx.annotation.Nullable;
import io.flutter.BuildConfig;
import io.flutter.FlutterInjector;
import io.flutter.embedding.engine.FlutterJNI;
import io.flutter.util.PathUtils;
import io.flutter.view.VsyncWaiter;
Expand Down Expand Up @@ -60,10 +60,22 @@ public static FlutterLoader getInstance() {
return instance;
}

public FlutterLoader() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

this(null);
}

public FlutterLoader(@Nullable FlutterJNI.FlutterJNILoader flutterJNILoader) {
if (flutterJNILoader == null) {
flutterJNILoader = new FlutterJNI.FlutterJNILoader();
}
this.flutterJNILoader = flutterJNILoader;
}

private boolean initialized = false;
@Nullable private Settings settings;
private long initStartTimestampMillis;
private FlutterApplicationInfo flutterApplicationInfo;
private FlutterJNI.FlutterJNILoader flutterJNILoader;

private static class InitResult {
final String appStoragePath;
Expand Down Expand Up @@ -125,9 +137,7 @@ public void startInitialization(@NonNull Context applicationContext, @NonNull Se
public InitResult call() {
ResourceExtractor resourceExtractor = initResources(appContext);

if (FlutterInjector.instance().shouldLoadNative()) {
System.loadLibrary("flutter");
}
flutterJNILoader.loadLibrary();

// Prefetch the default font manager as soon as possible on a background thread.
// It helps to reduce time cost of engine setup that blocks the platform thread.
Expand All @@ -136,7 +146,7 @@ public InitResult call() {
new Runnable() {
@Override
public void run() {
FlutterJNI.nativePrefetchDefaultFontManager();
flutterJNILoader.prefetchDefaultFontManager();
}
});

Expand Down Expand Up @@ -225,21 +235,20 @@ public void ensureInitializationComplete(
shellArgs.add("--log-tag=" + settings.getLogTag());
}

ActivityManager activityManager = (ActivityManager) getSystemService(ACTIVITY_SERVICE);
long oldGenHeapSizeMegaBytes = activityManager.getLargeMemoryClass();
ActivityManager activityManager =
(ActivityManager) applicationContext.getSystemService(Context.ACTIVITY_SERVICE);
int oldGenHeapSizeMegaBytes = activityManager.getLargeMemoryClass();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we actually set largeHeap="true" in our AndroidManifest template right? Should we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's for the Java heap. This property is unaffected by that setting - it's what the Java heap would be set to if largeHeap was set to true.

I don't think we need largeHeap set to true, since we really don't do much with the Java heap.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah got it. Thanks


long initTimeMillis = SystemClock.uptimeMillis() - initStartTimestampMillis;

if (FlutterInjector.instance().shouldLoadNative()) {
FlutterJNI.nativeInit(
applicationContext,
shellArgs.toArray(new String[0]),
kernelPath,
result.appStoragePath,
result.engineCachesPath,
initTimeMillis,
oldGenHeapSizeMegaBytes);
}
flutterJNILoader.nativeInit(
applicationContext,
shellArgs.toArray(new String[0]),
kernelPath,
result.appStoragePath,
result.engineCachesPath,
initTimeMillis,
oldGenHeapSizeMegaBytes);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can oldGenHeapSizeMegaBytes be provided as a shell command line flag in shell/common/switches.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could but then we lose some static type checking here. Is there some reason you have in mind to do it that way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more consistent with how other engine settings are assigned and would avoid a proliferation of nativeInit arguments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


initialized = true;
} catch (Exception e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;

import io.flutter.embedding.engine.loader.FlutterLoader;
import org.junit.Before;
Expand Down Expand Up @@ -35,7 +34,6 @@ public void itHasSomeReasonableDefaults() {
// Implicitly builds when first accessed.
FlutterInjector injector = FlutterInjector.instance();
assertNotNull(injector.flutterLoader());
assertTrue(injector.shouldLoadNative());
}

@Test
Expand All @@ -44,7 +42,6 @@ public void canPartiallyOverride() {
new FlutterInjector.Builder().setFlutterLoader(mockFlutterLoader).build());
FlutterInjector injector = FlutterInjector.instance();
assertEquals(injector.flutterLoader(), mockFlutterLoader);
assertTrue(injector.shouldLoadNative());
}

@Test()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,15 @@ public void setUp() {
@Test
public void pluginsCanAccessFlutterAssetPaths() {
// Setup test.
FlutterInjector.setInstance(new FlutterInjector.Builder().setShouldLoadNative(false).build());
FlutterJNI.FlutterJNILoader mockFlutterJNILoader = mock(FlutterJNI.FlutterJNILoader.class);
FlutterInjector.setInstance(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, lg. Can you push some changes to flutter/website#4561 directly for the new pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new FlutterInjector.Builder()
.setFlutterLoader(new FlutterLoader(mockFlutterJNILoader))
.build());
FlutterJNI flutterJNI = mock(FlutterJNI.class);
when(flutterJNI.isAttached()).thenReturn(true);

FlutterLoader flutterLoader = new FlutterLoader();
FlutterLoader flutterLoader = new FlutterLoader(mockFlutterJNILoader);

// Execute behavior under test.
FlutterEngine flutterEngine =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,21 @@

package io.flutter.embedding.engine.loader;

import static android.os.Looper.getMainLooper;
import static junit.framework.TestCase.assertFalse;
import static junit.framework.TestCase.assertTrue;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyLong;
import static org.mockito.Mockito.anyString;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.robolectric.Shadows.shadowOf;

import io.flutter.FlutterInjector;
import org.junit.Before;
import android.app.ActivityManager;
import android.content.Context;
import io.flutter.embedding.engine.FlutterJNI;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.robolectric.RobolectricTestRunner;
Expand All @@ -18,10 +28,6 @@
@Config(manifest = Config.NONE)
@RunWith(RobolectricTestRunner.class)
public class FlutterLoaderTest {
@Before
public void setUp() {
FlutterInjector.reset();
}

@Test
public void itReportsUninitializedAfterCreating() {
Expand All @@ -31,13 +37,26 @@ public void itReportsUninitializedAfterCreating() {

@Test
public void itReportsInitializedAfterInitializing() {
FlutterInjector.setInstance(new FlutterInjector.Builder().setShouldLoadNative(false).build());
FlutterLoader flutterLoader = new FlutterLoader();
FlutterJNI.FlutterJNILoader mockFlutterJNILoader = mock(FlutterJNI.FlutterJNILoader.class);
FlutterLoader flutterLoader = new FlutterLoader(mockFlutterJNILoader);

assertFalse(flutterLoader.initialized());
flutterLoader.startInitialization(RuntimeEnvironment.application);
flutterLoader.ensureInitializationComplete(RuntimeEnvironment.application, null);
shadowOf(getMainLooper()).idle();
assertTrue(flutterLoader.initialized());
FlutterInjector.reset();
verify(mockFlutterJNILoader, times(1)).loadLibrary();

ActivityManager activityManager =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be a separate test? Since it seems unrelated to the test above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

(ActivityManager) RuntimeEnvironment.application.getSystemService(Context.ACTIVITY_SERVICE);
verify(mockFlutterJNILoader, times(1))
.nativeInit(
eq(RuntimeEnvironment.application),
any(),
anyString(),
anyString(),
anyString(),
anyLong(),
eq(activityManager.getLargeMemoryClass()));
}
}