Changeset 57165 in webkit

Timestamp:

Apr 6, 2010 1:44:42 PM (14 years ago)

Author:

jamesr@google.com

Message:

2010-04-06 James Robinson <​jamesr@chromium.org>

Reviewed by Simon Fraser.

Reverts the incorrect fixed position fastpath scrolling logic
​https://bugs.webkit.org/show_bug.cgi?id=33150

This code does not properly handle overflow or transforms on fixed
position elements, causing repaint bugs on scroll.

No new tests.

• page/FrameView.cpp: (WebCore::FrameView::addFixedObject): (WebCore::FrameView::removeFixedObject):
• page/FrameView.h:
• platform/ScrollView.cpp: (WebCore::ScrollView::scrollContents):
• platform/ScrollView.h:
• rendering/RenderObject.cpp: (WebCore::RenderObject::styleWillChange):

Location:

trunk/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/FrameView.cpp (modified) (2 diffs)
• page/FrameView.h (modified) (1 diff)
• platform/ScrollView.cpp (modified) (2 diffs)
• platform/ScrollView.h (modified) (1 diff)
• rendering/RenderObject.cpp (modified) (2 diffs)

trunk/WebCore/ChangeLog

@@ +1 @@
+2010-04-06  James Robinson  <jamesr@chromium.org>
+
+        Reviewed by Simon Fraser.
+
+        Reverts the incorrect fixed position fastpath scrolling logic
+        https://bugs.webkit.org/show_bug.cgi?id=33150
+
+        This code does not properly handle overflow or transforms on fixed
+        position elements, causing repaint bugs on scroll.
+
+        No new tests.
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::addFixedObject):
+        (WebCore::FrameView::removeFixedObject):
+        * page/FrameView.h:
+        * platform/ScrollView.cpp:
+        (WebCore::ScrollView::scrollContents):
+        * platform/ScrollView.h:
+        * rendering/RenderObject.cpp:
+        (WebCore::RenderObject::styleWillChange):
+
 2010-04-06  Kevin Ollivier  <kevino@theolliviers.com>
 

trunk/WebCore/page/FrameView.cpp

@@ -871 +871 @@
 void FrameView::addFixedObject()
 {
-    if (!m_fixedObjectCount && platformWidget())
-        setCanBlitOnScroll(false);
     ++m_fixedObjectCount;
 }
@@ -880 +878 @@
     ASSERT(m_fixedObjectCount > 0);
     --m_fixedObjectCount;
-    if (!m_fixedObjectCount)
-        setCanBlitOnScroll(!useSlowRepaints());
-}
-
-bool FrameView::scrollContentsFastPath(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect)
-{
-    const size_t fixedObjectThreshold = 5;
-
-    ListHashSet<RenderBox*>* positionedObjects = 0;
-    if (RenderView* root = m_frame->contentRenderer())
-        positionedObjects = root->positionedObjects();
-
-    if (!positionedObjects || positionedObjects->isEmpty()) {
-        hostWindow()->scroll(scrollDelta, rectToScroll, clipRect);
-        return true;
-    }
-
-    // Get the rects of the fixed objects visible in the rectToScroll
-    Vector<IntRect, fixedObjectThreshold> subRectToUpdate;
-    bool updateInvalidatedSubRect = true;
-    ListHashSet<RenderBox*>::const_iterator end = positionedObjects->end();
-    for (ListHashSet<RenderBox*>::const_iterator it = positionedObjects->begin(); it != end; ++it) {
-        RenderBox* renderBox = *it;
-        if (renderBox->style()->position() != FixedPosition)
-            continue;
-        IntRect topLevelRect;
-        IntRect updateRect = renderBox->paintingRootRect(topLevelRect);
-        updateRect.move(-scrollX(), -scrollY());
-        updateRect.intersect(rectToScroll);
-        if (!updateRect.isEmpty()) {
-            if (subRectToUpdate.size() >= fixedObjectThreshold) {
-                updateInvalidatedSubRect = false;
-                break;
-            }
-            subRectToUpdate.append(updateRect);
-        }
-    }
-
-    // Scroll the view
-    if (updateInvalidatedSubRect) {
-        // 1) scroll
-        hostWindow()->scroll(scrollDelta, rectToScroll, clipRect);
-
-        // 2) update the area of fixed objects that has been invalidated
-        size_t fixObjectsCount = subRectToUpdate.size();
-        for (size_t i = 0; i < fixObjectsCount; ++i) {
-            IntRect updateRect = subRectToUpdate[i];
-            IntRect scrolledRect = updateRect;
-            scrolledRect.move(scrollDelta);
-            updateRect.unite(scrolledRect);
-            updateRect.intersect(rectToScroll);
-            hostWindow()->invalidateContentsAndWindow(updateRect, false);
-        }
-        return true;
-    }
-
-    // the number of fixed objects exceed the threshold, we cannot use the fast path
-    return false;
 }
 

