The fix looks good to me.
Thanks,
Alexandr.
On 30/06/16 11:34, Alexander Zvegintsev wrote:
The fix looks good to me.
On 6/30/16 9:07 AM, Alexandr Scherbatiy wrote:
On 6/29/2016 10:08 PM, Robin Stevens wrote:
Hello,
attached you find an updated webrev which addresses the comments:
- no custom remove implementation, but instead call fItems.clear()
after calling removeAll()
- Attached the container listener to the popupmenu
- Used the key instead of the value to remove items from the hashmap
- The test is now marked to run only on OS X
The uploaded webrev:
http://cr.openjdk.java.net/~alexsch/robin.stevens/8158325/webrev.01
Thanks,
Alexandr.
Robin
On Wed, Jun 29, 2016 at 2:01 PM, Alexander Zvegintsev
<alexander.zvegint...@oracle.com> wrote:
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.zvegint...@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.scherba...@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
