[webkit-reviews] review denied: [Bug 48509] [GTK] Implement UpdateChunk, ChunkedUpdateDrawingArea/Proxy, WebView classes for WebKit2 and add WKAPI : [Attachment 78233] WebView, WebViewInternal classes implementation for GTK port

Thu Jan 13 14:23:16 PST 2011

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 78233: WebView, WebViewInternal classes implementation for GTK port
https://bugs.webkit.org/attachment.cgi?id=78233&action=review

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

Here are my first impressions. I have listed some specific issues below. I will
do a more thorough review later though.

General: Is WebKitWidgetView going to be part of the public API? If so, it
should go into WebKit2/UIProcess/gtk, and be organized like the other ports.
I'm a little concered with the naming. The rest of the API follows the CF
naming conventions WKEtcEtc and it seems that the other ports try to follow
that as well. Maybe it makes sense to name this something like GTKWKView or
GWKView. WebKitWidgetView implies this a view for holding widgets though. :)

If it allows the code to use WebKit naming conventions everywhere, I think we
can avoid using G_DEFINE_TYPE.

>> WebKit2/UIProcess/gtk/WebViewInternal.cpp:31
>> +static GObjectClass *parentClass = 0;
> 
> You can use G_DEFINE_TYPE() macro that already does this and other things for
you.

Depending on how you decide about G_DEFINE_TYPE, this asterisk needs to go with
the typename.

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:68
> +static void
> +webkitWidgetViewDispose(GObject* obj)

This should be all one line. Please don't abbreviate the variable name.

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:80
> +static void
> +webkitWidgetViewFinalize(GObject* obj)

Ditto.

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:111
> +    g_type_class_add_private(klass, sizeof(WebKitWidgetViewPrivate));

Instead of doing this, it would be better to do it manually. This will allow
you to have the private section that can hold C++ members and properly call
their constructors and desctructors. Take a look at
WebKit/gtk/webkit/webkitwebplugin.cpp for an example of how to do this. We'll
be doign this all over the WebKit 1 API eventually.

>> WebKit2/UIProcess/gtk/WebViewInternal.cpp:126
>> +GType
>> +webkitWidgetViewGetType(void)
> 
> Use G_DEFINE_TYPE macro instead