trunk/WebCore/page/FrameView.h

@@ -210 +210 @@
     void invalidateScrollCorner();
 
-protected:
-    virtual bool scrollContentsFastPath(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect);
-
 private:
     FrameView(Frame*);

trunk/WebCore/platform/ScrollView.cpp

@@ -519 +519 @@
     if (canBlitOnScroll()) { // The main frame can just blit the WebView window
         // FIXME: Find a way to scroll subframes with this faster path
-        if (!scrollContentsFastPath(-scrollDelta, scrollViewRect, clipRect))
-            hostWindow()->invalidateContentsForSlowScroll(updateRect, false);
+        hostWindow()->scroll(-scrollDelta, scrollViewRect, clipRect);
     } else { 
        // We need to go ahead and repaint the entire backing store.  Do it now before moving the
@@ -532 +531 @@
     // Now blit the backingstore into the window which should be very fast.
     hostWindow()->invalidateWindow(IntRect(), true);
-}
-
-bool ScrollView::scrollContentsFastPath(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect)
-{
-    hostWindow()->scroll(scrollDelta, rectToScroll, clipRect);
-    return true;
 }
 

trunk/WebCore/platform/ScrollView.h

@@ -253 +253 @@
     virtual void paintScrollCorner(GraphicsContext*, const IntRect& cornerRect);
 
-    // Scroll the content by blitting the pixels
-    virtual bool scrollContentsFastPath(const IntSize& scrollDelta, const IntRect& rectToScroll, const IntRect& clipRect);
-    
 private:
     RefPtr<Scrollbar> m_horizontalScrollbar;

trunk/WebCore/rendering/RenderObject.cpp

@@ -1653 +1653 @@
 
     if (view()->frameView()) {
+        // FIXME: A better solution would be to only invalidate the fixed regions when scrolling.  It's overkill to
+        // prevent the entire view from blitting on a scroll.
+
         bool shouldBlitOnFixedBackgroundImage = false;
 #if ENABLE(FAST_MOBILE_SCROLLING)
@@ -1662 +1665 @@
 #endif
 
-        bool newStyleSlowScroll = newStyle && !shouldBlitOnFixedBackgroundImage && newStyle->hasFixedBackgroundImage();
-        bool oldStyleSlowScroll = m_style && !shouldBlitOnFixedBackgroundImage && m_style->hasFixedBackgroundImage();
+        bool newStyleSlowScroll = newStyle && (newStyle->position() == FixedPosition
+                                                || (!shouldBlitOnFixedBackgroundImage && newStyle->hasFixedBackgroundImage()));
+        bool oldStyleSlowScroll = m_style && (m_style->position() == FixedPosition
+                                                || (!shouldBlitOnFixedBackgroundImage && m_style->hasFixedBackgroundImage()));
+
         if (oldStyleSlowScroll != newStyleSlowScroll) {
             if (oldStyleSlowScroll)