<AWT Dev> [8] Review request for 6847588: AWT test fails

[Anthony Petrov]

On 06/11/2013 03:31 PM, Sergey Bylokhov wrote:
> On 11.06.2013 15:04, Anthony Petrov wrote:
>> In (X)KFMPeer.java we know we always operate on Window instances. The
>> setCurrentFocusedWindow(Window) method will never be called with any
>> other components. Therefore, to avoid the overhead of taking multiple
>> locks I suggest to use the AWTAccessor.getPeer directly in this case.
> SunToolkiit already used in osx version of KeyboardFocusManagerPeer.

That may as well be a mistake.


>> Also, in all other cases and for the same reason I don't agree with
>> the "making the life easier" proposed by you. Note that it's not just
>> 17 usages. We often call getPeer directly on component instances.
>> Perhaps we should replace those with calls via the AWTAccessor instead.
> Yes we should replace all of them by targetToPeer() or we should remove
> TTP, and use an accessor everywhere, but since TTP is widely used i do
> not see why not to use it.

I don't see how "TTP is widely used." Here's what I actually see:

$ grep -r targetToPeer src/* | wc -l
31
$ grep -r getPeer src/* | grep awt | wc -l
345

Looks like a bit opposite to me.

My main point is that TTP is unnecessarily inefficient for general use 
because it involves too much locking. That's why I'm suggesting to 
prefer the AWTAccessor.getPeer() instead. However, we can't just remove 
the TTP because it's useful for special cases mentioned by you 
(TrayIcon, menus, etc.). Also, I don't think we should change the 
AWTAccessor.getPeer() to handle these special cases because it may slow 
it down. I think that both methods are useful for their purposes and 
should stay as they are. The only thing that needs to be evaluated is 
whether direct calls to Component.getPeer() should be replaced with the 
AWTAccessor version or not. This may not be always required actually.

--
best regards,
Anthony


>> But that's something to be considered under a separate CR.
>>
>> --
>> best regards,
>> Anthony
>>
>> On 06/11/2013 02:53 PM, Sergey Bylokhov wrote:
>>> On 11.06.2013 14:01, Anthony Petrov wrote:
>>>> Anton: Your fix looks fine to me.
>>>>
>>>> Sergey: I grepped our code for targetToPeer and getPeer and I see a
>>>> lot more usages of the latter than the former. Could you please
>>>> explain why using the targetToPeer is a correct way in this case? As I
>>>> see it, it involves taking at least two locks in the AWTAutoShutdown
>>>> code, and hence it seems kind of slow. Why would we prefer it over a
>>>> direct call to (AWTAccessor.)getPeer?
>>> The difference is that SunToolkit.targetToPeer() works for trayIcon,
>>> Menu, MenuBar, MenuItem. So to make life easier I suggest everywhere use
>>> targetToPeer. And probably it is good idea to remove getPeer from
>>> AWTAccessor(17 usage in the jdk)
>>>>
>>>> --
>>>> best regards,
>>>> Anthony
>>>>
>>>> On 06/10/2013 07:30 PM, Sergey Bylokhov wrote:
>>>>> Hi, Anton.
>>>>> I suppose the correct way to get peer from the component is to use
>>>>> (X/LWC)SunToolkit.targetToPeer() if possible.see latest version of
>>>>> LWKeyboardFocusManagerPeer.
>>>>>
>>>>> On 10.06.2013 17:03, Anton Litvinov wrote:
>>>>>> Hello,
>>>>>>
>>>>>> Please review the following fix for the bug CR 6847588 which consists
>>>>>> in calling of an improper implementation of the method
>>>>>> "java.awt.Component.getPeer" in the class
>>>>>> "sun.awt.X11.XKeyboardFocusManagerPeer".
>>>>>>
>>>>>> Webrev: http://cr.openjdk.java.net/~alitvinov/6847588/webrev.00
>>>>>>
>>>>>> Thank you,
>>>>>> Anton
>>>>>
>>>>>
>>>
>>>
>
>