mobile: Part 2: Update JNI usages with JniHelper #30748
Conversation
This updates the use of `FindClass` with `JniHelper`. Signed-off-by: Fredy Wijaya <[email protected]>
Signed-off-by: Fredy Wijaya <[email protected]>
Signed-off-by: Fredy Wijaya <[email protected]>
Signed-off-by: Fredy Wijaya <[email protected]>
fredyw
requested review from
RyanTheOptimist,
alyssawilk and
abeyad
as code owners
|
/assign @abeyad @RyanTheOptimist |
| jmethodID jmid_hashMapPut = jni_helper.getMethodId( | ||
| jcls_hashMap, "put", "(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;"); | ||
| jcls_hashMap.get(), "put", "(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;"); |
There was a problem hiding this comment.
nit: not super important and not for this PR, but perhaps at some point we may want to consider adding the dereferencing operator (i.e. *) to LocalRefUniquePtr so we can use it instead of .get(). OTOH, .get() makes it clearer, so I'm indifferent either way, just thought I'd mention as food for thought.
There was a problem hiding this comment.
std::unique_ptr already implements operator *(), but it will dereference it whereas std::unique_ptr<>::get() will simply return the borrowed pointer.
There was a problem hiding this comment.
I'm slightly confused by this. It looks like the method signature for getMethodId() is:
jmethodID getMethodId(jclass clazz, const char* name, const char* signature);
The type of jcls_hashMap is LocalRefUniquePtr<jclass> so I would expect we would want to pass *jcls_hashMap into getMethodId since we want a jclass not jclass*. But clearly your code works so I must be misunderstanding. Can you explains?
There was a problem hiding this comment.
The JNI types are extremely confusing.
Here's the definition.
typedef _jclass *jclass;
LocalRefUniquePtr<jclass> foo = jni_helper.findClass("...");
jclass jc1 = foo.get(); // will return jclass
_jclass jc2 = *foo; // deference, will return _jclass instead of jclass
There was a problem hiding this comment.
Oh, ffs! That's crazy. Thanks, java :)
| jmethodID jmid_hashMapPut = jni_helper.getMethodId( | ||
| jcls_hashMap, "put", "(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;"); | ||
| jcls_hashMap.get(), "put", "(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;"); |
There was a problem hiding this comment.
I'm slightly confused by this. It looks like the method signature for getMethodId() is:
jmethodID getMethodId(jclass clazz, const char* name, const char* signature);
The type of jcls_hashMap is LocalRefUniquePtr<jclass> so I would expect we would want to pass *jcls_hashMap into getMethodId since we want a jclass not jclass*. But clearly your code works so I must be misunderstanding. Can you explains?
| @@ -76,7 +76,7 @@ jlongArray native_final_stream_intel_to_array(JniHelper& jni_helper, | |||
| */ | |||
| jobject native_map_to_map(JniHelper& jni_helper, envoy_map map); | |||
|
|
|||
| jstring native_data_to_string(JniHelper& jni_helper, envoy_data data); | |||
| LocalRefUniquePtr<jstring> native_data_to_string(JniHelper& jni_helper, envoy_data data); | |||
There was a problem hiding this comment.
Too late for this PR, but as we're moving away from a "C style" API here, should we consider moving these functions over to Envoy's camelCaseStyle?
There was a problem hiding this comment.
Yeah, that's also my plan once we have migrated everything to JniHelper.
This updates the following methods to
JniHelper.FindClassNewStringUTFRisk Level: low (refactor)
Testing: unit tests & integration tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: mobile