Bug 61065 - [GTK][WebKit2] Initial windowed plugins implementation
Summary: [GTK][WebKit2] Initial windowed plugins implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 60629
Blocks: 55659
  Show dependency treegraph
 
Reported: 2011-05-18 09:12 PDT by Carlos Garcia Campos
Modified: 2012-04-26 03:57 PDT (History)
8 users (show)

See Also:


Attachments
Patch (27.60 KB, patch)
2011-05-18 09:14 PDT, Carlos Garcia Campos
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Updated patch (26.51 KB, patch)
2011-05-19 05:43 PDT, Carlos Garcia Campos
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch updated for current git master (26.25 KB, patch)
2011-07-15 06:14 PDT, Carlos Garcia Campos
mrobinson: review-
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
New patch (25.84 KB, patch)
2011-11-30 06:16 PST, Carlos Garcia Campos
pnormand: review-
Details | Formatted Diff | Diff
Updated patch according to review comments (27.60 KB, patch)
2012-04-25 05:32 PDT, Carlos Garcia Campos
pnormand: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-05-18 09:12:11 PDT
Windowed plugins are currently not implemented in WebKit2 for the GTK port.
Comment 1 Carlos Garcia Campos 2011-05-18 09:14:04 PDT
Created attachment 93924 [details]
Patch
Comment 2 Early Warning System Bot 2011-05-18 09:25:39 PDT
Comment on attachment 93924 [details]
Patch

Attachment 93924 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8708600
Comment 3 Carlos Garcia Campos 2011-05-19 05:43:41 PDT
Created attachment 94063 [details]
Updated patch

 - Remove build fix, I filed a separate bug report, see bug #61113
 - Simplify webkitWebViewBaseChildMoveResize()
 - Fill the changelog a bit more
Comment 4 Early Warning System Bot 2011-05-19 05:54:12 PDT
Comment on attachment 94063 [details]
Updated patch

Attachment 94063 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8710964
Comment 5 Carlos Garcia Campos 2011-07-15 06:14:54 PDT
Created attachment 100963 [details]
Patch updated for current git master
Comment 6 Carlos Garcia Campos 2011-07-15 06:17:07 PDT
To test the patch I used a simple html file containing only:

<html>
<body>
    <iframe src="flash.swf"></iframe>
</body>
</html>

and this flash file downloaded in the same dir:

http://www.adobe.com/shockwave/welcome/flash.swf
Comment 7 Early Warning System Bot 2011-07-15 06:29:37 PDT
Comment on attachment 100963 [details]
Patch updated for current git master

Attachment 100963 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9099207
Comment 8 Martin Robinson 2011-11-23 04:37:28 PST
Comment on attachment 100963 [details]
Patch updated for current git master

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?
Comment 9 Carlos Garcia Campos 2011-11-30 06:16:12 PST
Created attachment 117182 [details]
New patch
Comment 10 WebKit Review Bot 2011-11-30 06:23:08 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 11 Philippe Normand 2012-04-24 07:25:25 PDT
Comment on attachment 117182 [details]
New patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117182&action=review

Patch looks good to me apart from some nits listed below. Also not sure about the Message changes, do they need to be approved by other WebKit2 reviewers? If so please CC the appropriate people :)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:126
> +    priv->children.set(widget, childAllocation);

Would it make sense to convert the GtkAllocation to an IntRect? I know they're quite similar but anyway :)

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:146
> +static void webkitWebViewBaseContainerForall(GtkContainer *container, gboolean includeInternals, GtkCallback callback, gpointer callbackData)

star on the wrong side it seems?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:231
> +    IntRect geometry(priv->children.get(child));

A different assignment style is used in webkitWebViewBaseChildMoveResize. Can the code be more coherent in that regard, if possible?

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:235
> +    GtkAllocation childAllocation(geometry);

Shouldn't a C-style assignment be used here?

> Source/WebKit2/UIProcess/gtk/WebPageProxyGtk.cpp:86
> +        GdkRectangle clip(clipRect);

Ditto about assignment style.

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:164
> +    // I guess it uses gdk_window_lookup(), se we create a new socket here

