[webkit-reviews] review denied: [Bug 48509] [GTK] Implement UpdateChunk, ChunkedUpdateDrawingArea/Proxy, WebView classes for WebKit2 and add WKAPI : [Attachment 78945] WebView & WebKitWebView implementation with review comments incorporation

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Ravi Phaneendra Kasibhatla
<ravi.kasibhatla at motorola.com>'s request for review:
Bug 48509: [GTK] Implement UpdateChunk, ChunkedUpdateDrawingArea/Proxy, WebView
classes for WebKit2 and add WKAPI
https://bugs.webkit.org/show_bug.cgi?id=48509

Attachment 78945: WebView & WebKitWebView implementation with review comments
incorporation
https://bugs.webkit.org/attachment.cgi?id=78945&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78945&action=review

Looking better! I'm slightly concerned with the name of the GTK+ widget because
it collides with the WebKit1 naming. I'm not sure if that would cause linking
issues or not (some older libraries might link against WebKit1 for
JavaScriptCore. I know it's unusual, but maybe we should consider the WKView
naming convention to match other ports.

> WebKit2/UIProcess/gtk/WebKitWebView.cpp:51
> +#if GTK_CHECK_VERSION(2, 18, 0)
> +    gtk_widget_get_allocation(widget, &allocation);
> +#else
> +    allocation = widget->allocation;
> +#endif

Here you can just include GtkVersioning.h from WebCore and only call
gtk_widget_get_allocation(...)

> WebKit2/UIProcess/gtk/WebKitWebView.cpp:168
> +	       sizeof(WebKitWebViewClass),	    /* class structure size */
> +	       0,				   /* base_init */
> +	       0,				   /* base_finalize */
> +	       reinterpret_cast<GClassInitFunc>(webkitWebViewClassInit), /*
class initializer */
> +	       0,				   /* class_finalize */
> +	       0,				   /* class_data */
> +	       sizeof(WebKitWebView),		    /* instance structure size
*/
> +	       0,				   /* preallocated instances */

> +	       reinterpret_cast<GInstanceInitFunc>(webkitWebViewInit),	 /*
instance initializer */
> +	       0				   /* function table */
> +    };   

This should be a four space indent. Nit!: do you mind lining up the comments?
:)

> WebKit2/UIProcess/gtk/WebView.h:61
> +#ifdef GTK_API_VERSION_2
> +    static gboolean webViewOnExpose(GtkWidget*, GdkEventExpose*);
> +#else
> +    static gboolean webViewOnDraw(GtkWidget*, cairo_t*);
> +#endif
> +    static void webViewOnSizeAllocate(GtkWidget*, GtkAllocation*);
> +    static gboolean webViewFocusInEvent(GtkWidget*, GdkEventFocus*);
> +    static gboolean webViewFocusOutEvent(GtkWidget*, GdkEventFocus*);
> +    static gboolean webViewKeyPressEvent(GtkWidget*, GdkEventKey*);
> +    static gboolean webViewKeyReleaseEvent(GtkWidget*, GdkEventKey*);
> +    static gboolean webViewButtonPressEvent(GtkWidget*, GdkEventButton*);
> +    static gboolean webViewButtonReleaseEvent(GtkWidget*, GdkEventButton*);
> +    static gboolean webViewScrollEvent(GtkWidget*, GdkEventScroll*);
> +    static gboolean webViewMotionNotifyEvent(GtkWidget*, GdkEventMotion*);

These seem to be GObject methods that are really only used in
WebKitWebView.cpp. I think it might make more sense to declare them statically
inside the GObject implementation file.

Ah, I see you have them here, because the methods on WebView are private.
Perhaps a better approach would be to make these methods on the private data
section of the GObject and make the private data struct a friend class of
WebView. Either that or make the WebView:onWhatever methods public. Technically
you are calling them from another class.

> WebKit2/UIProcess/gtk/WebView.h:118
> +    GdkRectangle m_windowRect;

Is this the rectangle that represents the entire window or just the viewport?
If it's the viewport, I think m_viewRect or m_viewportRect makes more sense.