RFR: 8301302: Platform preferences API [v34]

[Kevin Rushforth]

On Thu, 30 Nov 2023 01:38:13 GMT, Michael Strauß <mstrauss at 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