Changeset 142520 in webkit


Ignore:
Timestamp:
Feb 11, 2013 2:57:36 PM (11 years ago)
Author:
Simon Fraser
Message:

REGRESSION (r133807): Sticky-position review bar on bugzilla review page is jumpy
​https://bugs.webkit.org/show_bug.cgi?id=104276
<rdar://problem/12827187>

Reviewed by Tim Horton.

When committing new scrolling tree state, if the root node has a scroll
position update, we would handle that before updating the state of child
nodes (with possibly new viewport constraints). That would cause incorrect
child layer updates.

Fix by adding a second 'update' phase that happens after child nodes,
and moving the scroll position update into that.

Scrolling tests only dump the state tree, so cannot test the bug.

  • page/FrameView.cpp:

(WebCore::FrameView::setScrollPosition): If the scroll position didn't
actually change, don't request a scroll position update from the ScrollingCoordinator.

  • page/scrolling/ScrollingTree.cpp:

(WebCore::ScrollingTree::updateTreeFromStateNode): Keep track of the scrolling node so
that we can call updateAfterChildren() on it.

  • page/scrolling/ScrollingTreeNode.h:

(ScrollingTreeNode):
(WebCore::ScrollingTreeNode::updateAfterChildren):

  • page/scrolling/ScrollingTreeScrollingNode.cpp:

(WebCore::ScrollingTreeScrollingNode::updateBeforeChildren):

  • page/scrolling/ScrollingTreeScrollingNode.h:

(ScrollingTreeScrollingNode):

  • page/scrolling/mac/ScrollingCoordinatorMac.mm:

(WebCore::ScrollingCoordinatorMac::updateViewportConstrainedNode):
In the current bug the scrolling tree was scheduled for commit because of a
scroll position request, but if only the viewport constraints change, we also need
to commit the tree.

  • page/scrolling/mac/ScrollingTreeFixedNode.h:

(ScrollingTreeFixedNode):

  • page/scrolling/mac/ScrollingTreeFixedNode.mm:

(WebCore::ScrollingTreeFixedNode::updateBeforeChildren):

  • page/scrolling/mac/ScrollingTreeScrollingNodeMac.h:

(ScrollingTreeScrollingNodeMac):

  • page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:

(WebCore::ScrollingTreeScrollingNodeMac::updateBeforeChildren):
(WebCore::ScrollingTreeScrollingNodeMac::updateAfterChildren): Move code here
that updates things that have to happen after children.

  • page/scrolling/mac/ScrollingTreeStickyNode.h:

(ScrollingTreeStickyNode):

  • page/scrolling/mac/ScrollingTreeStickyNode.mm:

(WebCore::ScrollingTreeStickyNode::updateBeforeChildren):

