On Mon, 10 Mar 2025 13:16:21 GMT, Alexey Ivanov <aiva...@openjdk.org> wrote:
>> Jeremy Wood has updated the pull request incrementally with five additional >> commits since the last revision: >> >> - 8351108: adding empty line to bottom of file >> >> This is in response to: >> https://github.com/openjdk/jdk/pull/23884#discussion_r1987278977 >> - Merge remote-tracking branch 'origin/JDK-8351108' into JDK-8351108 >> - 8351108: replacing wildcard import >> >> This is in response to: >> https://github.com/openjdk/jdk/pull/23884#discussion_r1987268179 >> - 8351108: updating copyright year >> >> This is in response to: >> https://github.com/openjdk/jdk/pull/23884#discussion_r1987265582 >> - 8351108: changing indentation / line wrapping >> >> This is in response to: >> https://github.com/openjdk/jdk/pull/23884#discussion_r1987265134 > > src/java.desktop/share/classes/javax/imageio/ImageTypeSpecifier.java line 1: > >> 1: /* > > Could you bump the copyright year? Thanks, this is updated > src/java.desktop/share/classes/javax/imageio/ImageTypeSpecifier.java line 923: > >> 921: int bufferedImageType = ((BufferedImage)image).getType(); >> 922: if (bufferedImageType != BufferedImage.TYPE_CUSTOM && >> 923: bufferedImageType != BufferedImage.TYPE_BYTE_INDEXED) >> { > > Suggestion: > > if (bufferedImageType != BufferedImage.TYPE_CUSTOM && > bufferedImageType != BufferedImage.TYPE_BYTE_INDEXED) { > > Usually, 8-space indentation used for wrapped continuation lines, if the > wrapped isn't aligned to a structure on the first line. > > Personally, I'd move the `&&` operator to the wrapped line — it clearly > indicates it's a continuation. Yet neither style has been agreed upon. OK, this is updated. > test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java > line 33: > >> 31: >> 32: import javax.imageio.ImageIO; >> 33: import java.awt.*; > > Could you expand the wildcard imports? OK, this is updated. > test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java > line 81: > >> 79: e.printStackTrace(); >> 80: return false; >> 81: } > > Should an exception be allowed to escape? You throw an `Error` if the > expected return value doesn't match. This is by design. The main method resembles: public static void main(String[] args) throws IOException { boolean b1 = testJpegWriter(Transparency.OPAQUE, "OPAQUE", true); boolean b2 = testJpegWriter(Transparency.BITMASK, "BITMASK", false); boolean b3 = testJpegWriter(Transparency.TRANSLUCENT, "TRANSLUCENT", false); if (!(b1 && b2 && b3)) throw new Error("Test failed"); } The intention here is to help log multiple failures at once. (Otherwise: if there are two failures A and B, then you can only discover B after resolving A.) > test/jdk/javax/imageio/plugins/jpeg/JpegWriterWriteNonOpaqueIndexColorModelTest.java > line 83: > >> 81: } >> 82: } >> 83: } > > Having a blank line in the end of a file is preferred. OK, this is updated. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987717503 PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987717590 PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987717421 PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987717299 PR Review Comment: https://git.openjdk.org/jdk/pull/23884#discussion_r1987717159
