On Mon, 2 Dec 2019 15:03:47 GMT, Ambarish Rapte <[email protected]> wrote:
> On Tue, 26 Nov 2019 15:16:41 GMT, Kevin Rushforth <[email protected]> wrote: > >> On Tue, 26 Nov 2019 15:16:38 GMT, Ambarish Rapte <[email protected]> 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. > > Updated the catch statement. There is one more ImageLoader implementation class that still has a finalize method: [IosImageLoader.java(https://github.com/openjdk/jfx/blob/3ba659f0eb52496adb54a2b2c313e8da832e8389/modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ios/IosImageLoader.java#L215). That method should also be removed, but @johanvos should confirm. The rest looks fine to me. PR: https://git.openjdk.java.net/jfx/pull/50
