-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
jni: introduce custom find_class method #2483
Conversation
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
Signed-off-by: Rafal Augustyniak <[email protected]>
/retest |
Signed-off-by: Rafal Augustyniak <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe document somewhere that this only gets set up for Android? if we tried to use this for the jvm-only targets the static CL would be null
* defined by the project. For finding classes of Java built in-types use | ||
* `env->FindClass(...)` method instead as it is lighter to use. | ||
* | ||
* @param class_name, the name of the class to find (i.e. "org.chromium.net.AndroidNetworkLibrary"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its / delimited, not . right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It requires .
- I added an example to avoid confusion since I realize that /
would be a more obvious choice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i see, you're calling loadClass and not findClass
nice consistency java
Signed-off-by: Rafal Augustyniak <[email protected]>
Good idea, updated. |
Signed-off-by: Rafal Augustyniak <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests can come when we actually start using this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for doing this.
/retest |
Description: Update the implementation of `Java_io_envoyproxy_envoymobile_engine_AndroidJniLibrary_initialize` so that it takes a `class_loader` argument which was introduced in #2483. It turns out that this method is implemented in two separate places - `android_jni_interface.cc` and and `android_test_jni_interface.cc` files - and my previous PR updated only the former. This is needed to make `find_class` method work in tests as tests depend on the implementation from `android_test_jni_interface.cc` file. Risk Level: None, additive change for test targets only. Testing: None, a follow up tests that depend on `find_class` method are being worked on. Docs Changes: N/A Release Notes: N/A Signed-off-by: Rafal Augustyniak <[email protected]>
Description: Update the implementation of `Java_io_envoyproxy_envoymobile_engine_AndroidJniLibrary_initialize` so that it takes a `class_loader` argument which was introduced in envoyproxy/envoy-mobile#2483. It turns out that this method is implemented in two separate places - `android_jni_interface.cc` and and `android_test_jni_interface.cc` files - and my previous PR updated only the former. This is needed to make `find_class` method work in tests as tests depend on the implementation from `android_test_jni_interface.cc` file. Risk Level: None, additive change for test targets only. Testing: None, a follow up tests that depend on `find_class` method are being worked on. Docs Changes: N/A Release Notes: N/A Signed-off-by: Rafal Augustyniak <[email protected]> Signed-off-by: JP Simard <[email protected]>
Description: The change is needed in order for JNI to be able to find classes which are defined in our repository (as opposed to built in Java types). Google documents in here touches on this issue. The implementation could be improved by adding some caching to it but I am inclined to keep it simple unless we find out that its performance is an issue. Needed for #2432.
Risk Level: None, not used anywhere as of now.
Testing: Manual
Docs Changes: N/A
Release Notes: N/A
Signed-off-by: Rafal Augustyniak [email protected]