[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
From:       Ambarish Rapte <arapte () openjdk ! java ! net>
Date:       2019-11-27 11:57:43
Message-ID: pyzPJOgWnLRhiTrSqN6cgTgswPizZzCUiNsb1KRvkno=.26b46385-56b5-4d11-8ce9-6b8330d83bb1 () github ! com
[Download RAW message or body]

On Tue, 26 Nov 2019 15:16:41 GMT, Kevin Rushforth <kcr@openjdk.org> wrote:

> On Tue, 26 Nov 2019 15:16:38 GMT, Ambarish Rapte <arapte@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


[prev in list] [next in list] [prev in thread] [next in thread]