Bug 210463 – [GTK4] Provide an alternative to gtk_widget_{get,is}_toplevel()

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Adrian Perez]

Functions gtk_widget_{get,is}_toplevel() are not available in GTK4.

[Adrian Perez]

Created attachment 396354 [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

[Carlos Garcia Campos]

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.

[Adrian Perez]

Created attachment 396406 [details]
Patch v2

[Adrian Perez]

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

[Carlos Garcia Campos]

Comment on attachment 396406 [details]
Patch v2

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

> Source/WebCore/ChangeLog:23
> [phone]  Adrian Perez de Castro  <[e-mail]>

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

[Adrian Perez]

(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
> > [phone]  Adrian Perez de Castro  <[e-mail]>
> 
> 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

👌️

[Carlos Garcia Campos]

(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
> > > [phone]  Adrian Perez de Castro  <[e-mail]>
> > 
> > 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
> 
> 👌️

[Adrian Perez]

(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 =)

[Adrian Perez]

Created attachment 396525 [details]
Patch for landing

[EWS]

ChangeLog entry in Source/WebKit/ChangeLog contains OOPS!.

[Adrian Perez]

Created attachment 396526 [details]
Patch for landing

[EWS]

Committed r260125: <https://trac.webkit.org/changeset/260125>

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