RFR: 8280982: [Wayland] [XWayland] java.awt.Robot taking screenshots [v9]

[Alexander Zvegintsev]

On Wed, 31 May 2023 20:53:03 GMT, Andrey Turbanov <aturbanov at openjdk.org> wrote:

>> Alexander Zvegintsev has updated the pull request incrementally with four additional commits since the last revision:
>> 
>>  - improve retVal processing
>>  - address token storage comments
>>  - removing non-ascii
>>  - EXCEPTION_CHECK_DESCRIBE_CLEAR -> EXCEPTION_CHECK_DESCRIBE
>
> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 195:
> 
>> 193: 
>> 194:     LIBPIPEWIRE_HEADER_DIRS := \
>> 195:     	$(TOPDIR)/src/$(MODULE)/unix/native/libpipewire/include
> 
> Are you sure Tab should be used here?
> in all other places of this file spaces are used for indentation.

Fixed

> src/java.desktop/unix/classes/sun/awt/screencast/TokenItem.java line 38:
> 
>> 36: import static sun.awt.screencast.ScreencastHelper.SCREENCAST_DEBUG;
>> 37: 
>> 38: final class TokenItem {
> 
> add couple of words to the javadoc

Sure, updated

> src/java.desktop/unix/classes/sun/awt/screencast/TokenItem.java line 111:
> 
>> 109: 
>> 110:     public static TokenItem parse(String token, Object input) {
>> 111:         if (token == null || input == null) return null;
> 
> Do we need this `null` checks?
> Currently only non-null values are passed to this method

It won't hurt.

> src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 303:
> 
>> 301: 
>> 302:         Set<Map.Entry<Object, Object>> entries;
>> 303:         synchronized (PROPS) {
> 
> I'm not sure I understand purpose of this `synchronized` block.
> `entrySet()` returns _view_ of Properties entries, not a copy. It means concurrent thread could modify content of `PROPS` when we do `stream()` below.
> 
> From my opinion either we need to perform all work inside `synchronized` to be sure that no concurrent modification are possible. Or just remove `synchronized` (`Properties` is a thread safe class) and _be ready_ of possible concurrent updates

Thanks for catching this, updated.

> src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 307:
> 
>> 305:         }
>> 306: 
>> 307:         Set<String> malformed = new LinkedHashSet<>();
> 
> Can we use plain HashSet here? Seems there is not much reason to save original order of tokens.

Updated.

> src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 320:
> 
>> 318:                     return tokenItem;
>> 319:                 })
>> 320:                 .filter(obj -> !Objects.isNull(obj))
> 
> `Objects.isNull` wasn't supposed to be used like this. Let's either inline it and use `!= null` or use method reference `.filter(Objects::nonNull)`

Updated.

> src/java.desktop/unix/classes/sun/awt/screencast/TokenStorage.java line 346:
> 
>> 344:                         rectangle.height
>> 345:                 ))
>> 346:                 .collect(Collectors.toCollection(ArrayList::new));
> 
> why not plain toList() collector?

Updated.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1213080352
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1213081541
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1213082454
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1213082814
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1213083695
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1213083915
PR Review Comment: https://git.openjdk.org/jdk/pull/13803#discussion_r1213084108