On Thu, 30 Nov 2023 01:38:13 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> Please read [this >> document](https://gist.github.com/mstr2/9f46f92c98d3c86aa6a0b4224a9a6548) >> for an introduction to the Platform Preferences API, and how it interacts >> with the proposed style theme and stage appearance features. > > Michael Strauß has updated the pull request incrementally with one additional > commit since the last revision: > > initialize field with NULL I've finished my testing on all platforms. All looks good now. I'm most of the way through the code review -- just need to finish looking at the native Windows code and the test. I left a few comments about JNI error checking and a couple questions. modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.cpp line 37: > 35: env->CallObjectMethod(prefs, jMapPut, > 36: env->NewStringUTF(prefColorName), > 37: env->CallStaticObjectMethod( This sort of nesting of JNI function calls doesn't allow for checking for NULL (in the case of NewStringUTF) or a JNI Exception. We have open bugs to address these missing checks in existing code and I'd rather not add new code that will need to be similarly fixed. modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.cpp line 51: > 49: env->CallObjectMethod(preferences, jMapPut, > 50: env->NewStringUTF(name), > 51: env->NewStringUTF(value)); Check for pending exception after the call (and probably check the return value of `NewStringUTF` for NULL) modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.cpp line 68: > 66: jobject PlatformSupport::collectPreferences() const { > 67: jobject prefs = env->NewObject(jHashMapCls, jHashMapInit); > 68: if (EXCEPTION_OCCURED(env)) return NULL; You should also check for `prefs == NULL` (or `!prefs` as you prefer); modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.cpp line 70: > 68: if (EXCEPTION_OCCURED(env)) return NULL; > 69: > 70: GtkStyle* style = gtk_style_new(); Can `gtk_style_new` return NULL? modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.cpp line 119: > 117: > 118: if (!EXCEPTION_OCCURED(env)) { > 119: env->CallVoidMethod(application, > jApplicationNotifyPreferencesChanged, unmodifiablePreferences); I think you should call `EXCEPTION_OCCURED` after this call or at the end of the function in case there is a pending exception. modules/javafx.graphics/src/main/native-glass/mac/PlatformSupport.m line 225: > 223: > 224: if (unmodifiablePreferences != nil) { > 225: (*env)->CallVoidMethod( I think you should call `GLASS_CHECK_EXCEPTION` here or at the end of the method to clear any pending exception. modules/javafx.graphics/src/main/native-glass/mac/PlatformSupport.m line 243: > 241: (*env)->CallObjectMethod(env, preferences, jMapPutMethod, > 242: (*env)->NewStringUTF(env, key), > 243: value != nil ? (*env)->NewStringUTF(env, value) : nil); You should check for pending exceptions (`GLASS_CHECK_EXCEPTION`) after this call, and maybe check for `NewStringUTF` returning null. modules/javafx.graphics/src/main/native-glass/mac/PlatformSupport.m line 252: > 250: (*env)->CallObjectMethod(env, preferences, jMapPutMethod, > 251: (*env)->NewStringUTF(env, colorName), > 252: (*env)->CallStaticObjectMethod( Same comment as in the gtk code about nested calls to JNI method and checking for pending exceptions / NULL value from NewStringUTF. modules/javafx.graphics/src/main/native-glass/mac/PlatformSupport.m line 275: > 273: (double)[c alphaComponent]); > 274: > 275: (*env)->SetObjectArrayElement(env, res, i, fxcolor); You should check for pending exceptions after each of these call (each time through the loop), and maybe bail out if there is one. modules/javafx.graphics/src/main/native-glass/mac/PlatformSupport.m line 278: > 276: } > 277: > 278: (*env)->CallObjectMethod(env, preferences, jMapPutMethod, > (*env)->NewStringUTF(env, colorName), res); Unless this is returning directly to Java (i.e., is called from a JNI method that will return right away), you should check for pending exceptions / NULL value. modules/javafx.graphics/src/main/native-glass/win/GlassApplication.cpp line 174: > 172: lParam != NULL && wcscmp(LPCWSTR(lParam), > L"ImmersiveColorSet") == 0) && > 173: m_platformSupport.updatePreferences(m_grefThis)) { > 174: return 0; Do we need to fall through in this case? We used to do so, which is why I'm asking. modules/javafx.graphics/src/main/native-glass/win/GlassApplication.cpp line 189: > 187: return 0; > 188: } > 189: break; In the case of `WM_THEMECHANGED` we used to always return and not exit the switch statement. Could this cause any problems? ------------- PR Review: https://git.openjdk.org/jfx/pull/1014#pullrequestreview-1756304703 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1411381256 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1411392082 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1411382881 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1411383255 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1411388361 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1411404804 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1411405741 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1411406135 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1411412902 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1411410732 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1411415379 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1411416483
