Bug 13913

Summary: FrameView::clear doesn't reset m_slowRepaintObjectCount leading to 'sticky' static background
Product: WebKit Reporter: Holger Freyther <zecke>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: mitz
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
Reset FrameView::m_slowRepaintObjectCount from RenderView::destroy hyatt: review-

Description Holger Freyther 2007-05-29 16:00:37 PDT
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)
Comment 1 Holger Freyther 2007-05-29 16:50:14 PDT
Okay, this bug doesn't happen with the Mac port.
Comment 2 Holger Freyther 2007-05-30 15:36:30 PDT
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;
Comment 3 mitz 2007-05-30 15:43:08 PDT
(In reply to comment #2)
> +        m_slowRepaintObjectCount = 0;

I think that will reintroduce bug 13037.
Comment 4 Holger Freyther 2007-05-30 16:01:33 PDT
(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
Comment 5 Holger Freyther 2007-05-31 11:03:28 PDT
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.
Comment 6 Holger Freyther 2007-06-05 08:31:25 PDT
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.
Comment 7 Holger Freyther 2007-06-05 08:52:04 PDT
(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 8 mitz 2007-06-06 14:31:18 PDT
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.
Comment 9 Dave Hyatt 2007-06-26 21:57:24 PDT
Comment on attachment 14868 [details]
Reset FrameView::m_slowRepaintObjectCount from RenderView::destroy

Would rather hold off on this for now.
Comment 10 Maciej Stachowiak 2007-06-26 23:10:45 PDT
(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.)
Comment 11 Holger Freyther 2007-06-27 00:25:01 PDT
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)
Comment 12 mitz 2007-06-27 00:29:22 PDT
(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.
Comment 13 Holger Freyther 2009-10-28 04:40:45 PDT
The FrameView should not be reused... See ChromeClientGtk.cpp...