61065 – [GTK][WebKit2] Initial windowed plugins implementation

RESOLVED FIXED 61065

[GTK][WebKit2] Initial windowed plugins implementation

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

Summary [GTK][WebKit2] Initial windowed plugins implementation

Carlos Garcia Campos

Reported 2011-05-18 09:12:11 PDT

Windowed plugins are currently not implemented in WebKit2 for the GTK port.

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Carlos Garcia Campos

Comment 1 2011-05-18 09:14:04 PDT

Created attachment 93924 [details] Patch

Early Warning System Bot

Comment 2 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

Carlos Garcia Campos

Comment 3 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

Early Warning System Bot

Comment 4 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

Carlos Garcia Campos

Comment 5 2011-07-15 06:14:54 PDT

Created attachment 100963 [details] Patch updated for current git master

Carlos Garcia Campos

Comment 6 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

Early Warning System Bot

Comment 7 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

Martin Robinson

Comment 8 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?

Carlos Garcia Campos

Comment 9 2011-11-30 06:16:12 PST

Created attachment 117182 [details] New patch

WebKit Review Bot

Comment 10 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

Philippe Normand

Comment 11 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"

Carlos Garcia Campos

Comment 12 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.

Martin Robinson

Comment 13 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.

Carlos Garcia Campos

Comment 14 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.

Balazs Kelemen

Comment 15 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.

Carlos Garcia Campos

Comment 16 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.

Carlos Garcia Campos

Comment 17 2012-04-25 05:32:28 PDT

Created attachment 138789 [details] Updated patch according to review comments

Philippe Normand

Comment 18 2012-04-26 02:16:48 PDT

Comment on attachment 138789 [details] Updated patch according to review comments Ok! Seems quite good to me :)

Carlos Garcia Campos

Comment 19 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.

Carlos Garcia Campos

Comment 20 2012-04-26 03:57:23 PDT

Committed r115300: <http://trac.webkit.org/changeset/115300>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware PC

OS Linux

Product WebKit

Component WebKit2

Assignee

Nobody

Reported

2011-05-18 09:12 PDT

Modified

2012-04-26 03:57 PDT History

CC List

8 users Show

URL

Keywords Gtk

Depends on

60629

Blocks

55659

Dependencies

tree graph