[Rev 01] RFR: 8244112: Skin implementations: must not violate contract of dispose

[Ambarish Rapte]

On Wed, 6 May 2020 11:46:27 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> some skins have not been guarding themselves against multiple calls to dispose (see issue for details)
>> 
>> Fixed by backing out off dispose if skinnable is null. Added test (parameterized in control class) for all controls in
>> the controls package. Those that failed for the misbehaving skins before are passing after the fix.
>
> Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision:
> 
>   cleanup
>   
>   - corrected incorrect bug id in commented
>   - removed unrelated test change from TextAreaTest
>   - updated copyright year

Eventually we might need to add this check to all the skin's dispose() which will be fixed for cleanup.
Looks good to me, suggested some minor changes.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java line 222:

> 221:         // FIXME : JDK-8244112 - backout if we are already disposed
> 222:         // should check for getSkinnable to be null and return
> 223:         if (getSkinnable() == null) return;

This comment can be removed. FIXME is getting fixed. :)

modules/javafx.controls/src/shims/java/javafx/scene/control/ControlShim.java line 38:

> 37:      *
> 38:      * @param control the control the set the default skin on
> 39:      */

typo:- the control the set => the control **to** set

modules/javafx.controls/src/shims/java/javafx/scene/control/ControlShim.java line 34:

> 33:      *
> 34:      * Note that this has no noticeable effect if the the control's
> 35:      * skin already is set to the default skin (see skinProperty for

typo:-  if the the control's => if the control's

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinDisposeContractTest.java line 159:

> 158:             {TreeTableView.class, },
> 159:             {TreeView.class, },
> 160:         };

Should these controls be included too: `Tooltip`, `HTMLEditor`, `ContextMenu` ?

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

Changes requested by arapte (Reviewer).

PR: https://git.openjdk.java.net/jfx/pull/209