On Mon, 4 Dec 2023 23:55:33 GMT, Michael Strauß <[email protected]> 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: > > check return value of JNI functions I've finished reviewing the native code. I left a few more comments and questions, mainly in RoActivationSupport. modules/javafx.graphics/src/main/native-glass/mac/GlassMacros.h line 178: > 176: > 177: #define GLASS_CHECK_NONNULL_EXCEPTIONALLY_RETURN(ENV, EXPR) > \ > 178: GLASS_CHECK_EXCEPTION(ENV); > \ Should this call `GLASS_CHECK_EXCEPTIONALLY_RETURN` instead? It doesn't matter for the current places where it is used, but it might be more consistent. modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp line 159: > 157: memset(wstr, 0, wstr_len * sizeof(WCHAR)); > 158: MultiByteToWideChar(CP_UTF8, 0, str, -1, wstr, wstr_len); > 159: WindowsCreateString(wstr, wstr_len - 1, &hstr_); This could use some error checking. At a minimum, `wstr` should be checked for null and skip the rest if it is. I think you also should check `wstr_len == 0`, since it is will return 0 on error. You might also consider whether you need to check the return value of `WindowsCreateString`. You would need to make sure `hstr_ = nullptr` if there are any errors. modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp line 165: > 163: hstring::~hstring() > 164: { > 165: WindowsDeleteString(hstr_); After adding the check for a failure to allocate the string, a null check is needed here (unless `WindowsDeleteString` specifies that it does nothing if the string is null). modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp line 182: > 180: size_t len = strlen(message); > 181: char* msg = new char[len + 1]; > 182: strcpy_s(msg, len + 1, message); You should check for null before copying. modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp line 199: > 197: char* result = new char[message_length + error_length]; > 198: WideCharToMultiByte(CP_ACP, 0, error, -1, result + message_length, > error_length, NULL, FALSE); > 199: memcpy_s(result, message_length, message, message_length); Same comment about error checking as in `hstring`. modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp line 208: > 206: RoException::~RoException() > 207: { > 208: delete[] message_; Check `message_` for null (meaning you will need to initialize it to null in case of error). modules/javafx.graphics/src/main/native-glass/win/RoActivationSupport.cpp line 213: > 211: RoException& RoException::operator=(RoException source) > 212: { > 213: std::swap(*this, source); Did you mean to do the swap here? ------------- PR Review: https://git.openjdk.org/jfx/pull/1014#pullrequestreview-1763528794 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416402043 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416377863 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416379302 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416380431 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416380902 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416394171 PR Review Comment: https://git.openjdk.org/jfx/pull/1014#discussion_r1416396832
