Bug 210642

Summary: [GTK] Update for GdkKeymap API changes
Product: WebKit Reporter: Claudio Saavedra <csaavedra>
Component: WebKitGTKAssignee: Claudio Saavedra <csaavedra>
Status: RESOLVED FIXED    
Severity: Normal CC: aperez, bugs-noreply, cgarcia
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 210100, 210651    
Attachments:
Description Flags
Patch none

Description Claudio Saavedra 2020-04-17 02:04:38 PDT
[GTK] Update for GdkKeymap API changes
Comment 1 Claudio Saavedra 2020-04-17 02:07:28 PDT
Created attachment 396747 [details]
Patch
Comment 2 Adrian Perez 2020-04-17 08:39:53 PDT
Comment on attachment 396747 [details]
Patch

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

> Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:1345
> +    return gdk_keymap_get_caps_lock_state(gdk_keymap_get_for_display(gdk_display_get_default()));

Ah, too bad we are not saving the “GdkEventKey“ as part of the
“PlatformKeyboardEvent” struct; if that was the case we could use

  gdk_keymap_get_for_display(gdk_window_get_display(gdk_event_get_window(event)))

to make this more precise. Anyhoo, just throwing ideas around, I think
the proposed change is fine as-is because the _get_default() was being
used before anyway.

> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:307
> +    GdkDisplay* display = gdk_window_get_display(pressEvent->key.window);

Please change this to use “gdk_event_get_window(pressEvent)” before
landing instead of accessing “pressEvent->key.window” directly.
Comment 3 Claudio Saavedra 2020-04-17 08:45:14 PDT
Comment on attachment 396747 [details]
Patch

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

>> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:307
>> +    GdkDisplay* display = gdk_window_get_display(pressEvent->key.window);
> 
> Please change this to use “gdk_event_get_window(pressEvent)” before
> landing instead of accessing “pressEvent->key.window” directly.

There are other direct accesses to GdkEvent members, should we instead fix those in a follow up?
Comment 4 Adrian Perez 2020-04-17 08:47:26 PDT
(In reply to Claudio Saavedra from comment #3)
> Comment on attachment 396747 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396747&action=review
> 
> >> Tools/WebKitTestRunner/gtk/EventSenderProxyGtk.cpp:307
> >> +    GdkDisplay* display = gdk_window_get_display(pressEvent->key.window);
> > 
> > Please change this to use “gdk_event_get_window(pressEvent)” before
> > landing instead of accessing “pressEvent->key.window” directly.
> 
> There are other direct accesses to GdkEvent members, should we instead fix
> those in a follow up?

Sure, a follow-up works as well.
Comment 5 EWS 2020-04-17 08:50:17 PDT
Committed r260255: <https://trac.webkit.org/changeset/260255>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396747 [details].
Comment 6 Carlos Garcia Campos 2020-04-19 06:51:12 PDT
Comment on attachment 396747 [details]
Patch

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

>> Source/WebCore/platform/gtk/PlatformKeyboardEventGtk.cpp:1345
>> +    return gdk_keymap_get_caps_lock_state(gdk_keymap_get_for_display(gdk_display_get_default()));
> 
> Ah, too bad we are not saving the “GdkEventKey“ as part of the
> “PlatformKeyboardEvent” struct; if that was the case we could use
> 
>   gdk_keymap_get_for_display(gdk_window_get_display(gdk_event_get_window(event)))
> 
> to make this more precise. Anyhoo, just throwing ideas around, I think
> the proposed change is fine as-is because the _get_default() was being
> used before anyway.

We don't save the GdkKeyEvent, because this is created in the web process, and we can't send the GdkWindow of the WebKitWebView to the web process.