Re: [Integrated] RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
Changeset: 1140d343 Author:Ambarish Rapte Date: 2019-12-18 17:03:12 + URL: https://git.openjdk.java.net/jfx/commit/1140d343 8196587: Remove use of deprecated finalize method from JPEGImageLoader Reviewed-by: kcr, jvos ! modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ImageStorage.java ! modules/javafx.graphics/src/main/java/com/sun/javafx/iio/ios/IosImageLoader.java ! modules/javafx.graphics/src/main/java/com/sun/javafx/iio/jpeg/JPEGImageLoader.java
Re: [Rev 02] RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
On Tue, 17 Dec 2019 20:12:52 GMT, Ambarish Rapte 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() > > The pull request has been updated with 1 additional commit. While it's hard to proof that the dispose() is now called in all possible end scenario's, I don't see a path where it would not be called after applying the PR. The iOS change is fine. - Marked as reviewed by jvos (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/50
Re: [Approved] RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
On Thu, 5 Dec 2019 10:42:35 GMT, Ambarish Rapte wrote: > The pull request has been updated with additional changes. > > > > Added commits: > - c2ba46fd: Remove finalize() from IosImageLoader too > > Changes: > - all: https://git.openjdk.java.net/jfx/pull/50/files > - new: https://git.openjdk.java.net/jfx/pull/50/files/3ba659f0..c2ba46fd > > Webrevs: > - full: https://webrevs.openjdk.java.net/jfx/50/webrev.02 > - incr: https://webrevs.openjdk.java.net/jfx/50/webrev.01-02 > > Issue: https://bugs.openjdk.java.net/browse/JDK-8196587 > Stats: 5 lines in 1 file changed: 0 ins; 4 del; 1 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 Approved by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/50
Re: [Rev 02] RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
The pull request has been updated with additional changes. Added commits: - c2ba46fd: Remove finalize() from IosImageLoader too Changes: - all: https://git.openjdk.java.net/jfx/pull/50/files - new: https://git.openjdk.java.net/jfx/pull/50/files/3ba659f0..c2ba46fd Webrevs: - full: https://webrevs.openjdk.java.net/jfx/50/webrev.02 - incr: https://webrevs.openjdk.java.net/jfx/50/webrev.01-02 Issue: https://bugs.openjdk.java.net/browse/JDK-8196587 Stats: 5 lines in 1 file changed: 0 ins; 4 del; 1 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 PR: https://git.openjdk.java.net/jfx/pull/50
Re: RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
On Mon, 2 Dec 2019 20:40:09 GMT, Kevin Rushforth wrote:
> On Mon, 2 Dec 2019 20:36:20 GMT, Kevin Rushforth wrote:
>
>> On Mon, 2 Dec 2019 15:03:47 GMT, Ambarish Rapte wrote:
>>
>>> On Tue, 26 Nov 2019 15:16:41 GMT, Kevin Rushforth wrote:
>>>
On Tue, 26 Nov 2019 15:16:38 GMT, Ambarish Rapte
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
Re: RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
On Mon, 2 Dec 2019 20:36:20 GMT, Kevin Rushforth wrote:
> On Mon, 2 Dec 2019 15:03:47 GMT, Ambarish Rapte wrote:
>
>> On Tue, 26 Nov 2019 15:16:41 GMT, Kevin Rushforth wrote:
>>
>>> On Tue, 26 Nov 2019 15:16:38 GMT, Ambarish Rapte 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
Re: RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
On Mon, 2 Dec 2019 15:03:47 GMT, Ambarish Rapte wrote:
> On Tue, 26 Nov 2019 15:16:41 GMT, Kevin Rushforth wrote:
>
>> On Tue, 26 Nov 2019 15:16:38 GMT, Ambarish Rapte 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
Re: [Rev 01] RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
The pull request has been updated with additional changes. Added commits: - 3ba659f0: update the catch block Changes: - all: https://git.openjdk.java.net/jfx/pull/50/files - new: https://git.openjdk.java.net/jfx/pull/50/files/b48c8087..3ba659f0 Webrevs: - full: https://webrevs.openjdk.java.net/jfx/50/webrev.01 - incr: https://webrevs.openjdk.java.net/jfx/50/webrev.00-01 Issue: https://bugs.openjdk.java.net/browse/JDK-8196587 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 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 PR: https://git.openjdk.java.net/jfx/pull/50
Re: RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
On Tue, 26 Nov 2019 15:16:41 GMT, Kevin Rushforth wrote:
> On Tue, 26 Nov 2019 15:16:38 GMT, Ambarish Rapte 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.
PR: https://git.openjdk.java.net/jfx/pull/50
Re: RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
On Tue, 26 Nov 2019 15:16:41 GMT, Kevin Rushforth wrote:
> On Tue, 26 Nov 2019 15:16:38 GMT, Ambarish Rapte 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
Re: RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
On Tue, 26 Nov 2019 15:16:38 GMT, Ambarish Rapte 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.
PR: https://git.openjdk.java.net/jfx/pull/50
RFR: 8196587: Remove use of deprecated finalize method from JPEGImageLoader
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 PR: https://git.openjdk.java.net/jfx/pull/50
