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.
