On Tue, 26 Nov 2019 15:16:41 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
> On Tue, 26 Nov 2019 15:16:38 GMT, Ambarish Rapte <ara...@openjdk.org> wrote: > >> The finalize() method is deprecated in JDK9. See [Java 9 deprecated >> features](https://www.oracle.com/technetwork/java/javase/9-deprecated-features-3745636.html). >> And so the JPEGImageLoader.finalize() method should be removed. >> >> The change is, >> 1. Remove finalize method from JPEGImageLoader class. >> >> 2. Instance of JPEGImageLoader is created and used in ImageStorage class. >> JPEGImageLoader.dispose() should be called after it's use over. This would >> be a common call for the other (GIF, PNG, BMP) ImageLoader classes, which >> have empty dispose() method. >> >> 3. JPEGImageLoader.load() method almost always calls the dispose() method >> after the image is loaded. In normal scenario JPEGImageLoader is disposed >> here. The calls to dispose() added in ImageStorage seem logically correct >> place to add and should be added. >> >> Verification: >> Verified :graphics:test and system tests on all three platforms. >> Verified that JPEGImageLoader.dispose() is always initiated by >> JPEGImageLoader.load() >> >> ---------------- >> >> Commits: >> - b48c8087: 8196587: Remove use of deprecated finalize method from >> JPEGImageLoader >> >> Changes: https://git.openjdk.java.net/jfx/pull/50/files >> Webrev: https://webrevs.openjdk.java.net/jfx/50/webrev.00 >> Issue: https://bugs.openjdk.java.net/browse/JDK-8196587 >> Stats: 28 lines in 2 files changed: 14 ins; 12 del; 2 mod >> Patch: https://git.openjdk.java.net/jfx/pull/50.diff >> Fetch: git fetch https://git.openjdk.java.net/jfx pull/50/head:pull/50 > > I still need to double-check all calls to dispose, but I think this is > essentially the right solution, and is ready to be submitted for review. I > added one comment inline. > > modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java > line 273: > >> 272: } else { >> 273: throw new ImageStorageException("No loader for image >> data"); >> 274: } > > Now that this is moved inside a try/catch, this `ImageStorageException` will > get wrapped in another `ImageStorageException` if it is ever thrown. You > probably want to catch `ImageStorageException` below and re-throw it without > wrapping it. I will change the catch as below and update with next commit. } catch (ImageStorageException ise) { throw ise; } catch (IOException e) { throw new ImageStorageException(e.getMessage(), e); } PR: https://git.openjdk.java.net/jfx/pull/50