This should just be one line. Leave out the 'void' for the empty argument list
too, please.

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:130
> +    static GType type = 0; 
> +
> +    if (!type) {

Use an early return here.

> WebKit2/UIProcess/gtk/WebViewInternal.cpp:144
> +	   static const GTypeInfo info = {
> +	       sizeof(WebKitWidgetViewClass),	   /* class structure size */
> +	       0,				   /* base_init */
> +	       0,				   /* base_finalize */
> +	       (GClassInitFunc)webkitWidgetViewClassInit, /* class initializer
*/
> +	       0,				   /* class_finalize */
> +	       0,				   /* class_data */
> +	       sizeof(WebKitWidgetView),	   /* instance structure size
*/
> +	       0,				   /* preallocated instances */

> +	       (GInstanceInitFunc)webkitWidgetViewInit,   /* instance
initializer */
> +	       0				   /* function table */
> +	   };	
> +	   type = g_type_register_static(GTK_TYPE_CONTAINER,
"WebKitWidgetView", &info, (GTypeFlags)0);

Please use static_casts here (or reinterpret_casts where appropriate) instead
of a c-style casts.

> WebKit2/UIProcess/gtk/WebView.cpp:47
> +    gpointer webViewInstance = widgetView->priv->webViewInstance;
> +    WebView* webView = reinterpret_cast<WebView*>(webViewInstance);

I think it's a bad idea to use reinterpret_cast here to get the WebView
instance. Just store the pointer as a WebView* and make the private section
actually private.

> WebKit2/UIProcess/gtk/WebView.cpp:50

Instead of getting the entire clipbox for the region, wouldn't it make sense to
break it up into rectangles and paint them separately. Is that possible? Take a
look at the expose event in webkitwebview.cpp. In any case, I think you should
pass an IntRect to this method.

> WebKit2/UIProcess/gtk/WebView.cpp:59
> +    GObjectClass* parentClass =
(GObjectClass*)g_type_class_peek_parent(WEBKIT_WIDGET_VIEW_GET_CLASS(widget));
> +    GTK_WIDGET_CLASS(parentClass)->size_allocate(widget, allocation);
> +    WebKitWidgetView* widgetView = WEBKIT_WIDGET_VIEW(widget);

Can't you just do widgetView->parentClass?

> WebKit2/UIProcess/gtk/WebView.cpp:61
> +    gpointer webViewInstance = widgetView->priv->webViewInstance;
> +    WebView* webView = reinterpret_cast<WebView*>(webViewInstance);

Same comment as above.

> WebKit2/UIProcess/gtk/WebView.cpp:66
> +gboolean WebView::webViewFocusInEvent(GtkWidget* widget, GdkEventFocus*
event)

Same comments as above in this method.

> WebKit2/UIProcess/gtk/WebView.cpp:86
> +gboolean WebView::webViewFocusOutEvent(GtkWidget* widget, GdkEventFocus*
event)

Ditto.

> WebKit2/UIProcess/gtk/WebView.cpp:102
> +gboolean WebView::webViewKeyPressEvent(GtkWidget* widget, GdkEventKey*
event)

Ditto.

> WebKit2/UIProcess/gtk/WebView.cpp:113
> +gboolean WebView::webViewKeyReleaseEvent(GtkWidget* widget, GdkEventKey*
event)

Ditto.

> WebKit2/UIProcess/gtk/WebView.cpp:127
> +gboolean WebView::webViewButtonPressEvent(GtkWidget* widget, GdkEventButton*
const event)

Ditto.

> WebKit2/UIProcess/gtk/WebView.cpp:138
> +gboolean WebView::webViewButtonReleaseEvent(GtkWidget* widget,
GdkEventButton* event)

Ditto.

> WebKit2/UIProcess/gtk/WebView.cpp:169
> +WebView::WebView(GdkRectangle rect, WebContext* context, WebPageGroup*
pageGroup, GtkWidget*  hostWindow)

You should use a more descriptive parameter name than just "rect." I can't tell
from this patch what it represents. There's an extra space after the final
asterisk.

> WebKit2/UIProcess/gtk/WebView.cpp:179
> +    m_window = (GtkWidget*)g_object_new(WEBKIT_TYPE_WIDGET_VIEW, NULL); 

Use a static_cast here please.

> WebKit2/UIProcess/gtk/WebView.cpp:182
> +    WebKitWidgetView* webView = WEBKIT_WIDGET_VIEW(m_window);
> +    webView->priv->webViewInstance = reinterpret_cast<gpointer>(this);

Here you can just do WEBKIT_WIDGET_VIEW(m_window)->priv->webViewInstance =
this;

> WebKit2/UIProcess/gtk/WebView.cpp:206
> +void WebView::onSetFocusEvent(GtkWidget*, bool)

What's the use of the second parameter? It's never used or named.

> WebKit2/UIProcess/gtk/WebView.cpp:211
> +void WebView::onKillFocusEvent(GtkWidget*, bool)

Ditto.

> WebKit2/UIProcess/gtk/WebView.cpp:224
> +    m_page->handleMouseEvent(mouseEvent);

This can just be:
m_page->handleMouseEvent(WebEventFactory::createWebMouseEvent(event));

> WebKit2/UIProcess/gtk/WebView.cpp:230
> +    WebWheelEvent wheelEvent = WebEventFactory::createWebWheelEvent(event);
> +    m_page->handleWheelEvent(wheelEvent);

Same comment here.

> WebKit2/UIProcess/gtk/WebViewInternal.h:46

Please don't store this anonymously.

> WebKit2/UIProcess/gtk/WebViewInternal.h:47
> +    GtkIMContext *imContext;

The asterisk is in the wrong position here.

> WebKit2/UIProcess/gtk/WebViewInternal.h:54
> +    WebKitWidgetViewPrivate *priv;

Ditto.

> WebKit2/UIProcess/gtk/WebViewInternal.h:62
> +GType
> +webkitWidgetViewGetType(void);

This should be one line.

> WebKit2/UIProcess/gtk/WebView.h:112
> +    GdkRectangle m_rect;

I'd really prefer a more descriptive name for this member. What does the rect
actually represent?