<AWT Dev> [8] Request for review: 7166296 closed/java/awt/Frame/DisabledParentOfToplevel/DisabledParentOfToplevel.html failed since 1.8.0b36

[Anthony Petrov]

Minor typo in Window.java:
> 3917     // A window has a owner, but it does NOT have a container

Should be "an owner".

Otherwise looks good to me. No need to resend the webrev with the typo 
fixed.

--
best regards,
Anthony

On 04/29/13 16:31, 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.
>>>
>>>
>>>
>>
>
>