On Fri, 9 Apr 2021 06:15:42 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> The fix looks good. >> >> The test looks OK as far as it goes, although I note that it is testing for >> the leak in an indirect way, since it looks at the `changeListenerMap` >> rather than checking the listeners of `menuitem.acceleratorProperty()` which >> is what we really care about. Is there a way to test it more directly? If >> not then I think this is fine. > >> Is there a way to test it more directly? > > `ControlTestUtils` provides a way to get the number of listeners added to a > property. Please check the updated test. With this direct way, should we keep > the indirect way of testing which uses `changeListenerMap` ? The updated test looks good. I confirm that it fails without the fix and passes with the fix. In order to do that I locally deleted the newly added shim class and comment out all calls to `get_ListenerMapSize`. I'd recommend to get rid of the indirect way of testing, since it will mean you can remove the newly added `ControlAcceleratorSupportShim` class and the `ControlAcceleratorSupport ::testGetListenerMapSize` method. ------------- PR: https://git.openjdk.java.net/jfx/pull/429
