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

[Alexander Zvegintsev]

You should create the diff against the repository. This will allow to 
test your fix without applying a bunch of patches.

--
Thanks,
Alexander.

On 06/29/2016 02:49 PM, Robin Stevens wrote:
> Hello Alexander,
>
> just one last question. I assume I need to send a new webrev . But do 
> I have to create one which contains the diff compared to the current 
> tip of the repository, or do I need to create one which contains the 
> diff compared to my previous patch ?
>
> Robin
>
> On Wed, Jun 29, 2016 at 12:03 PM, Alexander Zvegintsev 
> <alexander.zvegintsev at oracle.com 
> <mailto:alexander.zvegintsev at oracle.com>> wrote:
>
>     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/d4fb4700/attachment.html>