8382049: [TestBug] Multiple StubToolkit checks in JavaFX production code#2146
8382049: [TestBug] Multiple StubToolkit checks in JavaFX production code#2146Maran23 wants to merge 2 commits into
Conversation
|
👋 Welcome back mhanl! A progress list of the required criteria for merging this PR into |
|
@Maran23 This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 28 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Webrevs
|
|
/reviewers 2 |
|
@andy-goryachev-oracle |
|
Re: TableRowSkinBases and DO_ANIMATIONS - there were quite a few memory leak fixes implemented recently (er, 2024), so enabling animations is likely to be ok (especially if it does not fail any headful CI runs). Tickets in question: |
I had the same thought. |
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
I am running CI builds, will update on the result. the code changes look good.
|
Out CI runs are all green. |
…ed in Java (looking at you, C++)
andy-goryachev-oracle
left a comment
There was a problem hiding this comment.
thank you! lgtm
|
@Maran23 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
sorry for letting this PR auto-expire. please re-open. |
|
@lukostyra could you be the second reviewer please? |
lukostyra
left a comment
There was a problem hiding this comment.
LGTM. Ran both local and CI testing and everything looks good.
|
/integrate |
|
Going to push as commit 58a7bea.
Your commit was automatically rebased without conflicts. |
This PR removes all
StubToolkitchecks from JavaFX production code.There are only a few.
Two notes:
Note 1:
TableRowSkinBasesDO_ANIMATIONSis nowtruefor tests. It was not before due to a leak (as far as I could find when digging JDK-8120657). But I could not find any problem, even running all tests with-Xmx512mChecking tests that trigger the affected code path and creating a heapdump after, I could not see anything weird:
Note 2: Now when the tests run in CI,

JavaFX: using test.com.sun.javafx.pgstub.StubToolkitis now printed 12 times (probably the amount of Unit Test Threads):If this is a problem, we can decide if we want to change the print code in
Toolkit(in a follow-up).-> Changed to
System.outin this PR.Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/2146/head:pull/2146$ git checkout pull/2146Update a local copy of the PR:
$ git checkout pull/2146$ git pull https://git.openjdk.org/jfx.git pull/2146/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2146View PR using the GUI difftool:
$ git pr show -t 2146Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/2146.diff
Using Webrev
Link to Webrev Comment