On Fri, 15 May 2026 19:36:23 GMT, Alexander Matveev <[email protected]> wrote:
> - Removed/disabled coded which uses `gunichartables.h`. We do not use it and > do not need. > - Only dependency on this code is `GVariant` which uses `g_unichar_isprint` > to determine if character can be printed or needs to be escaped. This code is > not being used currently, but might be in the future. Assert which validates > `GVariant` string uses it to log value of `GVariant` in case if assert fails. > We might need it someday, so I keep it. Code was updated to use if ascii is > printable instead, since we do not use any unicode in our code. In worst case > we will escape non-ascii characters in debug log. > - I tested on all platforms with all supported formats. > > --------- > - [x] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Overall looks good. I left a few minor comments. It would be good to at least revert the unneeded whitespace changes inside `!GSTREAMER_LITE`. modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/goption.c line 669: > 667: return TRUE; > 668: } > 669: } Minor: I recommend reverting this indentation change. Since it is in an `#ifndef GSTREAMER_LITE` its only effect is as a potential future merge conflict. modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/goption.c line 674: > 672: #endif // GSTREAMER_LITE > 673: > 674: #ifndef GSTREAMER_LITE Minor: here is another place where you could remove the `endif` ... `ifndef` modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/goption.h line 375: > 373: #endif // GSTREAMER_LITE > 374: > 375: #ifndef GSTREAMER_LITE Minor, you can remove these two lines (no need to close the `ifndef` only to start a new one). modules/javafx.media/src/main/native/gstreamer/3rd_party/glib/glib/gunicode.h line 738: > 736: GLIB_AVAILABLE_IN_ALL > 737: gint g_unichar_digit_value (gunichar c) G_GNUC_CONST; > 738: Minor: I recommend reverting this removal of a blank line. Since it is in an `#ifndef GSTREAMER_LITE` its only effect is as a potential future merge conflict. ------------- PR Review: https://git.openjdk.org/jfx/pull/2168#pullrequestreview-4323113303 PR Review Comment: https://git.openjdk.org/jfx/pull/2168#discussion_r3269626039 PR Review Comment: https://git.openjdk.org/jfx/pull/2168#discussion_r3269648645 PR Review Comment: https://git.openjdk.org/jfx/pull/2168#discussion_r3269562348 PR Review Comment: https://git.openjdk.org/jfx/pull/2168#discussion_r3269594925
