On Tue, 30 May 2023 19:33:20 GMT, Phil Race <p...@openjdk.org> wrote:
>> Alexander Zvegintsev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> fix macos build
>
> src/java.desktop/unix/classes/sun/awt/screencast/ScreencastHelper.java line
> 139:
>
>> 137:
>> 138: for (TokenItem tokenItem : tokensForRectangle) {
>> 139: retVal = getRGBPixelsImpl(
>
> This can't be right. You surely want to check all cases of retVal not just
> the last one.
Updated.
> src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 62:
>
>> 60: private static final String REL_NAME =
>> 61: ".java/.robot/screencast-tokens.properties";
>> 62:
>
> .java is already hidden. Why make .robot hidden ?
Updated.
> src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 150:
>
>> 148: }
>> 149:
>> 150: private static class WatcherThread extends Thread {
>
> What is all this for ? Reading and writing a file doesn't need all this.
This is to work from multiple VM with the file.
We only read the file the first time we use `createScreenCapture` and when
another VM externally modifies the file.
Alternatively we could read it before each use of `createScreenCapture`, but I
wanted to avoid unnecessary reading and reduce the possible simultaneous
writing and reading time.
> src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 387:
>
>> 385: }
>> 386:
>> 387: try (OutputStream output = Files.newOutputStream(PROPS_PATH)) {
>
> Note that this stream is un-buffered (slow)
> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/file/Files.html#newOutputStream(java.nio.file.Path,java.nio.file.OpenOption...)
> Maybe doesn't matter given how small the file is.
Moved to newBufferedWriter/Readed
> src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 43:
>
>> 41:
>> 42: #define EXCEPTION_CHECK_DESCRIBE_CLEAR() if
>> ((*env)->ExceptionCheck(env)) { \
>> 43:
>> (*env)->ExceptionDescribe(env); \
>
> Describe already clears the exception
> https://docs.oracle.com/en/java/javase/17/docs/specs/jni/functions.html#exceptiondescribe
Updated.
> src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c line 375:
>
>> 373: DEBUG_SCREENCAST("@@@ using screen %i\n", index);
>> 374: if (index >= screenSpace.screenCount) {
>> 375: DEBUG_SCREENCAST("⚠ Wrong index for screen\n", NULL);
>
> I've seen this "! in a triangle" symbol in a quite a few places in the DEBUG
> messages.
> Looks like you got some non-ascii char in there.
Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1211860159
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1211860753
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1211727223
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1211865131
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1211864955
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1211866052
