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

[Alexander Zvegintsev]

Hello Robin,

Actually I missed your review, when I've posted mine.

I think that we should proceed with your review as it was the first one. 
So please disregard my review request.


On 6/29/16 12:40 PM, Robin Stevens wrote:
> 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 
> <mailto: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/687c277c/attachment.html>