Bug 70190 – [GTK] Avoid unecessarily calling gtk_widget_size_allocate on plugin widgets

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

gtk_widget_size_allocate is called on plugin widgets that are scrolled off the page or haven't changed position. Since gtk_widget_size_allocate is an expensive operation we should only call it when necessary. This causes poor scrolling performance on pages with many plugins.

[Martin Robinson]

Created attachment 111159 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 111159 [details]
Patch

LGTM

[Xan Lopez]

Comment on attachment 111159 [details]
Patch

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

> Source/WebCore/plugins/gtk/PluginViewGtk.cpp:517
> +        m_windowRect = IntRect(-1, -1, 1, 1);

My understanding is that -1, -1 as an allocation is a sort of magic value that you are not meant to use from "user space".

> Source/WebCore/plugins/gtk/PluginViewGtk.cpp:581
> +    // gtk_widget_size_allocate is expensive, so don't call it unless the allocation has changed.

Perhaps it's better to note here that we don't care about some things size_allocate does even when the allocation is the same, like invalidating the area.

[Martin Robinson]

(In reply to comment #3)
> (From update of attachment 111159 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111159&action=review
> 
> > Source/WebCore/plugins/gtk/PluginViewGtk.cpp:517
> > +        m_windowRect = IntRect(-1, -1, 1, 1);
> 
> My understanding is that -1, -1 as an allocation is a sort of magic value that you are not meant to use from "user space".

Is there some place that talks about this? From a brief look at the GTK+ source code, this just seems like the allocation GTK+ uses when creating a widget. I also found in testing that GTK+ would return this allocation if I placed the widget anywhere offscreen. I do not think it's a problem to use a weird allocation, since we are implementing our own container.

> > Source/WebCore/plugins/gtk/PluginViewGtk.cpp:581
> > +    // gtk_widget_size_allocate is expensive, so don't call it unless the allocation has changed.
> 
> Perhaps it's better to note here that we don't care about some things size_allocate does even when the allocation is the same, like invalidating the area.

Okay. I can expand this comment to indicate that Flash is drawing into the window so we don't care what size_allocate does wrt to invalidations.

[Xan Lopez]

Comment on attachment 111159 [details]
Patch

I'm fine with this being committed, just try to double check sometime if what we are doing with allocations is kosher.

[Martin Robinson]

Created attachment 111401 [details]
Patch with no magic allocation

[Martin Robinson]

(In reply to comment #5)
> (From update of attachment 111159 [details])
> I'm fine with this being committed, just try to double check sometime if what we are doing with allocations is kosher.

I couldn't raise any expert opinions on #gtk+, so I rewrote the patch to remove the funky allocation. I think it's actually a little more explicit this way. Do you mind taking another look?

[Xan Lopez]

Comment on attachment 111401 [details]
Patch with no magic allocation

This makes sense to me.

[Martin Robinson]

Committed r97865: <http://trac.webkit.org/changeset/97865>

[Philippe Normand]

It seems this patch broke plugins/resize-from-plugin.html
Double checking now...

--- /home/slave/webkitgtk/gtk-linux-64-debug/build/layout-test-results/plugins/resize-from-plugin-expected.txt 
+++ /home/slave/webkitgtk/gtk-linux-64-debug/build/layout-test-results/plugins/resize-from-plugin-actual.txt 
@@ -7,6 +7,6 @@
 
 Test for NPP_SetWindow calls sent while a plug-in resizes itself.
 
-x: 0, y: 0, width: 200, height: 200, clipRect: (0, 0, 0, 0)
+x: 0, y: 0, width: 200, height: 200, clipRect: (0, 0, 200, 142)
 Height and width should equal 200, and the plug-in size should not change on scroll.

[Philippe Normand]

(In reply to comment #10)
> It seems this patch broke plugins/resize-from-plugin.html
> Double checking now...

Yep, confirmed! What should we do then?

[Philippe Normand]

(In reply to comment #11)
> (In reply to comment #10)
> > It seems this patch broke plugins/resize-from-plugin.html
> > Double checking now...
> 
> Yep, confirmed! What should we do then?

See bug 70481