Summary: | [GTK][WebKit2] Initial windowed plugins implementation | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | gustavo, kbalazs, l.slachciak, menard, mrobinson, pnormand, webkit.review.bot, zoltan | ||||||||||||
Priority: | P2 | Keywords: | Gtk | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Linux | ||||||||||||||
Bug Depends on: | 60629 | ||||||||||||||
Bug Blocks: | 55659 | ||||||||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2011-05-18 09:12:11 PDT
Created attachment 93924 [details]
Patch
Comment on attachment 93924 [details] Patch Attachment 93924 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8708600 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 on attachment 94063 [details] Updated patch Attachment 94063 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8710964 Created attachment 100963 [details]
Patch updated for current git master
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 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 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? Created attachment 117182 [details]
New patch
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 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" (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 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. (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 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. (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. Created attachment 138789 [details]
Updated patch according to review comments
Comment on attachment 138789 [details]
Updated patch according to review comments
Ok! Seems quite good to me :)
(In reply to comment #18) > (From update of attachment 138789 [details]) > Ok! Seems quite good to me :) Thank you very much for reviewing it. Committed r115300: <http://trac.webkit.org/changeset/115300> |