Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8196587: Remove use of deprecated finalize method from JPEGImageLoader #50

Closed
wants to merge 3 commits into from

Conversation

@arapte
Copy link

arapte commented Nov 22, 2019

The finalize() method is deprecated in JDK9. See Java 9 deprecated features.
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()

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

JDK-8196587: Remove use of deprecated finalize method from JPEGImageLoader

Approvers

  • Kevin Rushforth (kcr - Reviewer)
  • Johan Vos (jvos - Reviewer)
@bridgekeeper

This comment has been minimized.

Copy link

bridgekeeper bot commented Nov 22, 2019

👋 Welcome back arapte! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

Copy link
Collaborator

kevinrushforth left a comment

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.

@arapte arapte changed the title [WIP]8196587: Remove use of deprecated finalize method from JPEGImageLoader 8196587: Remove use of deprecated finalize method from JPEGImageLoader Nov 26, 2019
@arapte arapte marked this pull request as ready for review Nov 26, 2019
@openjdk openjdk bot added the rfr label Nov 26, 2019
@mlbridge

This comment has been minimized.

Copy link

mlbridge bot commented Nov 26, 2019

Webrevs

@kevinrushforth kevinrushforth self-requested a review Nov 27, 2019
Ambarish Rapte
@kevinrushforth kevinrushforth requested a review from johanvos Dec 2, 2019
@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

kevinrushforth commented Dec 2, 2019

There is one more ImageLoader implementation class that still has a finalize method: IosImageLoader.java.

That method should also be removed, but @johanvos should confirm.

The rest looks fine to me.

@kevinrushforth

This comment has been minimized.

Copy link
Collaborator

kevinrushforth commented Dec 2, 2019

This will need two reviewers anyway, so I tagged @johanvos to review as well.

@arapte

This comment has been minimized.

Copy link
Author

arapte commented Dec 5, 2019

There is one more ImageLoader implementation class that still has a finalize method: IosImageLoader.java.

Removed finalize() from IosImageLoader.java, updated PR.

@openjdk openjdk bot removed the rfr label Dec 5, 2019
@openjdk

This comment has been minimized.

Copy link

openjdk bot commented Dec 5, 2019

@arapte This change can now be integrated. The commit message will be:

8196587: Remove use of deprecated finalize method from JPEGImageLoader

Reviewed-by: kcr, jvos
  • If you would like to add a summary, use the /summary command.
  • To list additional contributors, use the /contributor command.

Since the source branch of this PR was last updated there have been 15 commits pushed to the master branch:

  • d2d44b4: 8207759: VK_ENTER not consumed by TextField when default Button exists
  • fc539b5: 8223296: NullPointerException in GlassScene.java at line 325
  • 71ca899: 8220722: ProgressBarSkin: adds strong listener to control's width property
  • 07af89a: 8221334: TableViewSkin: must initialize flow's cellCount in constructor
  • 4ddf554: 8234916: [macos 10.15] Garbled text running with native-image
  • 46338d0: 8232524: SynchronizedObservableMap cannot be be protected for copying/iterating
  • a68347c: 8235150: IosApplication does not pass the required object in _leaveNestedEventLoopImpl
  • 1c27fbd: 8210955: DOMTest::testEventListenerCascade fails
  • 2d4096a: 8235151: Nonexistent notifyQuit method referred from iOS GlassHelper.m
  • 98035cb: 8211308: Support HTTP/2 in WebView
  • 6892fa1: 8232064: Switch FX build to use JDK 13.0.1 as boot JDK
  • 1d670f1: 8200224: Multiple press event when JFXPanel gains focus
  • 83eb0a7: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
  • 798afbc: 8230610: Upgrade GStreamer to version 1.16.1
  • 126896d: 8234704: Fix attribution in libxslt.md

Since there are no conflicts, your changes will automatically be rebased on top of the above commits when integrating. If you prefer to do this manually, please merge master into your branch first.

  • To integrate this PR with the above commit message, type /integrate in a new comment.
@openjdk openjdk bot added the ready label Dec 5, 2019
Copy link
Collaborator

johanvos left a comment

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.

@arapte

This comment has been minimized.

Copy link
Author

arapte commented Dec 18, 2019

/integrate

@openjdk openjdk bot closed this Dec 18, 2019
@openjdk openjdk bot added the integrated label Dec 18, 2019
@openjdk

This comment has been minimized.

Copy link

openjdk bot commented Dec 18, 2019

@arapte The following commits have been pushed to master since your change was applied:

  • d2d44b4: 8207759: VK_ENTER not consumed by TextField when default Button exists
  • fc539b5: 8223296: NullPointerException in GlassScene.java at line 325
  • 71ca899: 8220722: ProgressBarSkin: adds strong listener to control's width property
  • 07af89a: 8221334: TableViewSkin: must initialize flow's cellCount in constructor
  • 4ddf554: 8234916: [macos 10.15] Garbled text running with native-image
  • 46338d0: 8232524: SynchronizedObservableMap cannot be be protected for copying/iterating
  • a68347c: 8235150: IosApplication does not pass the required object in _leaveNestedEventLoopImpl
  • 1c27fbd: 8210955: DOMTest::testEventListenerCascade fails
  • 2d4096a: 8235151: Nonexistent notifyQuit method referred from iOS GlassHelper.m
  • 98035cb: 8211308: Support HTTP/2 in WebView
  • 6892fa1: 8232064: Switch FX build to use JDK 13.0.1 as boot JDK
  • 1d670f1: 8200224: Multiple press event when JFXPanel gains focus
  • 83eb0a7: 8193445: JavaFX CSS is applied redundantly leading to significant performance degradation
  • 798afbc: 8230610: Upgrade GStreamer to version 1.16.1
  • 126896d: 8234704: Fix attribution in libxslt.md

Your commit was automatically rebased without conflicts.

Pushed as commit 1140d34.

@openjdk openjdk bot removed the ready label Dec 18, 2019
@mlbridge

This comment has been minimized.

Copy link

mlbridge bot commented Dec 18, 2019

Mailing list message from Ambarish Rapte on openjfx-dev:

Changeset: 1140d34
Author: Ambarish Rapte <arapte at openjdk.org>
Date: 2019-12-18 17:03:12 +0000
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.