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

[Sergey Bylokhov]

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


-- 
Best regards, Sergey.