[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]

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
possible
http://bugs.webkit.org/show_bug.cgi?id=14857

Attachment 15794: Make WebKitGtkPage a GtkContainer
http://bugs.webkit.org/attachment.cgi?id=15794&action=edit

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

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

   Please keep the cases indented from the switch.

+    switch (motion->type) {
+    case GDK_MOTION_NOTIFY:

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

+#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
similiar
+ * 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 =
WEBKIT_GTK_PAGE_GET_PRIVATE(WEBKIT_GTK_PAGE(page));

   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.