On Mon, 2 Dec 2019 20:40:09 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
> On Mon, 2 Dec 2019 20:36:20 GMT, Kevin Rushforth <k...@openjdk.org> wrote: > >> On Mon, 2 Dec 2019 15:03:47 GMT, Ambarish Rapte <ara...@openjdk.org> wrote: >> >>> 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. >>> >>> 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. > 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). Removed `finalize()` from `IosImageLoader.java`, updated PR. PR: https://git.openjdk.java.net/jfx/pull/50
