[Webkit-unassigned] [Bug 61065] [GTK] Initial windowed plugins implementation

[bugzilla-daemon at webkit.org]

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


Martin Robinson <mrobinson at webkit.org> changed:

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




--- Comment #8 from Martin Robinson <mrobinson at webkit.org>  2011-11-23 04:37:28 PST ---
(From update of attachment 100963)
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?

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