[8] Request for review: 7166296 closed/java/awt/Frame/DisabledParentOfToplevel/DisabledParentOfToplevel.html failed since 1.8.0b36 (original) (raw)

Sergey Bylokhov Sergey.Bylokhov at oracle.com
Mon Apr 29 06:19:26 PDT 2013

On 29.04.2013 16:50, Artem Ananiev wrote:

Looks fine. Please, add more comments to the bug and describe the difference between OS X and Windows platform wrt how parents, containers, and owners are treated. Bug was updated. What about X11? Since we change semantics of Component.getContainer(), which is shared code, all the platforms should be checked. Actually the bug starts from the X11. But I agree with Antony that the code in X11 is correct, and is necessary to fix methods which it uses. Thanks, Artem On 4/29/2013 4:31 PM, Sergey Bylokhov wrote: Hi, Anthony, Artem. Can you take a look to the new version of the fix: http://cr.openjdk.java.net/~serb/7166296/webrev.02

On 24.04.2013 13:53, Anthony Petrov wrote: Hi Sergey,

This second version of the fix looks much better. A couple of comments: src/macosx/classes/sun/lwawt/LWComponentPeer.java 189 final Container parent = SunToolkit.getNativeContainer(target); I still suggest to avoid using the word "parent" wherever possible. And in this particular case we know exactly that we're requesting a container here. Please rename this variable. src/windows/classes/sun/awt/windows/WComponentPeer.java 773 WComponentPeer getNativeParent(){ I suggest to add a comment above this method stating that we use the term "parent" explicitly because we override the method in top-level window peer implementations. Otherwise the fix looks fine to me. Thank you. -- best regards, Anthony On 04/23/2013 08:51 PM, Sergey Bylokhov wrote: On 23.04.2013 17:23, Anthony Petrov wrote:

Indeed, it does. But in e.g. AwtButton::Create() it uses the same thing as a container. This code is ugly, and the Toolkit/Component.getNativeContainer() is named incorrectly. I see that this erroneous pattern comes from the Windows implementation, but it doesn't make any sense on any of other platforms because they do make a clear distinction between owners and containers and use completely different APIs/concepts to set them. So I'd try and get rid of the pattern. I think we should investigate all use cases (~15 in JDK) and see whether we can rework them to something meaningful. I believe that in most cases a get*Container() method must really return a container belonging to the same top-level window only. And in cases where we do want to get an owner of a window, we must use the Window.getParent() explicitly. I think the latter is only really needed when we create a native window, actually. But somewhere it is necessary to do check of the type. - Like in the first version of the fix: XComponentPeer/XWindowPeer.getContainerPeer() - Or something like this: http://cr.openjdk.java.net/~serb/7166296/webrev.01/ WComponentPeer/WWindowPeer.getNativeParent - Or I can rename this method from getNativeContainer to getNativeParent, and add new getNativeContainer method with correct implementation. Note that in xawt/lwawt getNativeContainer is used only once.

-- Best regards, Sergey.

More information about the awt-dev mailing list