Bug 14781 - [gtk] Make WebKitGtkPage not connect to its own signals but set/override the default handlers of GtkWidgetClass
Summary: [gtk] Make WebKitGtkPage not connect to its own signals but set/override the ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 14797
  Show dependency treegraph
 
Reported: 2007-07-27 04:47 PDT by Holger Freyther
Modified: 2007-08-08 07:53 PDT (History)
0 users

See Also:


Attachments
Use default handlers and adjust WebCore to make that happen (55.16 KB, patch)
2007-07-27 04:51 PDT, Holger Freyther
zecke: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Freyther 2007-07-27 04:47:15 PDT
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.
Comment 1 Holger Freyther 2007-07-27 04:51:13 PDT
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.
Comment 2 Xan Lopez 2007-07-29 10:36:35 PDT
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.
Comment 3 Xan Lopez 2007-07-29 10:43:50 PDT
> +    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 :)
Comment 4 Holger Freyther 2007-07-30 12:46:28 PDT
Comment on attachment 15703 [details]
Use default handlers and adjust WebCore to make that happen

Included in #14729
Comment 5 Holger Freyther 2007-07-30 12:47:30 PDT
Comment on attachment 15703 [details]
Use default handlers and adjust WebCore to make that happen

Obsoleting it as well
Comment 6 Holger Freyther 2007-08-08 07:53:02 PDT
Mostly applied with patches on the 8th of August 2007.