On Tue, 6 May 2025 10:33:47 GMT, Mikhail Yankelevich <myankelev...@openjdk.org> wrote:
>> Jeremy Wood has updated the pull request incrementally with five additional >> commits since the last revision: >> >> - 8356137: removing sentence fragment >> >> This is in response to: >> https://github.com/openjdk/jdk/pull/25044#discussion_r2075200131 >> - 8356137: fixing typo >> - 8356137: adding empty line at bottom of file >> >> This is in response to: >> https://github.com/openjdk/jdk/pull/25044#discussion_r2075192144 >> - 8356137: removing layered pass/fail pattern >> >> This is in response to: >> https://github.com/openjdk/jdk/pull/25044#discussion_r2075190122 >> - 8356137: removing (c) 2002 >> >> This is in response to: >> https://github.com/openjdk/jdk/pull/25044#discussion_r2075186796 > > test/jdk/sun/awt/image/gif/bug8356137/GifEmptyBackgroundTest.java line 2: > >> 1: /* >> 2: * Copyright (c) 2002, 2025, Oracle and/or its affiliates. All rights >> reserved. > > Nitpick: do you need 2002 here? Isn't it a new file? OK, this is removed > test/jdk/sun/awt/image/gif/bug8356137/GifEmptyBackgroundTest.java line 51: > >> 49: boolean pass = true; >> 50: if (new Color(frames[3].getRGB(20, 20), true).getAlpha() != 0) { >> 51: System.err.println("Sampling at (20,20) failed"); > > Do you think it would be cleaner if you jsut throw a > `RuntimeException("Sampling at (20,20) failed");` instead of the whole > > System.err.println("Sampling at (20,20) failed"); > pass = false; > } > > if (!pass) > throw new Error("See System.err for details"); > > ? > It should result in the same level of details but with better readability imo I don't have a strong opinion; this is removed. (I often follow that pattern in case I try to add multiple criteria to pass/fail decisions.) > test/jdk/sun/awt/image/gif/bug8356137/GifEmptyBackgroundTest.java line 143: > >> 141: return returnValue.toArray(new BufferedImage[0]); >> 142: } >> 143: } > > nitpick: could you please add a new line here for github? 😃 OK, this is updated ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075926075 PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075926374 PR Review Comment: https://git.openjdk.org/jdk/pull/25044#discussion_r2075926503
