RFR: 8339603: Seal the class hierarchy of Node, Camera, LightBase, Shape, Shape3D [v8]

[Kevin Rushforth]

On Sat, 9 Nov 2024 07:45:14 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>> None of these classes can be extended by user code, and any attempt to do so will fail at runtime with an exception. For this reason, we can seal the class hierarchy and remove the run-time checks to turn this into a compile-time error instead.
>> 
>> In some cases, `Node` and `Shape` are extended by JavaFX classes in other modules, preventing those derived classes from being permitted subclasses. A non-exported `AbstractNode` and `AbstractShape` class is provided just for these scenarios. Note that introducing a new superclass is a source- and binary-compatible change (see [JLS ch. 13](https://docs.oracle.com/javase/specs/jls/se22/html/jls-13.html)).
>> 
>> I'm not sure if this change requires a CSR, as it doesn't change the specification in any meaningful way. There can be no valid JavaFX program that is affected by this change.
>
> Michael Strauß has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits:
> 
>  - Merge branch 'master' into feature/sealed-classes
>  - Merge branch 'master' into feature/sealed-classes
>  - Merge branch 'master' into feature/sealed-classes
>  - Merge branch 'master' into feature/sealed-classes
>  - add comment
>  - Merge branch 'master' into feature/sealed-classes
>  - remove documentation
>  - Seal Node, Camera, LightBase, Shape, Shape3D

This looks good. I left one question about whether (or not) to throw `InternalError` on a null (this was a case of a runtime check that should no longer be possible).

I also left a question in the CSR about the two public classes (MediaView and SwingNode) that extend `AbstractNode`. They show up in the javadocs, even though they are inaccessible. I think this is OK, but wanted Joe's take on it.

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/NodeHelper.java line 66:

> 64: 
> 65:     protected static NodeHelper getHelper(Node node) {
> 66:         return nodeAccessor.getHelper(node);

Is it worth checking for null and throwing an internal error if it is?

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

Marked as reviewed by kcr (Lead).

PR Review: https://git.openjdk.org/jfx/pull/1556#pullrequestreview-2462711912
PR Review Comment: https://git.openjdk.org/jfx/pull/1556#discussion_r1859251468