[webkit-reviews] review granted: [Bug 212338] [WPE] Avoid possible WTR event timing going back in time and fix conversion from s to ms : [Attachment 400185] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at gnome.org> has granted Lauro Moura
<lmoura at igalia.com>'s request for review:
Bug 212338: [WPE] Avoid possible WTR event timing going back in time and fix
conversion from s to ms
https://bugs.webkit.org/show_bug.cgi?id=212338

Attachment 400185: Patch

https://bugs.webkit.org/attachment.cgi?id=400185&action=review




--- Comment #4 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 400185
  --> https://bugs.webkit.org/attachment.cgi?id=400185
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=400185&action=review

> Source/WebKit/Shared/libwpe/WebEventFactory.cpp:66
> +    // From wayland docs:
> +    //  Input events also carry timestamps with millisecond granularity.
Their base
> +    //  is undefined, so they can't be compared against system time (as
obtained
> +    //  with clock_gettime or gettimeofday). They can be compared with each
other
> +    //  though, and for instance be used to identify sequences of button
presses
> +    //  as double or triple clicks.

Same for X event timestamps. Anyway, I like your solution since it drops the
assumption that the WPE backend (or, for GTK, the display server) is going to
be using CLOCK_MONOTONIC. This is true for X11, and it's true for GNOME, and
it's probably true for other Wayland compositors that exist today as well, so
it's a "safe" assumption, but it's never really been correct.

I was almost going to suggest changing the GTK implementation (duplicated in
GtkUtilities.h and GtkUtilities.cpp) as well, but now I see that your real
problem here is the handling of 0. In GTK, 0 always represents the current time
(GDK_CURRENT_TIME) and it's normal to see events using it whenever. I
understand from your comment that this handling is causing trouble for WPE, but
I'm not sure I entirely understand how exactly....

Probably adding a comment on Wayland is not good to do here, btw, because WPE
might not be using Wayland at all. Suffice to say that the WPE timestamp is
always monotonically increasing and unrelated to wall time (like X or Wayland
timestamps).

> Source/WebKit/Shared/libwpe/WebEventFactory.cpp:72
> +    std::unique_lock<std::mutex> lock(m);

I would use std::call_once for this, it's nice and idiomatic. (We also have
GOnce, but std::call_once is nicer in C++.)

> LayoutTests/ChangeLog:12
> +	   Added new test to check if WPE input events timestamps are actually
> +	   monotonically increasing since the start.
> +
> +	   * platform/wpe/fast/events/monotonic-event-time-expected.txt: Added.
> +	   * platform/wpe/fast/events/monotonic-event-time.html: Added.

Good test! I assume the test failed before your patch?

This test should pass on all ports though, right? I don't think it should be
WPE-specific. Try putting it under toplevel fast/events and see if it passes
EWS. :) We've got a problem if timestamps are jumping around on any port! (If
GTK or another port fails the test, you can add an appropriate TestExpectations
line in order to land it without fixing that port. But hopefully no other port
will fail the test.)