On Wed, 31 May 2023 20:53:03 GMT, Andrey Turbanov <aturba...@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