Bug 14781 – [gtk] Make WebKitGtkPage not connect to its own signals but set/override the default handlers of GtkWidgetClass

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Holger Freyther]

As pointed out by xan now that WebKitGtkPage is a GtkLayout itself we should not connect the signals but properly set/override the default handlers.

The signatures of the signals we are interested in use the specific GdkEvent* structure instead of the GdkEvent union requiring adjustments to Platform*Event in WebCore.

The event handling code was moved from FrameGdk to WebKitGtkPage and at that time WebCore/platform/gdk/FrameGdk.{cpp,h} got killed and the needed stubs got moved to WebCore/page/gdk/FrameGdk.cpp. Some more code was moved to WebKitGtkPage (settings) and a semi internal method was moved to ImageGdk.cpp as it is the only place using this stub ATM.

[Holger Freyther]

Created attachment 15703 [details]
Use default handlers and adjust WebCore to make that happen

Primary Goal: Use GtkWidgetClass default handlers instead of connecting to our own signals

Kill FrameGdk, reintroduce it as stubs. Move FrameGdk methods to WebKitGtkPage, ImageGdk. Adjust Platform*Event to use GdkEvent* structure.

[Xan Lopez]

Looks very good!
+static gboolean webkit_gtk_page_expose_event(GtkWidget* widget, GdkEventExpose* event)
+{
+    WebCore::Frame* frame = core(getFrameFromPage(WEBKIT_GTK_PAGE(widget)));
+    GdkRectangle clip;
+    gdk_region_get_clipbox(event->region, &clip);
+    gdk_window_begin_paint_region(event->window, event->region);

Why calling gdk_window_begin_paint_region? All drawing in GTK is double buffered by default, and I don't see you disabling it.

+    gtk_container_forall(GTK_CONTAINER(widget), frame_gdk_expose_child, &data);

GtkLayout should be doing this for you already, it isn't? Although I have to admit I don't get what you do in the handler, so...

+    gdk_window_end_paint(event->window);
+
+    return TRUE;

Maybe you want to return FALSE in the expose handler? It's not uncommon to connect to expose-event with g_signal_connect_after to do anything on the already drawn surface, returning TRUE it the default handler you are won't allow it.

[Xan Lopez]

> +    gtk_container_forall(GTK_CONTAINER(widget), frame_gdk_expose_child,
> &data);
> 
> GtkLayout should be doing this for you already, it isn't? Although I have to
> admit I don't get what you do in the handler, so...

Actually, you need to chain with your parent's implementation of course :)

[Holger Freyther]

Comment on attachment 15703 [details]
Use default handlers and adjust WebCore to make that happen

Included in #14729

[Holger Freyther]

Comment on attachment 15703 [details]
Use default handlers and adjust WebCore to make that happen

Obsoleting it as well

[Holger Freyther]

Mostly applied with patches on the 8th of August 2007.