On 09/12/15 17:17, Laurent Bourgès wrote:
Hi Sergey,
Thanks for your comments, did you review the PNGImageWriter changes too ?
A few notes:
- Should PNGImageWriteParam.unsetCompression resets the
compressionType to compressionTypes[0]?
- Is it necessary to copy the checks in PNGImageWriteParam from the
parent? We can just call super() and expect that the parent class
validate current state.
Yes, I could check that every png file is not zero-length or that better
compression gives smaller file size !
It would be good, because right now it check not all changes in the fix.
However, my changes rely on the public Deflater API that supports
compression levels between 0 to 9.
I suppose that this Deflater class is already covered by its own tests
so I would prefer leaving the test as it is (no verification): if the
Deflater raises any RuntimeException, then the PNGWriterCompressionTest
will fail.
But this test is passed even for such fix:
---
a/src/java.desktop/share/classes/com/sun/imageio/plugins/png/PNGImageWriter.java
Thu Dec 10 14:21:44 2015 +0300
+++
b/src/java.desktop/share/classes/com/sun/imageio/plugins/png/PNGImageWriter.java
Thu Dec 10 15:15:41 2015 +0300
@@ -280,6 +280,7 @@
super();
this.canWriteProgressive = true;
this.locale = locale;
+ this.canWriteCompressed = true;
}
}
Why not but it is a different scope ?
I wrote the test focused on testing my patch to validate all my changes
and fix ASAP that very old bug (created in 2006) !
I understood, but to change the test and cover of existed/future plugins
should not require a big effort. It does not mean that you need to fix
some bugs(if any then create a new CR) in other plugins.
See for example this bug JDK-8144245 which was found in a new plugin
just because the test cover all of them. But this is up2you.
--
Best regards, Sergey.
