Qt5 input events already have a native timestamp, use this timestamp in WK2's WebEventFactoryQt instead of WTF::currentTime if it is available.
Created attachment 117609 [details] proposed patch
Attachment 117609 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/Shared/qt/WebEventFactoryQt.h:45: The parameter name "e" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 117610 [details] proposed patch D'oh, qtcreator...
Comment on attachment 117610 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=117610&action=review Looks ok > Source/WebKit2/Shared/qt/WebEventFactoryQt.cpp:42 > +static inline double computeCurrentTimeSeconds(const QInputEvent* event) InSeconds ? TimeSeconds sounds strange. I think currentTimeForEvent(ev) would be fine
Created attachment 117611 [details] proposed patch Renamed to currentTimeForEvent.
Comment on attachment 117611 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=117611&action=review > Source/WebKit2/Shared/qt/WebEventFactoryQt.cpp:51 > + if (event->timestamp()) > + return static_cast<double>(event->timestamp()) / 1000; > + > + return WTF::currentTime(); Hrmm, a run-time check. Isn't this going to be cause all sorts of havoc if _some_ events had time stamps and some didn't? Imagine: first mouse events has a time stamp, second one doesn't and we use WTF::currentTime(). What if by chance WTF::currentTime() is less than the previous time stamp? Perhaps we should try to protect ourselves against this situation. Perhaps by keeping track which events (of which type that is) have a "native" time stamp and which ones require "emulation", and we let the first event decide. This could even be done in debug builds only. Perhaps this is overkill, just a thought. Otherwise I think the patch is good :)
(In reply to comment #6) > (From update of attachment 117611 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=117611&action=review > > > Source/WebKit2/Shared/qt/WebEventFactoryQt.cpp:51 > > + if (event->timestamp()) > > + return static_cast<double>(event->timestamp()) / 1000; > > + > > + return WTF::currentTime(); > > Hrmm, a run-time check. Isn't this going to be cause all sorts of havoc if _some_ events had time stamps and some didn't? > > Imagine: first mouse events has a time stamp, second one doesn't and we use WTF::currentTime(). What if by chance WTF::currentTime() is less than the previous time stamp? > > Perhaps we should try to protect ourselves against this situation. Perhaps by keeping track which events (of which type that is) have a "native" time stamp and which ones require "emulation", and > we let the first event decide. This could even be done in debug builds only. > All the input events should have timestamps now so this scenario should never happen. But when creating custom events someone forgets to set the timestamp this could be a real concern. I'm going to land this patch as is and try to figure out something which could catch these cases in debug.
Comment on attachment 117611 [details] proposed patch Clearing flags on attachment: 117611 Committed r101791: <http://trac.webkit.org/changeset/101791>
All reviewed patches have been landed. Closing bug.