[webkit-reviews] review denied: [Bug 61065] [GTK] Initial windowed plugins implementation : [Attachment 100963] Patch updated for current git master

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 61065: [GTK] Initial windowed plugins implementation
https://bugs.webkit.org/show_bug.cgi?id=61065

Attachment 100963: Patch updated for current git master
https://bugs.webkit.org/attachment.cgi?id=100963&action=review

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


This looks good to me, but I have a few suggestions.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:53
> +    GList* children;

I'd prefer a Vector here. You might be able to avoid manual memory management
entirely.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:62
> +typedef struct _WebKitWebViewChild {

I think you can just do

struct WebKitWebViewChild {
...
}

and that's more idiomatically C++.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:114
> +    WebKitWebViewChild* viewChild = g_slice_new(WebKitWebViewChild);

It'd be better to use the WTF allocator here, though even better if we can use
a Vector + OwnPtr.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:131
> +    for (GList* children = priv->children; children; children =
children->next) {

It might be better to use gtk_container_forall here, if possible.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:144
> +	       if (wasVisible && gtk_widget_get_visible(widgetContainer))
> +		   gtk_widget_queue_resize(widgetContainer);

It makes sense to move this out of the loop. Actually it seems if you use a
Vector there will be no loop.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:185
> +    WebKitWebViewChild* viewChild = webkitWebViewBaseGetChild(webView,
child);
> +
> +    if (viewChild->geometry == childRect)
> +	   return;
> +
> +    viewChild->geometry = childRect;
> +    gtk_widget_queue_resize_no_redraw(GTK_WIDGET(webView));

Here it might be good to do similar checks to what we do in WebKit1 now.  For
instance:
1. If the child was off the screen and is still off the screen, don't do
anything.
2. If the new allocation is the same as the old one, don't do anything.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:247
> +    for (GList* children = priv->children; children; children =
children->next) {

this can also be gtk_widget_forall.

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:179
> +    if (m_isWindowed) {

Please split out the windowed and windowless paths into helper functions.

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:192
> +	       m_npWindow.window =
(void*)gtk_socket_get_id(GTK_SOCKET(socket));

GINT_TO_POINTER?