RESOLVED FIXED 70190
[GTK] Avoid unecessarily calling gtk_widget_size_allocate on plugin widgets
https://bugs.webkit.org/show_bug.cgi?id=70190
Summary [GTK] Avoid unecessarily calling gtk_widget_size_allocate on plugin widgets
Martin Robinson
Reported 2011-10-15 17:56:00 PDT
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.
Attachments
Patch (3.21 KB, patch)
2011-10-15 18:02 PDT, Martin Robinson
no flags
Patch with no magic allocation (3.16 KB, patch)
2011-10-18 00:26 PDT, Martin Robinson
xan.lopez: review+
Martin Robinson
Comment 1 2011-10-15 18:02:53 PDT
Carlos Garcia Campos
Comment 2 2011-10-17 00:03:55 PDT
Comment on attachment 111159 [details] Patch LGTM
Xan Lopez
Comment 3 2011-10-17 00:27:39 PDT
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
Comment 4 2011-10-17 00:40:19 PDT
(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 5 2011-10-17 00:47:23 PDT
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
Comment 6 2011-10-18 00:26:02 PDT
Created attachment 111401 [details] Patch with no magic allocation
Martin Robinson
Comment 7 2011-10-18 00:27:18 PDT
(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 8 2011-10-18 00:56:50 PDT
Comment on attachment 111401 [details] Patch with no magic allocation This makes sense to me.
Martin Robinson
Comment 9 2011-10-19 08:24:14 PDT
Philippe Normand
Comment 10 2011-10-19 23:47:35 PDT
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
Comment 11 2011-10-20 00:41:25 PDT
(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
Comment 12 2011-10-20 00:48:25 PDT
(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
Note You need to log in before you can comment on or make changes to this bug.