<Swing Dev> [9] Review request for JDK-8158325: [macosx]Memory leak in com.apple.laf.ScreenMenu

[Robin Stevens]

Hello Alexandr, Semyon,

2 reviews of this proposed path have happened.

One from Alexandr Scherbatiy who stated that the fix looked good.
One from Alexander Zvegintsev who had some comments, and immediately mailed
his own review with a modified version of my proposed patch (see
http://mail.openjdk.java.net/pipermail/swing-dev/2016-June/006196.html).
His patch is based on my patch, but implements the comments he had.

I am not sure what I need to do now.
I can address his comments, but then I would end up with the same patch as
he proposed in
http://mail.openjdk.java.net/pipermail/swing-dev/2016-June/006196.html .
Please let me know how to proceed with this.

Thanks,

Robin

On Wed, Jun 29, 2016 at 11:16 AM, Alexandr Scherbatiy <
alexandr.scherbatiy at oracle.com> wrote:

> On 6/29/2016 11:43 AM, Semyon Sadetsky wrote:
>
> It looks like that fix is posted twice for the same issue...
>
> Which one is the correct one?
>
>   It should be the first contributed fix. We are just waiting fro the
> response from the fix contributor:
>     http://mail.openjdk.java.net/pipermail/swing-dev/2016-June/006200.html
>
>   Thanks,
>   Alexandr.
>
> --Semyon
>
> On 6/23/2016 7:08 PM, Robin Stevens wrote:
>
> Hello all,
>
> attached is a webrev for issue JDK-8158325 Memory leak in
> com.apple.laf.ScreenMenu: removed JMenuItems are still referenced.
>
> Patch contains a test case which reveals the bug, and a fix.
>
> There were a few issues with the ScreenMenu class:
>
> - The ContainerListener was attached to the JMenu and not to the JMenu#getPopupMenu.
> The JMenu itself does not fire any ContainerEvents, but
>
> the popup does. As a result, the cleanup code in ScreenMenu was never
> triggered. The patch fixes this by attaching the ContainerListener to the
> popup menu.
>
> Note that the ScreenMenu class also attaches a ComponentListener to the JMenu.
> I had no idea whether that one must be attached to the popup menu as well,
> so I did not change it.
>
> - The cleanup code was not triggered when removeAll() was called from the updateItems
> method. I fixed this by overriding the remove(int) method, and
>
> putting the cleanup code in that method. An alternative here would be to
> not override the remove(int) method, but instead call fItems.clear()
> after calling removeAll() . However, overriding the remove(int) method
> sounded more robust to me.
>
> - The cleanup code was incorrect. It tried to remove an item from fItems
> (a map) by calling remove with the value instead of the key. Now the
> remove is called with the key. Because the cleanup code has been moved,
> this required me to loop over the map as I have no direct access to the
> key in the
>
> remove(int) method
>
> - The test can be run on all platforms, although it was written for an OS
> X specific bug. As it can run on all platforms, I did not disable it on
> non OS X platforms. Let me know if I need to adjust this.
>
> Kind regards,
>
> Robin
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/swing-dev/attachments/20160629/c3b857df/attachment.html>