8254605: repaint on Android broken

[johanvos]

Don't check on windows visiblity when requesting focus.
For a new window, the call to requestFocus is done before the visibility is set to true.
Fix for JDK-8254605

---

Progress

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

Issue

• JDK-8254605: repaint on Android broken

Reviewers

• Kevin Rushforth (@kevinrushforth - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jfx pull/322/head:pull/322
$ git checkout pull/322

[bridgekeeper]

👋 Welcome back jvos! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 01: Full - Incremental (de562c1)
• 00: Full (45e983a)

[johanvos]

@arapte and @kevinrushforth Since you reviewed #153 can you review this?
I think the change in #153 does not check on "closed stages" but on "windows that are not visible yet" which is not the same. Therefore, this PR would revert #153 as it introduces regression.

[kevinrushforth]

Reverting the fix for JDK-8241840 does seem the best way to address this, since the current failure is much more serious that the memory leak was.

Two comments:

1. Can you file a new bug to redo the fix for JDK-8241840 with a summary like:

[REDO] Memoryleak: Closed focused Stages are not collected with Monocle

Indicate that the original fix is being reverted, so will need to be redone. Link the new bug to both this bug and to JDK-8241840.

2. Unsurprisingly, this backout fix causes one of the newly added unit tests to fail:

$ gradle -PFULL_TEST=true :systemTests:test --tests FocusedWindowMonocleTest
test.javafx.stage.FocusedWindowMonocleTest > testClosedFocusedStageLeak FAILED
    junit.framework.AssertionFailedError: Expected: <null> but was: javafx.stage.Stage@22ff86f5
        at junit.framework.Assert.fail(Assert.java:47)
        at junit.framework.Assert.assertTrue(Assert.java:20)
        at junit.framework.Assert.assertNull(Assert.java:233)
        at junit.framework.Assert.assertNull(Assert.java:226)
        at test.javafx.stage.FocusedWindowTestBase.assertCollectable(FocusedWindowTestBase.java:101)
        at test.javafx.stage.FocusedWindowTestBase.testClosedFocusedStageLeakBase(FocusedWindowTestBase.java:85)
        at test.javafx.stage.FocusedWindowMonocleTest.testClosedFocusedStageLeak(FocusedWindowMonocleTest.java:47)

Can you update your PR to ignore this test with @Ignore("JDK-nnnnnnn") using the newly filed "REDO" bug as the bug ID?

[openjdk]

@johanvos 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:

8254605: repaint on Android broken

Reviewed-by: kcr

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 1 new commit pushed to the master branch:

• 31fce21: 8223375: Remove Netbeans specific files from the source code repository

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 master branch, type /integrate in a new comment.

[arapte]

Reverting #153 seems right, I don't have any specific comments. Please go ahead and merge the change.

[johanvos]

/integrate

[openjdk]

@johanvos Since your change was applied there have been 2 commits pushed to the master branch:

• aab26d9: 8217472: Add attenuation for PointLight
• 31fce21: 8223375: Remove Netbeans specific files from the source code repository

Your commit was automatically rebased without conflicts.

Pushed as commit 01ba6e1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

[FlorianKirmaier]

@johanvos @jperedadnr
A unit-test would be great, otherwise, it would be very hard to fix the memory-leak again.
Is this reproducible outside of the android setup, when calling repaintFromNative explicitly?

Imo the importance of the memoryleak-fix is a bit underestimated because it's a requirement to test for correct memory-behavior. The only reason reverting it doesn't break all kinds of tests is that no one is testing for memory leaks.