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.tara...@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.nashaty...@oracle.com> wrote: >>>> >>>>> Hello, >>>>> could you please review the following fix: >>>>> >>>>> fix: http://cr.openjdk.java.net/~anashaty/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 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. >>>> >>> >> >
