RFR: 8301302: Platform preferences API [v39]

[Kevin Rushforth]

On Mon, 4 Dec 2023 23:55:33 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:
> 
>   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