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
