mobile: Replace direct JNIEnv access with JniHelper#30705
mobile: Replace direct JNIEnv access with JniHelper#30705RyanTheOptimist merged 5 commits intoenvoyproxy:mainfrom
Conversation
This PR replaces direct `JNIEnv` access, so that we can slowly migrate the code to use `JniHelper`. Follow-up PRs will migrate the code piecemeal to reduce the risk of breaking existing code. Signed-off-by: Fredy Wijaya <fredyw@google.com>
Signed-off-by: Fredy Wijaya <fredyw@google.com>
|
/retest |
Signed-off-by: Fredy Wijaya <fredyw@google.com>
|
/retest |
1 similar comment
|
/retest |
fredyw
requested review from
RyanTheOptimist,
abeyad and
alyssawilk
as code owners
|
/assign @abeyad |
RyanTheOptimist
left a comment
RyanTheOptimist
left a comment
There was a problem hiding this comment.
Very cool! One drive-by comment, and one question.
| envoy_data host = Envoy::Data::Utility::copyToBridgeData(hostname); | ||
| JNIEnv* env = Envoy::JNI::get_env(); | ||
| jstring java_host = Envoy::JNI::native_data_to_string(env, host); | ||
| Envoy::JNI::JniHelper jni_helper(Envoy::JNI::get_env()); |
There was a problem hiding this comment.
Would it make sense to have JniHelper call Envoy::JNI::get_env() in the constructor to simplify usage?
There was a problem hiding this comment.
Unfortunately the use of Envoy::JNI::get_env() is not consistent. Some will use it and some won't. I believe Envoy::JNI::get_env() is also very Android specific, which means it won't work when running on HotSpot VM (non Android). In the future PR, I plan on fixing this as well, which could possibly be using Envoy::JNI::get_env() everywhere (or removing it).
There was a problem hiding this comment.
Oh, I see. How confusing! Thanks for working on this.
| jclass jcls_AndroidNetworkLibrary = | ||
| Envoy::JNI::find_class("io.envoyproxy.envoymobile.utilities.AndroidNetworkLibrary"); | ||
| jmethodID jmid_isCleartextTrafficPermitted = env->GetStaticMethodID( | ||
| jmethodID jmid_isCleartextTrafficPermitted = jni_helper.getEnv()->GetStaticMethodID( |
There was a problem hiding this comment.
Your plan is to (incrementally) replace code like this with jni_helper.GetStaticMethodID(...), right?
There was a problem hiding this comment.
Yup. That's the plan. I will try to make small changes at a time, so it's easier to debug in case something breaks. Once everything has been fully migrated, I'll start removing JniHelper::getEnv().
Signed-off-by: Fredy Wijaya <fredyw@google.com>
|
/assign @RyanTheOptimist |
| envoy_data host = Envoy::Data::Utility::copyToBridgeData(hostname); | ||
| JNIEnv* env = Envoy::JNI::get_env(); | ||
| jstring java_host = Envoy::JNI::native_data_to_string(env, host); | ||
| Envoy::JNI::JniHelper jni_helper(Envoy::JNI::get_env()); |
There was a problem hiding this comment.
Oh, I see. How confusing! Thanks for working on this.
3 participants
This PR replaces direct
JNIEnvaccess, so that we can slowly migrate the code to useJniHelper. Follow-up PRs will migrate the code piecemeal to reduce the risk of breaking existing code.Risk Level: low (cosmetic changes)
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a