Summary: | FrameView::clear doesn't reset m_slowRepaintObjectCount leading to 'sticky' static background | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Holger Freyther <zecke> | ||||
Component: | New Bugs | Assignee: | 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
Holger Freyther
2007-05-29 16:00:37 PDT
Okay, this bug doesn't happen with the Mac port. 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. (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 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. 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.
(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.
Comment on attachment 14868 [details]
Reset FrameView::m_slowRepaintObjectCount from RenderView::destroy
Would rather hold off on this for now.
(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.) 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. The FrameView should not be reused... See ChromeClientGtk.cpp... |