[webkit-reviews] review granted: [Bug 14857] [gtk] ScrollView and WebKitGtkPage changes to make multiple frames possible : [Attachment 15794] Make WebKitGtkPage a GtkContainer

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Aug 6 12:13:14 PDT 2007

Adam Roben <aroben at apple.com> has granted Holger Freyther
<freyther at handhelds.org>'s request for review:
Bug 14857: [gtk] ScrollView and WebKitGtkPage changes to make multiple frames

Attachment 15794: Make WebKitGtkPage a GtkContainer

------- Additional Comments from Adam Roben <aroben at apple.com>
This patch looks like it should be N separate checkins:

1. FrameGdk move and changes
2. Changes to the various *Event classes
3. WebKitGtkPage changes

It would have been nice if this patch could have been broken up into those
pieces before putting it up for review. It would have made it much easier to

     switch (event->type) {
-	     m_eventType = MouseEventMoved;
-	     m_button = NoButton;
+    case GDK_2BUTTON_PRESS:
+    case GDK_3BUTTON_PRESS:

   Please keep the cases indented from the switch.

+    switch (motion->type) {

   Ditto here.

+    g_object_ref(event->expose.window);

   Do you need to release this ref?

+    gdk_event_free (event);

   Please remove the space before the (.

+ * TODO: We should follow Qt and move that to webkitgtksettings.

   Typo: that -> this

+GObject* webkit_gtk_frame_new(WebKitGtkPage* web_page)

Since we want to move towards the WebKit coding style, the parameter name
should be webPage (and other variables in this method should be named

+#include "PlatformMouseEvent.h"
+#include "PlatformKeyboardEvent.h"
+#include "PlatformWheelEvent.h"
+#include "GraphicsContext.h"
+#include "EventHandler.h"
+#include "HitTestRequest.h"
+#include "HitTestResult.h"

   Please keep #includes ordered alphabetically.

+    frame->view()->wheelEvent(wheelEvent);

I believe this should go through EventHandler::handleWheelEvent.

+    HitTestRequest hitTestRequest(true, true);
+    HitTestResult hitTestResult(wheelEvent.pos());
+    frame->renderer()->layer()->hitTest(hitTestRequest, hitTestResult);
+    Node* node = hitTestResult.innerNode();
+    if (!node)
+	 return FALSE;
+    return TRUE;

I'm not sure why this code is needed.

+    frame->forceLayout();
+    frame->view()->adjustViewSize();

   These two lines aren't needed on Windows. Why are they needed here?

+ * it is a GtkContainer to implement ScrollView. And we need to make it
+ * to GtkLayout to have sane painting.

   It would be good to say in what way this code makes WebKitGtkPage similar to
GtkLayout, and what "sane" means.

+	     gtk_widget_map((*current));

   Looks like you've got some extra parentheses here.

+    if (private_data->children.contains(widget)) {

   You should just return early if this condition is false.

+    G_OBJECT_CLASS (webkit_gtk_page_parent_class)->finalize (object);

   You've got extra spaces before your parentheses.

+    WebKitGtkPagePrivate *private_data =

   Please use camel case.

+    /*
+     * TODO: move that over to use webkitgtksettings 
+     */

   This should be a FIXME.

   This looks fine overall, though again I'm no Gtk expert.

More information about the webkit-reviews mailing list