RFR: 8356137: GifImageDecode can produce opaque image when disposal method changes [v2]

[Jeremy Wood]

On Tue, 6 May 2025 10:33:47 GMT, Mikhail Yankelevich <myankelevich at 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