Changeset 151581 in webkit

Timestamp:

Jun 13, 2013 9:58:11 PM (11 years ago)

Author:

Simon Fraser

Message:

Sometimes we stick in slow scrolling mode even after leaving a page
​https://bugs.webkit.org/show_bug.cgi?id=117622

Reviewed by Sam Weinig.

ScrollingCoordinator, and thus the scrolling state tree, is owned by Page,
and so persists when navigating between cached pages. We do give the ScrollingStateTree
a new ScrollingStateScrollingNode on navigation, however the ScrollingStateScrollingNode
would not have dirty flags set for, say, WheelEventHandlerCount if the document had
no wheel handlers. And because that dirty flag wasn't set, ScrollingTree::commitNewTreeState()
would fail to update m_hasWheelEventHandlers, so we'd remain in slow scrolling.

Fix by having ScrollingStateTree remember if it's been given a new root ScrollingStateScrollingNode,
and making ScrollingTree::commitNewTreeState() to the right thing in that case.

Also fix a couple of issues with the tiled scrolling indicator. First, on cached page
navigation, the indicator color would show the state for the old page, because
ScrollingCoordinatorMac::commitTreeState() checked scrollingTree()->hasWheelEventHandlers()
before the scrolling thread has committed the new scrolling tree.

Second, the color change would animate, so stop that.

Not testable, since tests can only dump the scrolling state tree.

• page/scrolling/ScrollingStateTree.cpp:

(WebCore::ScrollingStateTree::ScrollingStateTree):
(WebCore::ScrollingStateTree::attachNode):
(WebCore::ScrollingStateTree::commit):

• page/scrolling/ScrollingStateTree.h:

(WebCore::ScrollingStateTree::hasNewRootStateNode):

• page/scrolling/ScrollingTree.cpp:

(WebCore::ScrollingTree::commitNewTreeState):

• page/scrolling/mac/ScrollingCoordinatorMac.mm:

(WebCore::ScrollingCoordinatorMac::commitTreeState):

• platform/graphics/ca/mac/TileController.mm:

(-[WebTiledScrollingIndicatorLayer init]): Turn off borderColor implicit animations.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• page/scrolling/ScrollingStateTree.cpp (modified) (3 diffs)
• page/scrolling/ScrollingStateTree.h (modified) (2 diffs)
• page/scrolling/ScrollingTree.cpp (modified) (1 diff)
• page/scrolling/mac/ScrollingCoordinatorMac.mm (modified) (1 diff)
• platform/graphics/ca/mac/TileController.mm (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-06-13  Simon Fraser  <simon.fraser@apple.com>
+
+        Sometimes we stick in slow scrolling mode even after leaving a page
+        https://bugs.webkit.org/show_bug.cgi?id=117622
+
+        Reviewed by Sam Weinig.
+        
+        ScrollingCoordinator, and thus the scrolling state tree, is owned by Page,
+        and so persists when navigating between cached pages. We do give the ScrollingStateTree
+        a new ScrollingStateScrollingNode on navigation, however the ScrollingStateScrollingNode
+        would not have dirty flags set for, say, WheelEventHandlerCount if the document had
+        no wheel handlers. And because that dirty flag wasn't set, ScrollingTree::commitNewTreeState()
+        would fail to update m_hasWheelEventHandlers, so we'd remain in slow scrolling.
+        
+        Fix by having ScrollingStateTree remember if it's been given a new root ScrollingStateScrollingNode,
+        and making ScrollingTree::commitNewTreeState() to the right thing in that case.
+        
+        Also fix a couple of issues with the tiled scrolling indicator. First, on cached page
+        navigation, the indicator color would show the state for the old page, because
+        ScrollingCoordinatorMac::commitTreeState() checked scrollingTree()->hasWheelEventHandlers()
+        before the scrolling thread has committed the new scrolling tree.
+        
+        Second, the color change would animate, so stop that.
+
+        Not testable, since tests can only dump the scrolling state tree.
+
+        * page/scrolling/ScrollingStateTree.cpp:
+        (WebCore::ScrollingStateTree::ScrollingStateTree):
+        (WebCore::ScrollingStateTree::attachNode):
+        (WebCore::ScrollingStateTree::commit):
+        * page/scrolling/ScrollingStateTree.h:
+        (WebCore::ScrollingStateTree::hasNewRootStateNode):
+        * page/scrolling/ScrollingTree.cpp:
+        (WebCore::ScrollingTree::commitNewTreeState):
+        * page/scrolling/mac/ScrollingCoordinatorMac.mm:
+        (WebCore::ScrollingCoordinatorMac::commitTreeState):
+        * platform/graphics/ca/mac/TileController.mm:
+        (-[WebTiledScrollingIndicatorLayer init]): Turn off borderColor implicit animations.
+
 2013-06-13  Peter Gal  <galpeter@inf.u-szeged.hu>
 

trunk/Source/WebCore/page/scrolling/ScrollingStateTree.cpp

@@ -42 +42 @@
 ScrollingStateTree::ScrollingStateTree()
     : m_hasChangedProperties(false)
+    , m_hasNewRootStateNode(false)
 {
 }
@@ -71 +72 @@
         setRootStateNode(ScrollingStateScrollingNode::create(this, newNodeID));
         newNode = rootStateNode();
+        m_hasNewRootStateNode = true;
     } else {
         ScrollingStateNode* parent = stateNodeForID(parentID);
@@ -137 +139 @@
     m_hasChangedProperties = false;
 
+    treeStateClone->m_hasNewRootStateNode = m_hasNewRootStateNode;
+    m_hasNewRootStateNode = false;
+
     return treeStateClone.release();
 }

