Bug 212298 – [GTK][WTR] EventSender: stop using GdkEvent API in preparation for GTK4

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Add internal API to WebKitWebViewBase to synthesize events instead, because with GTK4 it's not possible to create events. In the case of layout tests, the web view is always the target of the events, so we don't really need to send the events to GTK to process them.

[Carlos Garcia Campos]

Created attachment 400115 [details]
Patch

There's only one regression, pointer-lock/mouse-event-delivery.html I'll rework pointer lock ot ot use GdkEvents in a follow up patch.

[Carlos Garcia Campos]

Created attachment 400116 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Adrian Perez]

Comment on attachment 400116 [details]
Patch

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.

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #4)
> Comment on attachment 400116 [details]
> Patch
> 
> 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.)

It makes sense, but we already use WallTime for events, so if we want to change that it must happen in a separate patch to fix all other places where we already use WallTime.

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

Sure!

> > 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)”.

You could have read the ChangeLog :-)

> Could you please add a comment here mentioning that?

Sure!

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

It didn't work.

[Carlos Garcia Campos]

(In reply to Carlos Garcia Campos from comment #5)
> (In reply to Adrian Perez from comment #4)
> > Comment on attachment 400116 [details]
> > Patch
> > 
> > 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.)
> 
> It makes sense, but we already use WallTime for events, so if we want to
> change that it must happen in a separate patch to fix all other places where
> we already use WallTime.
> 
> > > 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.
> 
> Sure!
> 
> > > 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)”.
> 
> You could have read the ChangeLog :-)
> 
> > Could you please add a comment here mentioning that?
> 
> Sure!
> 
> > (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.
> 
> It didn't work.

Ok, it was because of the const, it just needs an additional const_cast.

[Carlos Garcia Campos]

Created attachment 400152 [details]
Patch

[Carlos Garcia Campos]

Created attachment 400157 [details]
Patch

[Carlos Garcia Campos]

Committed r262184: <https://trac.webkit.org/changeset/262184>

[Radar WebKit Bug Importer]

<rdar://problem/63664943>