RFR: 8350048: Enforce threading restrictions for show and hide methods in Window, Control, and Skin

[Kevin Rushforth]

On Wed, 19 Feb 2025 20:39:19 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

> - enforced fx application thread
> - added a headful test `TestThreadingRestrictions`
> 
> ## Note to the Reviewers
> 
> To avoid merge conflicts, the preferred order of integrations:
> 
> #1697 
> #1713 
> #1717

The API doc changes look good, although I noted a couple additional changes that are needed.

The API docs for `Dialog::hide` should also be documented to throw ISE (since it calls `close()` directly the implementation is fine)

The API docs for `Stage::close` should also be documented to throw ISE (since it calls `hide()` directly the implementation is fine)

For `ComboBoxBase`, were you going to get rid of the word "aspect" in the opening sentence "... display the popup aspect of the user interface."? I think that's a little awkward even with that word removed. Maybe it could just be: "... display the popup associated with this control."

As for the implementation, There are a few places where the skin will call hide(), and I can't tell by just looking at the code that they are all guaranteed to be on the FX app thread.

For example, I recommend checking the following:

* `MenuBarSkin` line 799
* `ComboBoxListViewSkin` line 199

There may be others.

tests/system/src/test/java/test/robot/javafx/scene/TestThreadingRestrictions.java line 63:

> 61:  * This test ensures that the threading restrictions are in place where required.
> 62:  */
> 63: @TestMethodOrder(MethodOrderer.MethodName.class)

This is unnecessary (and not a usual practice).

tests/system/src/test/java/test/robot/javafx/scene/TestThreadingRestrictions.java line 239:

> 237:         test(TPopupWindow::new, (p) -> p.show(stage));
> 238:         test(TPopupWindow::new, (p) -> p.show(stage, 0, 0));
> 239:         test(TPopupWindow::new, (p) -> p.show(contentPane, 0, 0));

It's probably worth adding a test for `hide` when it isn't showing (like you've done for other objects).

tests/system/src/test/java/test/robot/javafx/scene/TestThreadingRestrictions.java line 262:

> 260:                 p.show();
> 261:             });
> 262:             p.hide(); // do we need to fail early here, or only when showing?

We should fail early (although this comment seems misplaced, since it is the case where it _is_ showing).

tests/system/src/test/java/test/robot/javafx/scene/TestThreadingRestrictions.java line 268:

> 266: 
> 267:     @Test
> 268:     @Timeout(value = 1, unit = TimeUnit.DAYS)

Um, 1 DAY???

Seriously, though: Remove the timeout (it isn't needed or wanted).

-------------

PR Review: https://git.openjdk.org/jfx/pull/1717#pullrequestreview-2642150796
PR Review Comment: https://git.openjdk.org/jfx/pull/1717#discussion_r1970380200
PR Review Comment: https://git.openjdk.org/jfx/pull/1717#discussion_r1970398687
PR Review Comment: https://git.openjdk.org/jfx/pull/1717#discussion_r1970400883
PR Review Comment: https://git.openjdk.org/jfx/pull/1717#discussion_r1970401521