Bug 210463

Created attachment 396354 [details] Patch

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

Created attachment 396406 [details] Patch v2

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

(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 👌️

(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 > > 👌️

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

Created attachment 396525 [details] Patch for landing

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

Created attachment 396526 [details] Patch for landing

Committed r260125: <https://trac.webkit.org/changeset/260125> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396526 [details].