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> > 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> > 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>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 >> >> >> >> >> > >
