Hello, Anton. Thank you for the cleanup, the new version still looks good to me.
With best regards. Petr. On 18 июля 2014 г., at 14:28, anton nashatyrev <anton.nashaty...@oracle.com> 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/ > > 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.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. >>>>>>> >>>>>> >>>>> >>>> >>> >> >
