Bug 210463 - [GTK4] Provide an alternative to gtk_widget_{get,is}_toplevel()
Summary: [GTK4] Provide an alternative to gtk_widget_{get,is}_toplevel()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords:
Depends on:
Blocks: GTK4
  Show dependency treegraph
 
Reported: 2020-04-13 16:00 PDT by Adrian Perez
Modified: 2020-04-15 07:25 PDT (History)
9 users (show)

See Also:


Attachments
Patch (19.68 KB, patch)
2020-04-13 16:14 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch v2 (10.06 KB, patch)
2020-04-14 05:57 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch for landing (8.82 KB, patch)
2020-04-15 06:42 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch for landing (8.82 KB, patch)
2020-04-15 06:44 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adrian Perez 2020-04-13 16:00:21 PDT
Functions gtk_widget_{get,is}_toplevel() are not available in GTK4.
Comment 1 Adrian Perez 2020-04-13 16:14:45 PDT
Created attachment 396354 [details]
Patch
Comment 2 EWS Watchlist 2020-04-13 16:15:18 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 Carlos Garcia Campos 2020-04-13 22:13:36 PDT
Comment on attachment 396354 [details]
Patch

We used to have GtkVersioning when we supported gtk2 and 3 where we added the functions not available in one or the other. So, for example, now we would add gtk_widget_get_toplevel and gtk_widget_is_top_level only when building with GTK4 to leave most of the code free of ifdefs.
Comment 4 Adrian Perez 2020-04-14 05:57:38 PDT
Created attachment 396406 [details]
Patch v2
Comment 5 Adrian Perez 2020-04-14 05:59:22 PDT
(In reply to Carlos Garcia Campos from comment #3)
> Comment on attachment 396354 [details]
> Patch
> 
> We used to have GtkVersioning when we supported gtk2 and 3 where we added
> the functions not available in one or the other. So, for example, now we
> would add gtk_widget_get_toplevel and gtk_widget_is_top_level only when
> building with GTK4 to leave most of the code free of ifdefs.

Adding a “GtkVersioning.h” header which provides an implementation for
these functions when building against GTK4 made the patch much simpler,
thanks for the idea!
Comment 6 Carlos Garcia Campos 2020-04-15 06:00:10 PDT
Comment on attachment 396406 [details]
Patch v2

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

> Source/WebCore/ChangeLog:23
> +2020-04-13  Adrian Perez de Castro  <aperez@igalia.com>

Double changelog.

> Source/WebCore/platform/gtk/GtkUtilities.cpp:50
> +

why?

> Source/WebCore/platform/gtk/GtkVersioning.h:25
> +#if GTK_CHECK_VERSION(3, 90, 0)

why not USE(GTK4)? it's clearer.

> Source/WebCore/platform/gtk/GtkVersioning.h:43
> +gtk_window_get_position(GtkWindow*, int* x, int *y)

int* y

> Source/WebKit/UIProcess/API/glib/WebKitUIClient.cpp:202
> +            // There is no GtkWidget.configure-event in GTK4, move is not supported.

GtkWidget::configure-event
Comment 7 Adrian Perez 2020-04-15 06:10:42 PDT
(In reply to Carlos Garcia Campos from comment #6)
> Comment on attachment 396406 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396406&action=review
> 
> > Source/WebCore/ChangeLog:23
> > +2020-04-13  Adrian Perez de Castro  <aperez@igalia.com>
> 
> Double changelog.

Ouch 🤦️

> > Source/WebCore/platform/gtk/GtkUtilities.cpp:50
> > +
> 
> why?

This slipped in, I think at some point I tried to move some code
around here and didn't undo the change completely. I'll remove it.

> > Source/WebCore/platform/gtk/GtkVersioning.h:25
> > +#if GTK_CHECK_VERSION(3, 90, 0)
> 
> why not USE(GTK4)? it's clearer.

Putting USE(), PLATFORM(), or ENABLE() macros in headers will make
the style checker complain; but we can rely on GTK_CHECK_VERSION
everywhere. If it's okay to ignore the style checker complain I can
put USE(GTK4) back here.
> > Source/WebCore/platform/gtk/GtkVersioning.h:43
> > +gtk_window_get_position(GtkWindow*, int* x, int *y)
> 
> int* y

👌️

> > Source/WebKit/UIProcess/API/glib/WebKitUIClient.cpp:202
> > +            // There is no GtkWidget.configure-event in GTK4, move is not supported.
> 
> GtkWidget::configure-event

👌️
Comment 8 Carlos Garcia Campos 2020-04-15 06:38:12 PDT
(In reply to Adrian Perez from comment #7)
> (In reply to Carlos Garcia Campos from comment #6)
> > Comment on attachment 396406 [details]
> > Patch v2
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=396406&action=review
> > 
> > > Source/WebCore/ChangeLog:23
> > > +2020-04-13  Adrian Perez de Castro  <aperez@igalia.com>
> > 
> > Double changelog.
> 
> Ouch 🤦️
> 
> > > Source/WebCore/platform/gtk/GtkUtilities.cpp:50
> > > +
> > 
> > why?
> 
> This slipped in, I think at some point I tried to move some code
> around here and didn't undo the change completely. I'll remove it.
> 
> > > Source/WebCore/platform/gtk/GtkVersioning.h:25
> > > +#if GTK_CHECK_VERSION(3, 90, 0)
> > 
> > why not USE(GTK4)? it's clearer.
> 
> Putting USE(), PLATFORM(), or ENABLE() macros in headers will make
> the style checker complain; but we can rely on GTK_CHECK_VERSION
> everywhere. If it's okay to ignore the style checker complain I can
> put USE(GTK4) back here.

Why? we use those macros in headers a lot. What's the exactly error message you get?

> > > Source/WebCore/platform/gtk/GtkVersioning.h:43
> > > +gtk_window_get_position(GtkWindow*, int* x, int *y)
> > 
> > int* y
> 
> 👌️
> 
> > > Source/WebKit/UIProcess/API/glib/WebKitUIClient.cpp:202
> > > +            // There is no GtkWidget.configure-event in GTK4, move is not supported.
> > 
> > GtkWidget::configure-event
> 
> 👌️
Comment 9 Adrian Perez 2020-04-15 06:41:36 PDT
(In reply to Carlos Garcia Campos from comment #8)
> (In reply to Adrian Perez from comment #7)
> > (In reply to Carlos Garcia Campos from comment #6)
> >
> > > […]
> > > 
> > > > Source/WebCore/platform/gtk/GtkVersioning.h:25
> > > > +#if GTK_CHECK_VERSION(3, 90, 0)
> > > 
> > > why not USE(GTK4)? it's clearer.
> > 
> > Putting USE(), PLATFORM(), or ENABLE() macros in headers will make
> > the style checker complain; but we can rely on GTK_CHECK_VERSION
> > everywhere. If it's okay to ignore the style checker complain I can
> > put USE(GTK4) back here.
> 
> Why? we use those macros in headers a lot. What's the exactly error message
> you get?

You are right, I re-checked today with USE(GTK4) and only with this
patch on top of “trunk” and the style checker did not complain. It
seems that I mixed it up in my head with some other warning I saw
yesterday.

Let's USE(GTK4) and land the patch, then =)
Comment 10 Adrian Perez 2020-04-15 06:42:26 PDT
Created attachment 396525 [details]
Patch for landing
Comment 11 EWS 2020-04-15 06:43:08 PDT
ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.
Comment 12 Adrian Perez 2020-04-15 06:44:45 PDT
Created attachment 396526 [details]
Patch for landing
Comment 13 EWS 2020-04-15 07:25:18 PDT
Committed r260125: <https://trac.webkit.org/changeset/260125>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396526 [details].