typo: "so we"
Comment 12 Carlos Garcia Campos 2012-04-24 07:53:54 PDT
(In reply to comment #11)
> (From update of attachment 117182 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117182&action=review
> 
> Patch looks good to me apart from some nits listed below.

Thanks for reviewing!

> Also not sure about the Message changes, do they need to be approved by other WebKit2 reviewers? If so please CC the appropriate people :)

I haven't changed any messages, just added a new one needed by ports using X11 plugin arch.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:126
> > +    priv->children.set(widget, childAllocation);
> 
> Would it make sense to convert the GtkAllocation to an IntRect? I know they're quite similar but anyway :)

It's converted, the hashmap stores IntRects.

typedef HashMap<GtkWidget*, IntRect> WebKitWebViewChildrenMap;

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:146
> > +static void webkitWebViewBaseContainerForall(GtkContainer *container, gboolean includeInternals, GtkCallback callback, gpointer callbackData)
> 
> star on the wrong side it seems?

right.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:231
> > +    IntRect geometry(priv->children.get(child));
> 
> A different assignment style is used in webkitWebViewBaseChildMoveResize. Can the code be more coherent in that regard, if possible?

Sure

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:235
> > +    GtkAllocation childAllocation(geometry);
> 
> Shouldn't a C-style assignment be used here?

why? does the coding style say something regarding assignments? I don't mind using either one, just wondering if there's a reason to prefer C-style one.

> > Source/WebKit2/UIProcess/gtk/WebPageProxyGtk.cpp:86
> > +        GdkRectangle clip(clipRect);
> 
> Ditto about assignment style.
> 
> > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:164
> > +    // I guess it uses gdk_window_lookup(), se we create a new socket here
> 
> typo: "so we"

Ok.
Comment 13 Martin Robinson 2012-04-24 08:30:18 PDT
Comment on attachment 117182 [details]
New patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117182&action=review

>>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:231
>>> +    IntRect geometry(priv->children.get(child));
>> 
>> A different assignment style is used in webkitWebViewBaseChildMoveResize. Can the code be more coherent in that regard, if possible?
> 
> Sure

Typically, assignment in WebKit uses the C syntax. I'm not sure if the entire code base follows this rule. I've definitely seen other reviewers express this desire before though, so it's best to stick with what's typical in the project, I think. I believe this could be considered one of the unwritten rules.
Comment 14 Carlos Garcia Campos 2012-04-24 08:38:42 PDT
(In reply to comment #13)
> (From update of attachment 117182 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117182&action=review
> 
> >>> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:231
> >>> +    IntRect geometry(priv->children.get(child));
> >> 
> >> A different assignment style is used in webkitWebViewBaseChildMoveResize. Can the code be more coherent in that regard, if possible?
> > 
> > Sure
> 
> Typically, assignment in WebKit uses the C syntax. I'm not sure if the entire code base follows this rule. I've definitely seen other reviewers express this desire before though, so it's best to stick with what's typical in the project, I think. I believe this could be considered one of the unwritten rules.

Ok, I'll use the C-style then, thanks for the clarification.
Comment 15 Balazs Kelemen 2012-04-25 01:02:45 PDT
Comment on attachment 117182 [details]
New patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117182&action=review

> Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:229
> +bool NetscapePlugin::platformPostInitialize()
> +{
> +#if PLATFORM(GTK)
> +    if (moduleMixesGtkSymbols(m_pluginModule->module()))
> +        return false;
> +#endif
> +
> +    uint64_t windowID = 0;

I think we should still short-circuit the windowed case for Qt since we don't support it.
Comment 16 Carlos Garcia Campos 2012-04-25 05:12:21 PDT
(In reply to comment #15)
> (From update of attachment 117182 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117182&action=review
> 
> > Source/WebKit2/WebProcess/Plugins/Netscape/x11/NetscapePluginX11.cpp:229
> > +bool NetscapePlugin::platformPostInitialize()
> > +{
> > +#if PLATFORM(GTK)
> > +    if (moduleMixesGtkSymbols(m_pluginModule->module()))
> > +        return false;
> > +#endif
> > +
> > +    uint64_t windowID = 0;
> 
> I think we should still short-circuit the windowed case for Qt since we don't support it.

The patch shouldn't break Qt, if windowID is 0 (which will be the case of Qt until it's implemented), it returns early.
Comment 17 Carlos Garcia Campos 2012-04-25 05:32:28 PDT
Created attachment 138789 [details]
Updated patch according to review comments
Comment 18 Philippe Normand 2012-04-26 02:16:48 PDT
Comment on attachment 138789 [details]
Updated patch according to review comments

Ok! Seems quite good to me :)
Comment 19 Carlos Garcia Campos 2012-04-26 03:17:58 PDT
(In reply to comment #18)
> (From update of attachment 138789 [details])
> Ok! Seems quite good to me :)

Thank you very much for reviewing it.
Comment 20 Carlos Garcia Campos 2012-04-26 03:57:23 PDT
Committed r115300: <http://trac.webkit.org/changeset/115300>