[Webkit-unassigned] [Bug 48509] [GTK] Implement UpdateChunk, ChunkedUpdateDrawingArea/Proxy, WebView classes for WebKit2 and add WKAPI

Thu Jan 13 14:23:18 PST 2011

https://bugs.webkit.org/show_bug.cgi?id=48509

Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #78233|review?                     |review-
               Flag|                            |

--- Comment #13 from Martin Robinson <mrobinson at webkit.org>  2011-01-13 14:23:17 PST ---
(From update of attachment 78233)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.