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

[Artem Ananiev]

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