On Tue, 3 Nov 2020 21:20:49 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Johan Vos has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> process reviewer comments
>
> modules/javafx.graphics/src/main/native-glass/monocle/egl/eglBridge.c line 56:
>
>> 54: JNIEXPORT jlong JNICALL
>> Java_com_sun_glass_ui_monocle_EGLAcceleratedScreen_nEglChooseConfig
>> 55: (JNIEnv *env, jclass clazz, jlong eglDisplay, jintArray attribs) {
>> 56: jint *attrArray = (*env)->GetIntArrayElements(env, attribs,
>> JNI_FALSE);
>
> Should this check the return value in case it fails (returns NULL)?
I checked other places in the code, and calls to env->GetIntArrayElements are
typically not checked.
However, if something fails in that call, it must be something really bad that
happened. I added a fprintf to stderr, and throw an IllegalArgumentException in
the java code (as there is something wrong with the attribs)
> modules/javafx.graphics/src/main/native-glass/monocle/egl/egl_ext.h line 1:
>
>> 1: #include <jni.h>
>
> This needs a standard copyright header.
>
> Also should it have a guard to prevent including it more than once?
correct. fixed.
> modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/EGLAcceleratedScreen.java
> line 45:
>
>> 43: * implementation to get the best matching configuration.
>> 44: */
>> 45: EGLAcceleratedScreen(int[] attributes) throws GLException {
>
> I presume you intended to call the (newly added) default `super` method and
> not `super(int[])`?
Yes, this will (implicitly) call the super() method instead of the super(int[]
) method. The problem with the latter (and actually the reason for this
approach) is that the flow in the super(int[]) constructor is very specific to
specific use-cases, and would require subclasses for every EGL-system that does
not behave 100% similar to the specific usecase.
-------------
PR: https://git.openjdk.java.net/jfx/pull/343
