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

[Artem Ananiev]

Thanks for clarification. Looks fine.

Thanks,

Artem

On 4/29/2013 5:19 PM, Sergey Bylokhov wrote:
> 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.
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>
>