On Tue, 18 Apr 2023 02:08:03 GMT, Alexander Matveev <almat...@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.
Looks good overall, suggesting few minor changes.
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.
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`.
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)`
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.
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`
-------------
Changes requested by arapte (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1094#pullrequestreview-1418209160
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1188351969
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1188473239
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1188478660
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1188581142
PR Review Comment: https://git.openjdk.org/jfx/pull/1094#discussion_r1190643308
