[webkit-reviews] review denied: [Bug 212298] [WTR] EventSender: stop using GdkEvent API in preparation for GTK4 : [Attachment 400116] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat May 23 16:31:01 PDT 2020

Adrian Perez <aperez at igalia.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 212298: [WTR] EventSender: stop using GdkEvent API in preparation for GTK4

Attachment 400116: Patch


--- Comment #4 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 400116
  --> https://bugs.webkit.org/attachment.cgi?id=400116

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

> Source/WebKit/Shared/gtk/NativeWebKeyboardEventGtk.cpp:43
> +    : WebKeyboardEvent(type, text, key, code, keyIdentifier,
windowsVirtualKeyCode, nativeVirtualKeyCode, false, WTF::nullopt, WTF::nullopt,
WTFMove(commands), isKeypad, modifiers, WallTime::now())

Shouldn't events rather use “MonotonicTime” instead? Using “WallTime” can
result in odd situations where an event created later in time has an older
timestamp if the system clock gets adjusted in between, which would break
their sequencing.

(System clock adjustments can happen at any time if there is a NTP client
running: if it detects that it cannot correct drift with small incremental
adjustments it may decide to to a bigger time bump and go back in time.
Another case is the user changing timezones e.g. while traveling.)

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2356
> +   
gdk_window_get_root_coords(gtk_widget_get_window(GTK_WIDGET(webViewBase)), x,
y, &xRoot, &yRoot);

See next comment below in line 2469.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:2469
> +   
gdk_window_get_root_coords(gtk_widget_get_window(GTK_WIDGET(webViewBase)), x,
y, &xRoot, &yRoot);

This same #if is repeated. I would put a “void gtkGetWidgetRootCoords()”
helper function in “GtkUtilities.h” with versions for both GTK3 and GTK4,
then use it here and also above in line 2356.

> Source/WebKit/UIProcess/gtk/WebContextMenuProxyGtk.cpp:103
> +	   g_object_ref(window);

Ah, it took me a while (and a detour through GTK code) to figure out
that “gdk_event_destroy()” results in calling “g_object_unref(window)”.
Could you please add a comment here mentioning that?

(Nice catch, by the way!)

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:124
> +	   if (WKStringIsEqualToUTF8CString(keyRef, "rightArrow"))

Wow, out of curiosity I did a “git blame” here and it turns out that
this has gone unnoticed since 2012, and we have the same typo in
“EventSenderProxyWPE.cpp” because parts of the code were very likely
copied ��️

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:232
> +    return (WebKitWebViewBase*)view;

I think a “reinterpret_cast” should work as well.

More information about the webkit-reviews mailing list