On Tue, 9 May 2023 09:05:57 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> - Added missing exception checks for JNI calls.
>> - Improved JNI error checking by checking for both exception and return
>> value.
>> - Minor code clean up.
>
> modules/javafx.media/src/main/native/jfxmedia/Locator/Locator.cpp line 66:
>
>> 64: mid_toString = env->GetMethodID(klass, "getStringLocation",
>> "()Ljava/lang/String;");
>> 65: env->DeleteLocalRef(klass);
>> 66: if (javaEnv.clearException() || mid_toString == NULL)
>
> The variable name can be changed. It holds id to
> `Locator.getStringLocation()`, so the variable name does not reflect it
> correctly. As the PR is touching this code, it seems ok to change.
Fixed.
> modules/javafx.media/src/main/native/jfxmedia/jni/JavaInputStreamCallbacks.cpp
> line 68:
>
>> 66: createConnectionHolder = env->GetMethodID(klass,
>> "createConnectionHolder",
>> "()Lcom/sun/media/jfxmedia/locator/ConnectionHolder;");
>> 67: env->DeleteLocalRef(klass);
>> 68: if (javaEnv.reportException() || createConnectionHolder == 0)
>
> I would recommend to add as `(createConnectionHolder == NULL)`, similar to
> the below changes.
> And may be change previous instance and declaration of
> `createConnectionHolder`.
Fixed.
> modules/javafx.media/src/main/native/jfxmedia/jni/JavaInputStreamCallbacks.cpp
> line 73:
>
>> 71:
>> 72: jobject connectionHolder = env->CallObjectMethod(jLocator,
>> createConnectionHolder);
>> 73: if (javaEnv.reportException() || NULL == connectionHolder)
>
> May be enclose in parenthesis, like the below changes : `(NULL ==
> connectionHolder)`
Fixed.
> modules/javafx.media/src/main/native/jfxmedia/jni/JavaInputStreamCallbacks.cpp
> line 244:
>
>> 242: }
>> 243:
>> 244: javaEnv.reportException();
>
> modules/javafx.media/src/main/native/jfxmedia/jni/JavaInputStreamCallbacks.cpp
> : 240
>
> The `reportException()` method clears the exception and **prints** the
> exception message. So this change would result in behavior change. Is this
> change safe ?
> Similar change of removing `reportException()` can be done in `NeedBuffer()`
> method also.
Yes, it should be safe. I do not see any need to print exception, since it is
unlikely will happen. Also, these methods called many times to read data and
`clearException()` is more light weight. I updated `NeedBuffer()` to use
`clearException()`.
> modules/javafx.media/src/main/native/jfxmedia/jni/JavaMediaWarningListener.cpp
> line 49:
>
>> 47: jclass mediaUtilsClass =
>> pEnv->FindClass("com/sun/media/jfxmediaimpl/MediaUtils");
>> 48: if (!javaEnv.clearException() && mediaUtilsClass != NULL) {
>> 49: jmethodID errorMethodID =
>> pEnv->GetStaticMethodID(mediaUtilsClass,
>
> `errorMethodID` -> `nativeWarningMethodID`
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212352448
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212354698
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212354937
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212365506
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1212367015
