RFR: 8280982: [Wayland] [XWayland] java.awt.Robot taking screenshots [v9]
Alexander Zvegintsev
azvegint at openjdk.org
Thu Jun 1 12:41:05 UTC 2023
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
More information about the client-libs-dev
mailing list