So, there are no memory leaks? I'm OK with the fix, then. On Jul 24, 2013, at 4:28 PM, Sergey Bylokhov <sergey.bylok...@oracle.com> wrote:
> Hi, Leonid. > Yes. When the child window is created, it create its parent also. And the > owner cannot be deallocated until child window is live, but when the owner is > disposed, it dispose all its child windows. > > On 24.07.2013 15:46, Leonid Romanov wrote: >> Looks good for me as well. The only thing that is not clear to me is the >> memory management in AWTWindow: since every child references its parent now, >> wouldn't it prevent the parent from being deallocated after it has been >> disposed? >> >> On Jul 24, 2013, at 3:17 PM, Sergey Bylokhov <sergey.bylok...@oracle.com> >> wrote: >> >>> Hi, Anthony. >>> Please review the updated version >>> http://cr.openjdk.java.net/~serb/8017189/webrev.02/ >>> In the fix a dependency from the dialog was removed. now we use menubar >>> from the toplvl parent window. >>> >>> On 24.07.2013 0:12, Anthony Petrov wrote: >>>> Hi Sergey, >>>> >>>> I'll let Leonid test this patch since he has a number of good test cases. >>>> As for the code changes, they look good to me overall. The only scenario >>>> that concerns me is if we have a hierarchy of a frame and an owned >>>> undecorated window (e.g., a toolbar). With your current fix the menu will >>>> disappear as soon as the window gets activated because it is not a dialog >>>> and its menubar is obviously null: >>>> >>>>> 546 // Finds appropriate menubar in our hierarchy, >>>>> 547 if (self.javaMenuBar != nil || !IS(self.styleBits, IS_DIALOG)) { >>>>> 548 // shortpath >>>>> 549 [CMenuBar activate:self.javaMenuBar modallyDisabled:NO]; >>>>> 550 } >>>> IMO, this is undesirable. Can we remove this if/else altogether and >>>> instead code this logic as follows: >>>> >>>> CMenuBar *menu = <traverse-owners-till-first-non-null-menu>; >>>> [CMenuBar activate:menu modallyDisabled:!<menu-owner>.isEnabled]; >>>> >>>> ? It seems to me that this should cover all possible use cases. >>>> >>>> -- >>>> best regards, >>>> Anthony >>>> >>>> On 07/23/2013 09:37 PM, Sergey Bylokhov wrote: >>>>> Hello. >>>>> Please review updated version of the fix: >>>>> http://cr.openjdk.java.net/~serb/8017189/webrev.01 >>>>> After the fix, for dialogs we activates a menubar from the first visible >>>>> and enabled owner. I use awtwindow owner instead of >>>>> nswindow.parentWindow, because when the windowDidBecomeKey is called for >>>>> the first time our nswindow still have no parentWindow(it is added later). >>>>> Any testing are welcome. >>>>> Thanks. >>>>> >>>>> On 23.07.2013 14:37, Leonid Romanov wrote: >>>>>> On 7/23/2013 14:06, Sergey Bylokhov wrote: >>>>>>> On 22.07.2013 23:32, Leonid Romanov wrote: >>>>>>>> Well, I'd like us to stay consistent with JDK 6. However, if we >>>>>>>> decide to fix this issue in some other way, we need to be consistent >>>>>>>> with other possible cases, like setting frame's menu to null before >>>>>>>> showing a dialog, making frame invisible, and so on. >>>>>>>> But as you've said, this issue is not related to 8017189, so let's >>>>>>>> go back to the fix for 8017189. I've got another question about it. >>>>>>>> When native window loses focus, we call -(void) deactivate method of >>>>>>>> CMenuBar class. At first, I thought that it basically removes all >>>>>>>> the menu items from the menu bar, but then I realized that it is not >>>>>>>> the case, because your fix depends on the fact that the window >>>>>>>> gaining focus inherits the menu bar from the window that just lost >>>>>>>> it. Now, consider step 4 of your scenario. Here, the dialog2 is the >>>>>>>> window that is loosing focus, and dialog1 is the window that is >>>>>>>> gaining it. As a result of dialog2 loosing focus, the current menu >>>>>>>> bar gets marked as not active (sActiveMenuBar in CMenuBar is set to >>>>>>>> false). When dialog1 gains focus, we do nothing with the current >>>>>>>> menu, because the opposite window (dialog2) doesn't formally have a >>>>>>>> menu (opposite->javaMenuBar is NULL). This means that dialog1 now >>>>>>>> has a menu that is formally inactive. >>>>>>>> Since I don't really understand the purpose of -(void) deactivate >>>>>>>> method, I can't say whether the situation I've described above is >>>>>>>> problematic or not. What do you think? >>>>>>> Actually this is not a problem of my fix, this is a problem of >>>>>>> 8010906, which was implemented on top of "opposite" property instead >>>>>>> of "dialog parent". Probably you know why? >>>>>>> I'll try to change it, but not sure is it dangerous to change it now >>>>>>> or not. >>>>>> I agree, after looking more closely at the original code, it seems >>>>>> that we will get the same deactivation issue in case of showing non >>>>>> modal dialog. I've no idea why 8010906 was implemented on top of the >>>>>> opposite, perhaps it looked like the simplest approach back then. Do >>>>>> you think that traversing windows hierarchy tree from the dialog being >>>>>> shown to an ancestor frame with a menu would have been a better idea? >>>>>> >>>>>>>>> On 22.07.2013 16:57, Leonid Romanov wrote: >>>>>>>>>> Hi. >>>>>>>>>> Here is a test case that, with your patch applied, works >>>>>>>>>> differently than JDK 6: >>>>>>>>>> 1. Show JFrame with a menu >>>>>>>>>> 2. Create a modal dialog with the frame as a parent >>>>>>>>>> 3. Dispose the frame >>>>>>>>>> 4. Make dialog visible >>>>>>>>>> >>>>>>>>>> With JDK 6, the dialog's menu will be disabled. With JDK 8, it >>>>>>>>>> will be enabled. So, formally, we've got a regression. I'm not >>>>>>>>>> sure whether it is worth fixing, because it looks like a corner >>>>>>>>>> case, but still. >>>>>>>>>> >>>>>>>>>> On Jul 19, 2013, at 10:15 PM, Sergey Bylokhov >>>>>>>>>> <sergey.bylok...@oracle.com> wrote: >>>>>>>>>> >>>>>>>>>>> Hello, >>>>>>>>>>> Please review the fix for jdk 8 and 7u40. >>>>>>>>>>> The fix for JDK-8010906 don't take into account situation then >>>>>>>>>>> first parent has no menu bar, but the second has. >>>>>>>>>>> So it introduce the next scenario: >>>>>>>>>>> #1. Open the window with File menu. >>>>>>>>>>> #2. Open modal dialog1 =>File menu is disabled >>>>>>>>>>> #3. Open modal dialog2 =>File menu disappears >>>>>>>>>>> #4. Close dialog two >>>>>>>>>>> #5. Close dialog one. File menu reappears, but File still disabled >>>>>>>>>>> >>>>>>>>>>> The steps #3. occurred, because CMenuBar.activate resets the >>>>>>>>>>> current menubar if a passed javaMenuBar is null. >>>>>>>>>>> The steps #5. occurred, because at step #3 we do not remove our >>>>>>>>>>> nsmenu from the deleted NSMenuItem, when the appropriate >>>>>>>>>>> NSMenuItem removed from mainMenu. So at step #5 we got a >>>>>>>>>>> situation, when our nsmenu was added to the two different >>>>>>>>>>> nsmenuitems: old(disabled) and new(enabled). >>>>>>>>>>> >>>>>>>>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8017189 >>>>>>>>>>> Webrev can be found at: >>>>>>>>>>> http://cr.openjdk.java.net/~serb/8017189/webrev.00 >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> Best regards, Sergey. >>>>>>>>>>> >>>>>>>>> -- >>>>>>>>> Best regards, Sergey. >>>>>>>>> >>>>>>> >>>>> >>> >>> -- >>> Best regards, Sergey. >>> > > > -- > Best regards, Sergey. >
