On Fri, 30 Oct 2020 15:09:49 GMT, Johan Vos <j...@openjdk.org> wrote:
> Allow the EGL functionality in monocle to leverage EGL-based systems. The > low-level specific details about how the EGL calls should be constructed are > left out, and a native interface (egl_ext.h) is created that can be mapped to > any low-level system. Looks OK as near as I can tell. I left a few questions along with a comment about a missing copyright header (that's the only thing that definitely needs to be 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[])`? modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/EGLPlatform.java line 37: > 35: String lib = System.getProperty("monocle.egl.lib"); > 36: if (lib != null) { > 37: LinuxSystem.getLinuxSystem().dlopen(lib, > LinuxSystem.RTLD_LAZY | LinuxSystem.RTLD_GLOBAL); Do you need to check the return value (and maybe throw an Exception)? modules/javafx.graphics/src/main/java/com/sun/glass/ui/monocle/EGLPlatform.java line 43: > 41: @Override > 42: public synchronized AcceleratedScreen getAcceleratedScreen(int[] > attributes) throws GLException { > 43: return new EGLAcceleratedScreen(attributes); Should this method cache the result so it returns a singleton (not sure, but the other `NativePlatform` subclasses, including `Dispman` do)? 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)? 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? ------------- PR: https://git.openjdk.java.net/jfx/pull/343