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.

PR: https://git.openjdk.java.net/jfx/pull/50

Reply via email to