<AWT Dev> [8] Review request for 8046495: KeyEvent can not be accepted in quick mouse clicking

Tue Jul 29 16:13:13 UTC 2014

Hello, Anton.

The fix looks good to me.

With best regards. Petr.

> On Jul 29, 2014, at 3:35 PM, Artem Ananiev <artem.ananiev at oracle.com> wrote:
> 
> Hi, Anton,
> 
> 8u backport looks fine.
> 
> Thanks,
> 
> Artem
> 
> On 7/24/2014 7:29 PM, anton nashatyrev wrote:
>> Hello,
>> 
>>     Artem, Petr, thanks for the fix discussion and review!
>> 
>>     could you please review the backport to 8u-dev now?
>> 
>>     It is a little bit more conservative variant of the fix to jdk9 and
>> was originally proposed for jdk9 as is.
>> 
>> Webrev: http://cr.openjdk.java.net/%7Eanashaty/8046495/8/webrev.00/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8046495
>> 
>> Thank you!
>> Anton.
>> 
>> 
>> On 24.07.2014 18:10, Artem Ananiev wrote:
>>> 
>>> On 7/18/2014 2:28 PM, anton nashatyrev wrote:
>>>> Hello,
>>>> 
>>>>     in offline discussion with Artem and Petr we decided to further
>>>> clean up the code and completely remove TimeHelper functions from
>>>> awt_Component.cpp in JDK9.
>>>> 
>>>>     Could you please review the updated fix:
>>>> http://cr.openjdk.java.net/~anashaty/8046495/9/webrev.01/
>>>> <http://cr.openjdk.java.net/%7Eanashaty/8046495/9/webrev.01/>
>>> 
>>> Looks fine to me.
>>> 
>>> Thanks,
>>> 
>>> Artem
>>> 
>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8046495
>>>> 
>>>> Thank you!
>>>> Anton.
>>>> 
>>>> On 17.07.2014 13:14, anton nashatyrev wrote:
>>>>> Hello All,
>>>>> 
>>>>>    could please anyone else take a look at the fix?
>>>>> 
>>>>> Thanks!
>>>>> Anton.
>>>>> 
>>>>> On 02.07.2014 19:37, Petr Pchelko wrote:
>>>>>> Hello, Anton.
>>>>>> 
>>>>>> Thanks for clarifications and additional testing.
>>>>>> The fix looks good to me too.
>>>>>> 
>>>>>> With best regards. Petr.
>>>>>> 
>>>>>> On 02 июля 2014 г., at 19:34, Anton V. Tarasov
>>>>>> <anton.tarasov at oracle.com <mailto:anton.tarasov at oracle.com>> wrote:
>>>>>> 
>>>>>>> On 02.07.2014 19:28, anton nashatyrev wrote:
>>>>>>>> Hello, Anton
>>>>>>>> 
>>>>>>>> On 02.07.2014 18:13, Anton V. Tarasov wrote:
>>>>>>>>> On 02.07.2014 11:44, Petr Pchelko wrote:
>>>>>>>>>> Hello, Anton.
>>>>>>>>>> 
>>>>>>>>>> I'm not sure I have a detailed understanding of what's happening.
>>>>>>>>>> 
>>>>>>>>>> Before your fix the timestamp of the event represented the time
>>>>>>>>>> when the event was created, and now it's the time when it's sent
>>>>>>>>>> to java.
>>>>>>>>>> This might be important if some events cause other events to be
>>>>>>>>>> issued on the java side.
>>>>>>>>>> 
>>>>>>>>>> So suppose the following:
>>>>>>>>>> Toolkit thread: InputEvent#1 created      - timestamp 0
>>>>>>>>>> Toolkit thread: InputEvent#2 created      - timestamp 1
>>>>>>>>>> Toolkit thread: InputEvent#1 sent           - timestamp 2
>>>>>>>>>> EDT: InputEvent#1 dispatched - timestamp 3
>>>>>>>>>> EDT:               FocusEvent  created  - timestamp 4
>>>>>>>>>> Toolkit thread: InputEvent#2 sent           - timestamp 5
>>>>>>>>>> 
>>>>>>>>>> Before you fix we had the following event order:
>>>>>>>>>> InputEvent#1(ts=0), InputEvent#2(ts=1), FocusEvent(ts=4).
>>>>>>>>>> But after your fix we will have a different order:
>>>>>>>>>> InputEvent#1(ts=2), FocusEvent(ts=4), InputEvent#2(ts=5).
>>>>>>>>>> So now we would have troubles, because the Input Event will go to
>>>>>>>>>> the new focused component instead of the old one.
>>>>>>>>>> Do you think that my arguments are correct? I understand that the
>>>>>>>>>> likelihood of such a situation is very low, but for me it looks
>>>>>>>>>> possible? Am I missing something?
>>>>>>>>> 
>>>>>>>>> A timestamp for a FocusEvent is taken from
>>>>>>>>> the_most_recent_KeyEvent_time which is set on EDT when the event
>>>>>>>>> is dispatched. So the fix shouldn't affect it.
>>>>>>>>> 
>>>>>>>>> Also, in awt_Window.cpp, when a TimedWindowEvent is created it is
>>>>>>>>> passed a timestamp got with TimeHelper::getMessageTimeUTC(). It
>>>>>>>>> appears, that the fix makes key events times even more consistent
>>>>>>>>> with it. So, from the kbd focus perspective, it shouldn't make any
>>>>>>>>> harm.
>>>>>>>>> 
>>>>>>>>> Anton, could you just please run the following tests, at least:
>>>>>>>>> 
>>>>>>>>> test/java/awt/Focus/6981400/*
>>>>>>>>> test/java/awt/KeyboardFocusManager/TypeAhead/*
>>>>>>>> 
>>>>>>>> I've tested with the following set:
>>>>>>>> [closed]/java/awt/event/*
>>>>>>>> [closed]/java/awt/Focus/*
>>>>>>>> [closed]java/awt/KeyboardFocusManager/*
>>>>>>>> 
>>>>>>>> : no unexpected failures here.
>>>>>>>> 
>>>>>>>> I've also verified that my regression test which comes with the fix
>>>>>>>> works fine on Linux and Mac (despite the fix is Win specific).
>>>>>>> 
>>>>>>> Great, thanks!
>>>>>>> 
>>>>>>> Anton.
>>>>>>> 
>>>>>>>> 
>>>>>>>> Thanks for review!
>>>>>>>> Anton.
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Thanks,
>>>>>>>>> Anton.
>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> Another thing I do not understand is why we were used to use the
>>>>>>>>>> complicated formula instead of initializing the msg.time field
>>>>>>>>>> with the JVM current time and using it when sending the event?
>>>>>>>>>> Wouldn't that resolve both your issue and the issue the original
>>>>>>>>>> fix was made for?
>>>>>>>>>> 
>>>>>>>>>> I have a couple of comments about the code, but let's postpone
>>>>>>>>>> that until we decide on the approach.
>>>>>>>>>> 
>>>>>>>>>> Thank you.
>>>>>>>>>> With best regards. Petr.
>>>>>>>>>> 
>>>>>>>>>> On 01 июля 2014 г., at 21:20, anton nashatyrev
>>>>>>>>>> <anton.nashatyrev at oracle.com
>>>>>>>>>> <mailto:anton.nashatyrev at oracle.com>> wrote:
>>>>>>>>>> 
>>>>>>>>>>> Hello,
>>>>>>>>>>>    could you please review the following fix:
>>>>>>>>>>> 
>>>>>>>>>>> fix: http://cr.openjdk.java.net/~anashaty/8046495/9/webrev.00/
>>>>>>>>>>> <http://cr.openjdk.java.net/%7Eanashaty/8046495/9/webrev.00/>
>>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8046495
>>>>>>>>>>> 
>>>>>>>>>>> *Problem:*
>>>>>>>>>>> On Windows the later InputEvent may have earlier timestamp
>>>>>>>>>>> (getWhen()), which results in incorrect processing of enqueued
>>>>>>>>>>> input events and may also potentially be the reason of other
>>>>>>>>>>> artifacts
>>>>>>>>>>> 
>>>>>>>>>>> *Evaluation*:
>>>>>>>>>>> On Windows we have some algorithm for calculating input event
>>>>>>>>>>> timestamp:
>>>>>>>>>>> jdk/src/windows/native/sun/windows/awt_Component.cpp:2100
>>>>>>>>>>> Shortly the timestamp is calculated by the following formula:
>>>>>>>>>>>    when = JVM_CurrentTimeMillis() - (GetTickCount() -
>>>>>>>>>>> GetMessageTime())
>>>>>>>>>>> 
>>>>>>>>>>> Where:
>>>>>>>>>>>  JVM_CurrentTimeMillis() - the same as
>>>>>>>>>>> System.currentTimeMillis()
>>>>>>>>>>>  GetTickCount() - Win32 function, current millis from boot time
>>>>>>>>>>>  GetMessageTime() - Win32 function, millis from boot time of
>>>>>>>>>>> the latest native Message
>>>>>>>>>>> 
>>>>>>>>>>> In theory the formula looks good: we are trying our best to
>>>>>>>>>>> calculate the actual time of the OS message by taking the
>>>>>>>>>>> current JVM time (JVM_CurrentTimeMillis) and adjusting it with
>>>>>>>>>>> the offset (GetTickCount - GetMessageTime) which should indicate
>>>>>>>>>>> how many milliseconds ago from the current moment (GetTickCount)
>>>>>>>>>>> the message has been actually issued (GetMessageTime).
>>>>>>>>>>> In practice due to usage of different system timers by the
>>>>>>>>>>> JVM_CurrentTimeMillis and GetTickCount their values are not
>>>>>>>>>>> updated synchronously and we may get an earlier timestamp for
>>>>>>>>>>> the later event.
>>>>>>>>>>> 
>>>>>>>>>>> *Fix*:
>>>>>>>>>>> Just to use JVM_CurrentTimeMillis only as events timestamp. On
>>>>>>>>>>> Mac this is done in exactly the same way:
>>>>>>>>>>> CPlatformResponder.handleMouse/KeyEvent()
>>>>>>>>>>> 
>>>>>>>>>>> The message time offset calculation has been introduced with the
>>>>>>>>>>> fix for JDK-4434193
>>>>>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-4434193> and it seems
>>>>>>>>>>> that the issue has addressed very similar problem (At times
>>>>>>>>>>> getWhen()in ActionEvent returns higher value than the
>>>>>>>>>>> CurrentSystemTime) which has not been completely resolved in
>>>>>>>>>>> fact.
>>>>>>>>>>> I also didn't find any benefits in using the existing approach:
>>>>>>>>>>> - all the usages of the TimerHelper are in fact reduced to the
>>>>>>>>>>> mentioned formula: when = JVM_CurrentTimeMillis() -
>>>>>>>>>>> (GetTickCount() - GetMessageTime())
>>>>>>>>>>> - GetMessageTime() always increases (except of the int overflow
>>>>>>>>>>> moments), thus we couldn't get earlier OS messages after later
>>>>>>>>>>> ones
>>>>>>>>>>> - TimerHelper::windowsToUTC(DWORD windowsTime) doesn't guarantee
>>>>>>>>>>> returning the same timestamp across different calls for the same
>>>>>>>>>>> message time
>>>>>>>>>>> 
>>>>>>>>>>> Thanks!
>>>>>>>>>>> Anton.
>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>>>> 
>>>>> 
>>>> 
>> 