Hello, Anton. The fix looks good to me.
With best regards. Petr. > On Jul 29, 2014, at 3:35 PM, Artem Ananiev <[email protected]> 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 >>>>>> <[email protected] <mailto:[email protected]>> 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 >>>>>>>>>> <[email protected] >>>>>>>>>> <mailto:[email protected]>> 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. >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>
