On Thu, 9 Nov 2023 15:17:25 GMT, Johan Vos <j...@openjdk.org> wrote:
>> When the Java layer removes a systemmenu, release the native resources
>> related to this systemmenu.
>> This removes the strong JNI Global ref, which prevents its references from
>> being gc'ed.
>>
>> The current implementation for the mac-specific system menu creates a menu,
>> but never releases its resources. In the `dealloc` of this menu, the strong
>> jni refs are deleted.
>> With this PR, we now release the native resources associated with a menuItem
>> when that one is removed from a menu. A consequence is that this menuItem
>> should never be used after being removed from its current menu (e.g. it
>> should not be re-added, or its text/shortcut should not be altered).
>> The current implementation will create a new MacMenuDelegate every time a
>> menuItem is inserted into a menu, so there should be no references to the
>> native resources lingering.
>
> Johan Vos has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Use only 10 cycles instead of 50 (preventing this test to take 50 seconds
> in case it fails).
Looks good, tested successfully before (fails) and after (passes).
I've left some comments, mostly minor remarks.
tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 34:
> 32: import java.lang.ref.WeakReference;
> 33: import java.util.ArrayList;
> 34:
minor, move `test.util.memory.JMemoryBuddy` here, and remove empty line:
import test.util.Util;
+ import test.util.memory.JMemoryBuddy;
+ import java.lang.ref.WeakReference;
+ import java.util.ArrayList;
import java.util.concurrent.CountDownLatch;
tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 50:
> 48: import static org.junit.jupiter.api.Assertions.assertEquals;
> 49:
> 50: import test.util.memory.JMemoryBuddy;
remove
tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 133:
> 131: }
> 132:
> 133: public void createMenuBarWithItemsStage() {
change to `private`
tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 148:
> 146: stage.show();
> 147: stage.requestFocus();
> 148: Thread t = new Thread(){
minor, use `new Thread(() -> {...});`(or else add a whitespace before the curly
brace: `new Thread() {`)
tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 159:
> 157: menu.getItems().clear();
> 158: MenuItem menuItem = new MenuItem("MyItem");
> 159: WeakReference wr = new WeakReference<>(menuItem);
Use type parameter `WeakReference<MenuItem> wr`
tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 164:
> 162: });
> 163: }
> 164: Platform.runLater( () -> {
Why is `Platform.runLater` needed here? To wait until the last
`Platform.runLater` in the for loop ends? Is it guaranteed that it will be
called only after the previous one ends?
Minor: remove unneeded white space: `Platform.runLater(() -> {`
tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 166:
> 164: Platform.runLater( () -> {
> 165: int strongCount = 0;
> 166: for (WeakReference wr: uncollectedMenuItems) {
Use type parameter `WeakReference<MenuItem> wr`
tests/system/src/test/java/test/javafx/stage/SystemMenuBarTest.java line 175:
> 173: };
> 174: t.start();
> 175:
minor: remove empty line
-------------
PR Review: https://git.openjdk.org/jfx/pull/1277#pullrequestreview-1723576135
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388631396
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388631765
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388645794
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388635094
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388642837
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388636531
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388647360
PR Review Comment: https://git.openjdk.org/jfx/pull/1277#discussion_r1388639830
