On Thu, 19 Dec 2019 15:12:12 GMT, Florian Kirmaier <fkirma...@openjdk.org> wrote:
>> Hi everyone, >> >> ticket: https://bugs.openjdk.java.net/browse/JDK-8236259 >> >> The fix itself is quite straight forward. >> It basically just removed the listener which causes the leak. >> >> The unit-test for the fix is a bit more complicated. >> >> I added a library JMemoryBuddy https://github.com/Sandec/JMemoryBuddy >> (written by myself), which simplifies testing for memory leaks. >> I think there are many places in the JavaFX-codebase that could highly >> benefit from this library. >> It could also simplify many of the already existing unit tests. >> It makes testing for memory-leaks readably and reliable. >> It would also be possible to just copy the code of the library into the >> JavaFX-codebase. >> It only contains a single class. >> >> I also had to make a method public, to write the test. I'm open to ideas, >> how I could solve it differently. > > The pull request has been updated with 1 additional commit. I haven't looked at the fix itself, but there are two things that _must_ be fixed before this PR can be considered: 1. You need to remove the additional 3rd-party test library dependency. 2. You need to revert the change that makes a test method public API. build.gradle line 2514: > 2513: compile project(':graphics') > 2514: testCompile "de.sandec:JMemoryBuddy:0.1.3" > 2515: } This is a new third-party dependency, which would need legal approval. You need to remove this. modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 1929: > 1928: */ > 1929: public List<Node> test_getRemoved() { > 1930: return removed; This is not an acceptable change. We do not add public API to support tests. Take a look at the Shim classes for an example of how to handle this. ------------- Changes requested by kcr (Lead). PR: https://git.openjdk.java.net/jfx/pull/71
