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

Anthony Petrov anthony.petrov at oracle.com
Wed Apr 24 02:53:43 PDT 2013

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.

More information about the awt-dev mailing list