trunk/Source/WebCore/page/scrolling/ScrollingStateTree.h

@@ -63 +63 @@
     bool hasChangedProperties() const { return m_hasChangedProperties; }
 
+    bool hasNewRootStateNode() const { return m_hasNewRootStateNode; }
+
 private:
     ScrollingStateTree();
@@ -76 +78 @@
     Vector<ScrollingNodeID> m_nodesRemovedSinceLastCommit;
     bool m_hasChangedProperties;
+    bool m_hasNewRootStateNode;
 };
 

trunk/Source/WebCore/page/scrolling/ScrollingTree.cpp

@@ -133 +133 @@
     ASSERT(ScrollingThread::isCurrentThread());
 
+    bool rootStateNodeChanged = scrollingStateTree->hasNewRootStateNode();
+    
     ScrollingStateScrollingNode* rootNode = scrollingStateTree->rootStateNode();
     if (rootNode
-        && (rootNode->hasChangedProperty(ScrollingStateScrollingNode::WheelEventHandlerCount)
+        && (rootStateNodeChanged
+            || rootNode->hasChangedProperty(ScrollingStateScrollingNode::WheelEventHandlerCount)
             || rootNode->hasChangedProperty(ScrollingStateScrollingNode::NonFastScrollableRegion)
             || rootNode->hasChangedProperty(ScrollingStateNode::ScrollLayer))) {
         MutexLocker lock(m_mutex);
 
-        if (rootNode->hasChangedProperty(ScrollingStateNode::ScrollLayer))
+        if (rootStateNodeChanged || rootNode->hasChangedProperty(ScrollingStateNode::ScrollLayer))
             m_mainFrameScrollPosition = IntPoint();
-        if (rootNode->hasChangedProperty(ScrollingStateScrollingNode::WheelEventHandlerCount))
+        if (rootStateNodeChanged || rootNode->hasChangedProperty(ScrollingStateScrollingNode::WheelEventHandlerCount))
             m_hasWheelEventHandlers = scrollingStateTree->rootStateNode()->wheelEventHandlerCount();
-        if (rootNode->hasChangedProperty(ScrollingStateScrollingNode::NonFastScrollableRegion))
+        if (rootStateNodeChanged || rootNode->hasChangedProperty(ScrollingStateScrollingNode::NonFastScrollableRegion))
             m_nonFastScrollableRegion = scrollingStateTree->rootStateNode()->nonFastScrollableRegion();
     }

trunk/Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm

@@ -443 +443 @@
     if (shouldUpdateScrollLayerPositionOnMainThread())
         indicatorMode = MainThreadScrollingBecauseOfStyleIndication;
-    else if (scrollingTree() && scrollingTree()->hasWheelEventHandlers())
+    else if (m_scrollingStateTree->rootStateNode()->wheelEventHandlerCount())
         indicatorMode =  MainThreadScrollingBecauseOfEventHandlersIndication;
     else

trunk/Source/WebCore/platform/graphics/ca/mac/TileController.mm

@@ -64 +64 @@
 
         _visibleRectFrameLayer = [CALayer layer];
-        [_visibleRectFrameLayer setStyle:[NSDictionary dictionaryWithObject:[NSDictionary dictionaryWithObjectsAndKeys:[NSNull null], @"bounds", [NSNull null], @"position", nil] forKey:@"actions"]];
+        [_visibleRectFrameLayer setStyle:[NSDictionary dictionaryWithObject:[NSDictionary dictionaryWithObjectsAndKeys:[NSNull null], @"bounds", [NSNull null], @"position", [NSNull null], @"borderColor", nil] forKey:@"actions"]];
         [self addSublayer:_visibleRectFrameLayer];
         [_visibleRectFrameLayer setBorderColor:WebCore::cachedCGColor(WebCore::Color(255, 0, 0), WebCore::ColorSpaceDeviceRGB)];