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

Tue Jul 29 11:35:08 UTC 2014

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