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

[Artem Ananiev]

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.

What about X11? Since we change semantics of Component.getContainer(), 
which is shared code, all the platforms should be checked.

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.
>>>
>>>
>>>
>>
>
>