Bug 13913 – FrameView::clear doesn't reset m_slowRepaintObjectCount leading to 'sticky' static background

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Holger Freyther]

While exploring static background handling on the Gdk port. I found the following issue and I think it applies to all ports.

Behaviour: Visit a site with a non static background, e.g http://www.kde.org move to developer.gnome.org/doc/API/2.0/gtk/GtkWidget.html which has a static background and then go back. On going back the static background is temporarily 'deleted' from FrameView::clear but it gets reset by FraneView::layout due useSlowRepaint() returning true.


The two backtraces should illustrate the behaviour:
#0  WebCore::ScrollView::setStaticBackground (this=0x12e5c980, flag=false) at ../../../WebCore/platform/gdk/ScrollViewGdk.cpp:232
#1  0x0124b2c1 in WebCore::FrameView::clear (this=0x12e5c980) at ../../../WebCore/page/FrameView.cpp:184
#2  0x0120e5e3 in WebCore::FrameLoader::clear (this=0x1301ea00, clearWindowProperties=true) at ../../../WebCore/loader/FrameLoader.cpp:781
#3  0x01213615 in WebCore::FrameLoader::begin (this=0x1301ea00, url=@0x1301eba0) at ../../../WebCore/loader/FrameLoader.cpp:841

And then on layout:
#0  WebCore::ScrollView::setStaticBackground (this=0x12e5c980, flag=true) at ../../../WebCore/platform/gdk/ScrollViewGdk.cpp:232
#1  0x0124c73b in WebCore::FrameView::layout (this=0x12e5c980, allowSubtree=true) at ../../../WebCore/page/FrameView.cpp:467
#2  0x01330328 in WebCore::FrameGdk::handleGdkEvent (this=0x12e56d30, event=0xbfffd4c0) at ../../../WebCore/platform/gdk/FrameGdk.cpp:145

As you see on ::layout setStaicBackground(true) is getting called even if the newly loaded page does not require it. This is due FrameViewPrivate::reset not resetting m_slowRepaintObjectCount to zero. I can't judge what breaks when setting m_slowRepaintObjectCount to zero on a reset, at least ScrollView::setStaticBackground seems to have the 'right' value.

(Not creating a patch yet, I hope this gets reviewed anyway)

[Holger Freyther]

Okay, this bug doesn't happen with the Mac port.

[Holger Freyther]

It works on the mac because Safari is creating a new FrameView for each site. I have added the following patch and the amount of failed layout tests didn't change.
I believe this patch is correct, it is a way without throwing away a frameview to disable static backgrounds when leaving a site that required them.

Three things to consider:
   -Maybe it is better to make sure FrameView::removeSlowRepaintObject gets called often enough from FrameLoader::clear?
   -Check if more attributes needs to be reset.
   -Make it mandantory to recreate the FrameView?




Index: WebCore/page/FrameView.cpp
===================================================================
--- WebCore/page/FrameView.cpp  (Revision 21902)
+++ WebCore/page/FrameView.cpp  (Arbeitskopie)
@@ -71,6 +71,7 @@
     }
     void reset()
     {
+        m_slowRepaintObjectCount = 0;
         useSlowRepaints = false;
         borderX = 30;
         borderY = 30;

(In reply to comment #2)
> +        m_slowRepaintObjectCount = 0;

I think that will reintroduce bug 13037.

[Holger Freyther]

(In reply to comment #3)
> (In reply to comment #2)
> > +        m_slowRepaintObjectCount = 0;
>
> I think that will reintroduce bug 13037.

Looking at the patch of #13037 I see that this was removed from the reset. From my test I saw that Safari creates  a new FrameView und URL navigation (back,forward) anyway. So we will end up with a performance penatly on ports not throwing away their FrameView.

So the only possible solution I can see not reintroducing bug #13037 is to somehow call  FrameView::removeSlowRepaintObject() (often enough) when leaving a site requiring static background. I assume that this is not easily possible?

Any ideas and comments would be appreciated

[Holger Freyther]

Summary of a small discussion with mitz ending with the following proposal:

Reimplement RenderObject::destroy in RenderView. Add a new method to FrameView resetting the m_slowRepaintObjectCount to zero. And make RenderView::destroy call this method on the view()->frameView().
This solution shouldn't regress as RenderView::destroy is not called on Pages in the b/f Cache.

[Holger Freyther]

Created attachment 14868 [details]
Reset FrameView::m_slowRepaintObjectCount from RenderView::destroy

Implement the idea of Mitz as described in the previous comment. I will run the layout tests and will report the result.

[Holger Freyther]

(In reply to comment #6)
> Implement the idea of Mitz as described in the previous comment. I will run the
> layout tests and will report the result.
> 

Okay, the number of failed tests didn't change for me.

Comment on attachment 14868 [details]
Reset FrameView::m_slowRepaintObjectCount from RenderView::destroy

I think this approach is good for fixing this bug (no surprise there), but I think resetSlowRepaintObject is not a good name. removeAllSlowRepaintObjcets is more in line with the other two function names.

However, Hyatt has suggested that a better approach would be not to reuse FrameViews for new page loads, as the Mac port doesn't do it, so there may be other code that assumes that it's not done.

[Dave Hyatt]

Comment on attachment 14868 [details]
Reset FrameView::m_slowRepaintObjectCount from RenderView::destroy

Would rather hold off on this for now.

[Maciej Stachowiak]

(I think a change to not reuse the FrameView would probably be ok, since it would not affect the core code. Having a WebKit-like API layer woudl help with this.)

[Holger Freyther]

I agree with comment #9 and #10. Would it make sense to make not reusing FrameView a bit harder? I wonder what use a FrameView::clear method has when one isn't recycling FrameViews?

(Feel free to close this bug)

(In reply to comment #11)
> I wonder what use a FrameView::clear method has when
> one isn't recycling FrameViews?

I don't think it should exist if you enforce a no-recycling policy.

[Holger Freyther]

The FrameView should not be reused... See ChromeClientGtk.cpp...