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

[Anthony Petrov]

Hi Sergey,

Please find replies to all your messages below.

On 06/11/13 17:49, Sergey Bylokhov wrote:
> On 11.06.2013 17:21, Anthony Petrov wrote:
>> On 06/11/2013 03:31 PM, Sergey Bylokhov wrote:
>>> On 11.06.2013 15:04, Anthony Petrov wrote:
>> 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
> and only 17 use accessor.

As I mentioned, some of them might need to be converted to using the 
AWTAccessor version. E.g. at least the case that's being fixed in 
Anton's fix.

>> 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.
> if you care of performance, it is possible speedup TTP, if to implement
> it via accessor for Components subclasses.

Yes, I do. And yes, it's possible. However, we rarely really need to 
retrieve peers for TrayIcon and other such "non-component" objects. 
Hence to me the usage of TTP looks like a special case.

On 06/11/13 18:00, Sergey Bylokhov wrote:
> On 11.06.2013 17:21, Anthony Petrov wrote:
>> 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.
>>>
> Also at least in XToolkit TTP has its own peers map for text components: specialPeerMap

The text components have nothing to do with the 
setCurrentFocusedWindow(Window) case.

On 06/13/13 13:32, Sergey Bylokhov wrote:
> And absence of the lock in an accessor is a bug too.

I wholeheartedly agree on this one. But again, this is a separate issue.

--
best regards,
Anthony

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