On Mon, 2 Dec 2019 20:36:20 GMT, Kevin Rushforth <[email protected]> wrote:
> 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. This will need two reviewers anyway, so I tagged @johanvos to review as well. PR: https://git.openjdk.java.net/jfx/pull/50
