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

[Sergey Bylokhov]

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.