Bug 198829 - [GTK] Stop accessing GdkEvent fields when possible
Summary: [GTK] Stop accessing GdkEvent fields when possible
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-13 07:09 PDT by Ludovico de Nittis
Modified: 2019-06-17 08:01 PDT (History)
7 users (show)

See Also:


Attachments
Patch (39.08 KB, patch)
2019-06-13 07:33 PDT, Ludovico de Nittis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-highsierra (3.00 MB, application/zip)
2019-06-13 09:20 PDT, Build Bot
no flags Details
Patch (40.82 KB, patch)
2019-06-14 00:40 PDT, Ludovico de Nittis
no flags Details | Formatted Diff | Diff
Patch (40.91 KB, patch)
2019-06-14 00:54 PDT, Ludovico de Nittis
no flags Details | Formatted Diff | Diff
Patch (40.94 KB, patch)
2019-06-14 01:09 PDT, Ludovico de Nittis
no flags Details | Formatted Diff | Diff
Patch (40.91 KB, patch)
2019-06-14 01:19 PDT, Ludovico de Nittis
no flags Details | Formatted Diff | Diff
Patch (43.32 KB, patch)
2019-06-14 07:24 PDT, Ludovico de Nittis
no flags Details | Formatted Diff | Diff
Patch (45.21 KB, patch)
2019-06-17 05:30 PDT, Ludovico de Nittis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ludovico de Nittis 2019-06-13 07:09:12 PDT
Direct access to GdkEvent structs is no longer possible in GTK 4.
While it is not (yet?) deprecated in GTK 3, when possible I think that we should already move away from accessing directly Gdk event fields when possible.

For the majority of these fields, getters are already available in GTK 3.
Comment 1 Ludovico de Nittis 2019-06-13 07:33:18 PDT
Created attachment 372053 [details]
Patch
Comment 2 Build Bot 2019-06-13 07:34:52 PDT
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
Comment 3 Michael Catanzaro 2019-06-13 07:37:51 PDT
/home/ews/gtk-wk2-ews/WebKit/Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp: In constructor ‘WebCore::PlatformWheelEvent::PlatformWheelEvent(GdkEventScroll*)’:
/home/ews/gtk-wk2-ews/WebKit/Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:65:5: error: ‘gdk_event_get_scroll_direction’ was not declared in this scope
     gdk_event_get_scroll_direction(reinterpret_cast<GdkEvent*>(event), &direction);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/ews/gtk-wk2-ews/WebKit/Source/WebCore/platform/gtk/PlatformWheelEventGtk.cpp:65:5: note: suggested alternative: ‘gdk_event_get_screen’
     gdk_event_get_scroll_direction(reinterpret_cast<GdkEvent*>(event), &direction);
     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     gdk_event_get_screen
ninja: build stopped: subcommand failed.
Comment 4 Ludovico de Nittis 2019-06-13 07:43:57 PDT
Meh apparently GTK 2 is not happy.
I'm on it.
Comment 5 Build Bot 2019-06-13 09:20:29 PDT
Comment on attachment 372053 [details]
Patch

Attachment 372053 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12465307

New failing tests:
webgl/2.0.0/conformance/context/context-release-upon-reload.html
Comment 6 Build Bot 2019-06-13 09:20:30 PDT
Created attachment 372059 [details]
Archive of layout-test-results from ews116 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 7 Ludovico de Nittis 2019-06-14 00:40:30 PDT
Created attachment 372110 [details]
Patch
Comment 8 Ludovico de Nittis 2019-06-14 00:54:57 PDT
Created attachment 372112 [details]
Patch
Comment 9 Ludovico de Nittis 2019-06-14 01:09:34 PDT
Created attachment 372114 [details]
Patch
Comment 10 Ludovico de Nittis 2019-06-14 01:19:16 PDT
Created attachment 372116 [details]
Patch
Comment 11 Ludovico de Nittis 2019-06-14 01:20:16 PDT
Sorry for the multiple patches. This time should be everything ok.
Comment 12 Michael Catanzaro 2019-06-14 06:48:24 PDT
Comment on attachment 372116 [details]
Patch

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

Thanks.

> Source/WebCore/platform/gtk/PlatformMouseEventGtk.cpp:58
> -    m_position = IntPoint((int)event->x, (int)event->y);
> -    m_globalPosition = IntPoint((int)event->x_root, (int)event->y_root);
> +    m_position = IntPoint((int)x, (int)y);
> +    m_globalPosition = IntPoint((int)rootX, (int)rootY);

Preexisting problem, but since you're modifying these lines, change them to static_cast because C-style casts aren't allowed in WebKit.
Comment 13 Ludovico de Nittis 2019-06-14 07:24:11 PDT
Created attachment 372124 [details]
Patch
Comment 14 WebKit Commit Bot 2019-06-15 11:33:10 PDT
Comment on attachment 372124 [details]
Patch

Clearing flags on attachment: 372124

Committed r246467: <https://trac.webkit.org/changeset/246467>
Comment 15 WebKit Commit Bot 2019-06-15 11:33:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Carlos Garcia Campos 2019-06-17 01:52:36 PDT
Reverted r246467 for reason:

It broke scrolling with mouse wheel

Committed r246494: <https://trac.webkit.org/changeset/246494>
Comment 17 Ludovico de Nittis 2019-06-17 02:08:42 PDT
Comment on attachment 372124 [details]
Patch

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

> Source/WebKit/Shared/gtk/WebEventFactory.cpp:-210
> -    switch (event->scroll.direction) {

Carlos was the scroll broken only with GTK2 enabled? Because I found that I mistyped the direction here. It should be "direction = event->scroll.direction" instead of "direction = event->direction"
Comment 18 Ludovico de Nittis 2019-06-17 03:46:57 PDT
Ok, I'm able to reproduce the problem. I'll upload soon a patch for it.
Comment 19 Ludovico de Nittis 2019-06-17 05:30:02 PDT
Created attachment 372243 [details]
Patch
Comment 20 Ludovico de Nittis 2019-06-17 05:33:53 PDT
The problem was created by "gdk_event_get_scroll_direction" that returns false if the direction is GDK_SCROLL_SMOOTH, so a different behavior compared to "event->direction".

Now hopefully it should be fine.
Comment 21 WebKit Commit Bot 2019-06-17 08:01:15 PDT
Comment on attachment 372243 [details]
Patch

Clearing flags on attachment: 372243

Committed r246496: <https://trac.webkit.org/changeset/246496>
Comment 22 WebKit Commit Bot 2019-06-17 08:01:17 PDT
All reviewed patches have been landed.  Closing bug.