Bug 73647 – [Qt] [WK2] Use input event timestamps in WebEvents if available

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Andras Becsi]

Qt5 input events already have a native timestamp, use this timestamp in WK2's WebEventFactoryQt instead of WTF::currentTime if it is available.

[Andras Becsi]

Created attachment 117609 [details]
proposed patch

[WebKit Review Bot]

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.

[Andras Becsi]

Created attachment 117610 [details]
proposed patch

D'oh, qtcreator...

[Kenneth Rohde Christiansen]

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

[Andras Becsi]

Created attachment 117611 [details]
proposed patch

Renamed to currentTimeForEvent.

[Simon Hausmann]

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 :)

[Andras Becsi]

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

[Andras Becsi]

Comment on attachment 117611 [details]
proposed patch

Clearing flags on attachment: 117611

Committed r101791: <http://trac.webkit.org/changeset/101791>

[Andras Becsi]

All reviewed patches have been landed.  Closing bug.