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

[Anthony Petrov]

Hi Sergey,

On 04/23/2013 04:46 PM, Sergey Bylokhov wrote:
> On 23.04.2013 16:24, Anthony Petrov wrote:
>> This code path exists in order to ensure that a heavyweight component
>> (e.g. a Button) has a valid native container to be inserted to.
>> Obviously, the container must always reside in the same top-level
>> window where the component itself resides. This code should never
>> traverse containers in other top-level windows (even though it does
>> now, which is the real bug here).
> Not sure that It used for container validation, to me it is used for the
> creation of the window with appropriate parent(in AwtWindow::Create).

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.

This should resolve this bug and also make our code clearer.

--
best regards,
Anthony

>>
>> I still think that the real bug here is an absent override method
>> Window.getNativeContainer() that should return "this". Do you agree?
> It should be null, in this case, but will it be better than owner?
>>
>> --
>> best regards,
>> Anthony
>>
>> On 04/23/2013 01:37 AM, Sergey Bylokhov wrote:
>>> Hi, Anthony.
>>> WComponentPeer.java:762 -> WComponentPeer.java:764 ->
>>> WWindowPeer.java:create(WComponentPeer parent).
>>>
>>> On 23.04.2013 0:01, Anthony Petrov wrote:
>>>> I don't believe getNativeContainer() should ever traverse a hierarchy
>>>> outside of the toplevel window for component of which it's been called
>>>> (or for the window itself if it's called on a Window instance). If
>>>> some code expects otherwise, then there is a bug in that code which
>>>> needs fixing.
>>>>
>>>> Could you please point me to exact locations where the code does that?
>>>>
>>>> --
>>>> best regards,
>>>> Anthony
>>>>
>>>> On 04/22/2013 06:24 PM, Sergey Bylokhov wrote:
>>>>> On 22.04.2013 17:55, Anthony Petrov wrote:
>>>>>> Ah, I see the Component.getNativeContainer() checks whether the peer
>>>>>> is lightweight or not. I believe it needs to be overridden in the
>>>>>> Window class to return "this", though. Otherwise there's still a bug
>>>>>> in this method.
>>>>> But window's implementation mix owner/container during
>>>>> initialization of
>>>>> WWindowPeer. There is only one field "parent", which assigned from the
>>>>> SunToolkit.getNativeContainer().
>>>>> I assume that, when this method was added the window's
>>>>> parent/container
>>>>> and the window's owner were one concept.
>>>>>> I don't quite understand this point. Please elaborate.
>>>>>>
>>>>>> What I see is that we call SunToolkit.getNativeContainer() which
>>>>>> isn't
>>>>>> overridden in UNIXToolkit nor XToolkit, and thus results in a call to
>>>>>> Toolkit.getNativeContainer(). The latter calls
>>>>>> Component.getNativeContainer(). Which, w/o an override suggested
>>>>>> above, produces an incorrect result and is the root cause of our bug,
>>>>>> isn't it?
>>>>> The question is, what the result is correct. Currently, some code
>>>>> expects the owner will be refunded.
>>>>>>
>>>>>> --
>>>>>> best regards,
>>>>>> Anthony
>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> best regards,
>>>>>>>> Anthony
>>>>>>>>
>>>>>>>> On 04/22/13 16:06, Sergey Bylokhov wrote:
>>>>>>>>> Hello,
>>>>>>>>> Please review the fix for jdk 8.
>>>>>>>>> SetEnable method check status of all parent containers and
>>>>>>>>> windows(via
>>>>>>>>> getParent() in SunToolkit.getNativeContainer()). But only
>>>>>>>>> containers in
>>>>>>>>> the same window should be checked.
>>>>>>>>> The new method was added to return the peer of the nearest
>>>>>>>>> heavyweight
>>>>>>>>> container.
>>>>>>>>>
>>>>>>>>> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7166296
>>>>>>>>> Webrev can be found at:
>>>>>>>>> http://cr.openjdk.java.net/~serb/7166296/webrev.00
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>
>