Location:
trunk/Source/WebCore
Files:
13 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r142517 r142520  
     12013-02-11  Simon Fraser  <simon.fraser@apple.com>
     2
     3        REGRESSION (r133807): Sticky-position review bar on bugzilla review page is jumpy
     4        https://bugs.webkit.org/show_bug.cgi?id=104276
     5        <rdar://problem/12827187>
     6
     7        Reviewed by Tim Horton.
     8
     9        When committing new scrolling tree state, if the root node has a scroll
     10        position update, we would handle that before updating the state of child
     11        nodes (with possibly new viewport constraints). That would cause incorrect
     12        child layer updates.
     13       
     14        Fix by adding a second 'update' phase that happens after child nodes,
     15        and moving the scroll position update into that.
     16
     17        Scrolling tests only dump the state tree, so cannot test the bug.
     18
     19        * page/FrameView.cpp:
     20        (WebCore::FrameView::setScrollPosition): If the scroll position didn't
     21        actually change, don't request a scroll position update from the ScrollingCoordinator.
     22        * page/scrolling/ScrollingTree.cpp:
     23        (WebCore::ScrollingTree::updateTreeFromStateNode): Keep track of the scrolling node so
     24        that we can call updateAfterChildren() on it.
     25        * page/scrolling/ScrollingTreeNode.h:
     26        (ScrollingTreeNode):
     27        (WebCore::ScrollingTreeNode::updateAfterChildren):
     28        * page/scrolling/ScrollingTreeScrollingNode.cpp:
     29        (WebCore::ScrollingTreeScrollingNode::updateBeforeChildren):
     30        * page/scrolling/ScrollingTreeScrollingNode.h:
     31        (ScrollingTreeScrollingNode):
     32        * page/scrolling/mac/ScrollingCoordinatorMac.mm:
     33        (WebCore::ScrollingCoordinatorMac::updateViewportConstrainedNode):
     34        In the current bug the scrolling tree was scheduled for commit because of a
     35        scroll position request, but if only the viewport constraints change, we also need
     36        to commit the tree.
     37        * page/scrolling/mac/ScrollingTreeFixedNode.h:
     38        (ScrollingTreeFixedNode):
     39        * page/scrolling/mac/ScrollingTreeFixedNode.mm:
     40        (WebCore::ScrollingTreeFixedNode::updateBeforeChildren):
     41        * page/scrolling/mac/ScrollingTreeScrollingNodeMac.h:
     42        (ScrollingTreeScrollingNodeMac):
     43        * page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm:
     44        (WebCore::ScrollingTreeScrollingNodeMac::updateBeforeChildren):
     45        (WebCore::ScrollingTreeScrollingNodeMac::updateAfterChildren): Move code here
     46        that updates things that have to happen after children.
     47        * page/scrolling/mac/ScrollingTreeStickyNode.h:
     48        (ScrollingTreeStickyNode):
     49        * page/scrolling/mac/ScrollingTreeStickyNode.mm:
     50        (WebCore::ScrollingTreeStickyNode::updateBeforeChildren):
     51
    1522013-02-11  Roger Fong  <roger_fong@apple.com>
    253

  • trunk/Source/WebCore/page/FrameView.cpp

    r142416 r142520  
    17761776void FrameView::setScrollPosition(const IntPoint& scrollPoint)
    17771777{
     1778    if (scrollPoint == scrollPosition())
     1779        return;
     1780
    17781781    TemporaryChange<bool> changeInProgrammaticScroll(m_inProgrammaticScroll, true);
    17791782    m_maintainScrollPositionAnchor = 0;

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

    r142505 r142520  
    152152    ScrollingTreeNodeMap::const_iterator it = m_nodeMap.find(stateNode->scrollingNodeID());
    153153
     154    ScrollingTreeNode* node;
    154155    if (it != m_nodeMap.end()) {
    155         ScrollingTreeNode* node = it->value;
    156         node->update(stateNode);
     156        node = it->value;
     157        node->updateBeforeChildren(stateNode);
    157158    } else {
    158159        // If the node isn't found, it's either new and needs to be added to the tree, or there is a new ID for our
    … …  
    165166            m_rootNode = ScrollingTreeScrollingNode::create(this, nodeID);
    166167            m_nodeMap.set(nodeID, m_rootNode.get());
    167             m_rootNode->update(stateNode);
     168            m_rootNode->updateBeforeChildren(stateNode);
     169            node = m_rootNode.get();
    168170        } else {
    169171            OwnPtr<ScrollingTreeNode> newNode;
    … …  
    177179                ASSERT_NOT_REACHED();
    178180
    179             ScrollingTreeNode* newNodeRawPtr = newNode.get();
    180             m_nodeMap.set(nodeID, newNodeRawPtr);
     181            node = newNode.get();
     182            m_nodeMap.set(nodeID, node);
    181183            ScrollingTreeNodeMap::const_iterator it = m_nodeMap.find(stateNode->parent()->scrollingNodeID());
    182184            ASSERT(it != m_nodeMap.end());
    … …  
    186188                parent->appendChild(newNode.release());
    187189            }
    188             newNodeRawPtr->update(stateNode);
     190            node->updateBeforeChildren(stateNode);
    189191        }
    190192    }
    … …  
    198200    for (size_t i = 0; i < size; ++i)
    199201        updateTreeFromStateNode(stateNodeChildren->at(i).get());
     202   
     203    node->updateAfterChildren(stateNode);
    200204}
    201205

  • trunk/Source/WebCore/page/scrolling/ScrollingTreeNode.h

    r141703 r142520  
    4545    virtual ~ScrollingTreeNode();
    4646
    47     virtual void update(ScrollingStateNode*) = 0;
     47    virtual void updateBeforeChildren(ScrollingStateNode*) = 0;
     48    virtual void updateAfterChildren(ScrollingStateNode*) { }
    4849
    4950    virtual void parentScrollPositionDidChange(const IntRect& viewportRect, const FloatSize& cumulativeDelta) = 0;

  • trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.cpp

    r141704 r142520  
    5050}
    5151
    52 void ScrollingTreeScrollingNode::update(ScrollingStateNode* stateNode)
     52void ScrollingTreeScrollingNode::updateBeforeChildren(ScrollingStateNode* stateNode)
    5353{
    5454    ScrollingStateScrollingNode* state = toScrollingStateScrollingNode(stateNode);

  • trunk/Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h

    r141703 r142520  
    4646    virtual ~ScrollingTreeScrollingNode();
    4747
    48     virtual void update(ScrollingStateNode*) OVERRIDE;
     48    virtual void updateBeforeChildren(ScrollingStateNode*) OVERRIDE;
    4949
    5050    // FIXME: We should implement this when we support ScrollingTreeScrollingNodes as children.

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

    r142505 r142520  
    362362    }
    363363    }
     364    scheduleTreeStateCommit();
    364365}
    365366

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeFixedNode.h

    r141703 r142520  
    4848    ScrollingTreeFixedNode(ScrollingTree*, ScrollingNodeID);
    4949
    50     virtual void update(ScrollingStateNode*) OVERRIDE;
     50    virtual void updateBeforeChildren(ScrollingStateNode*) OVERRIDE;
    5151    virtual void parentScrollPositionDidChange(const IntRect& viewportRect, const FloatSize& cumulativeDelta) OVERRIDE;
    5252

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeFixedNode.mm

    r141704 r142520  
    4848}
    4949
    50 void ScrollingTreeFixedNode::update(ScrollingStateNode* stateNode)
     50void ScrollingTreeFixedNode::updateBeforeChildren(ScrollingStateNode* stateNode)
    5151{
    5252    ScrollingStateFixedNode* fixedStateNode = toScrollingStateFixedNode(stateNode);

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.h

    r141703 r142520  
    4444private:
    4545    // ScrollingTreeNode member functions.
    46     virtual void update(ScrollingStateNode*) OVERRIDE;
     46    virtual void updateBeforeChildren(ScrollingStateNode*) OVERRIDE;
     47    virtual void updateAfterChildren(ScrollingStateNode*) OVERRIDE;
    4748    virtual void handleWheelEvent(const PlatformWheelEvent&) OVERRIDE;
    4849

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeScrollingNodeMac.mm

    r141704 r142520  
    6666}
    6767
    68 void ScrollingTreeScrollingNodeMac::update(ScrollingStateNode* stateNode)
    69 {
    70     ScrollingTreeScrollingNode::update(stateNode);
     68void ScrollingTreeScrollingNodeMac::updateBeforeChildren(ScrollingStateNode* stateNode)
     69{
     70    ScrollingTreeScrollingNode::updateBeforeChildren(stateNode);
    7171    ScrollingStateScrollingNode* scrollingStateNode = toScrollingStateScrollingNode(stateNode);
    7272
    … …  
    7676    if (scrollingStateNode->hasChangedProperty(ScrollingStateScrollingNode::CounterScrollingLayer))
    7777        m_counterScrollingLayer = scrollingStateNode->counterScrollingPlatformLayer();
    78 
    79     if (scrollingStateNode->hasChangedProperty(ScrollingStateScrollingNode::RequestedScrollPosition))
    80         setScrollPosition(scrollingStateNode->requestedScrollPosition());
    81 
    82     if (scrollingStateNode->hasChangedProperty(ScrollingStateNode::ScrollLayer) || scrollingStateNode->hasChangedProperty(ScrollingStateScrollingNode::ContentsSize) || scrollingStateNode->hasChangedProperty(ScrollingStateScrollingNode::ViewportRect))
    83         updateMainFramePinState(scrollPosition());
    8478
    8579    if (scrollingStateNode->hasChangedProperty(ScrollingStateScrollingNode::ShouldUpdateScrollLayerPositionOnMainThread)) {
    … …  
    10599            logWheelEventHandlerCountChanged(scrollingStateNode->wheelEventHandlerCount());
    106100    }
     101}
     102
     103void ScrollingTreeScrollingNodeMac::updateAfterChildren(ScrollingStateNode* stateNode)
     104{
     105    ScrollingTreeScrollingNode::updateAfterChildren(stateNode);
     106
     107    ScrollingStateScrollingNode* scrollingStateNode = toScrollingStateScrollingNode(stateNode);
     108
     109    // Update the scroll position after child nodes have been updated, because they need to have updated their constraints before any scrolling happens.
     110    if (scrollingStateNode->hasChangedProperty(ScrollingStateScrollingNode::RequestedScrollPosition))
     111        setScrollPosition(scrollingStateNode->requestedScrollPosition());
     112
     113    if (scrollingStateNode->hasChangedProperty(ScrollingStateNode::ScrollLayer) || scrollingStateNode->hasChangedProperty(ScrollingStateScrollingNode::ContentsSize) || scrollingStateNode->hasChangedProperty(ScrollingStateScrollingNode::ViewportRect))
     114        updateMainFramePinState(scrollPosition());
    107115}
    108116

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeStickyNode.h

    r141703 r142520  
    4848    ScrollingTreeStickyNode(ScrollingTree*, ScrollingNodeID);
    4949
    50     virtual void update(ScrollingStateNode*) OVERRIDE;
     50    virtual void updateBeforeChildren(ScrollingStateNode*) OVERRIDE;
    5151    virtual void parentScrollPositionDidChange(const IntRect& viewportRect, const FloatSize& cumulativeDelta) OVERRIDE;
    5252

  • trunk/Source/WebCore/page/scrolling/mac/ScrollingTreeStickyNode.mm

    r141704 r142520  
    4848}
    4949
    50 void ScrollingTreeStickyNode::update(ScrollingStateNode* stateNode)
     50void ScrollingTreeStickyNode::updateBeforeChildren(ScrollingStateNode* stateNode)
    5151{
    5252    ScrollingStateStickyNode* stickyStateNode = toScrollingStateStickyNode(stateNode);
Note: See TracChangeset for help on using the changeset viewer.