Bug 101001 – Pages with position:fixed elements should still be able to scroll on the scrolling thread

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

Currently when we have position:fixed elements on a page we fall into the slow "scroll on the main thread" mode. 

The scrolling tree should know about fixed layers so we can do scrolling on the scrolling thread.

<rdar://problem/10857315>

[Beth Dakin]

Created attachment 171959 [details]
Patch

[WebKit Review Bot]

Attachment 171959 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/rendering/RenderLayerCompositor.cpp:2565:  Declaration has space between type name and * in RenderBoxModelObject *renderer  [whitespace/declaration] [3]
Source/WebCore/rendering/RenderLayerCompositor.cpp:2562:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 171959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171959&action=review

Looks really great. I found one or two showstopper issues, so review- because of those. Most of the other comments are just minor style ones.

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:54
>  enum ScrollingNodeType {
> -    ScrollingNode
> -    // FIXME: This will soon contain other types such as FixedNode.
> +    ScrollingNode,
> +    FixedNode
>  };

Could this just be defined on one line?

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:122
> +    virtual void updateViewportConstrainedNode(ScrollingNodeID, PassOwnPtr<ViewportConstraints>, GraphicsLayer*) { }

It’s the empty function body that creates the only need to include PassOwnPtr.h here. Could we put that empty body into a cpp file instead? I don’t think it gets inlined.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp:43
> +ScrollingStateFixedNode::ScrollingStateFixedNode(ScrollingStateTree* stateTree, ScrollingNodeID nodeID)

I’d just name this “tree”.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp:49
> +ScrollingStateFixedNode::ScrollingStateFixedNode(const ScrollingStateFixedNode& stateNode)

I’d just name this “node”.

>> Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp:100
>> +    if (m_constraints->anchorEdges() != 0) {
> 
> Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]

You should obey the style bot here.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp:114
> +    if (m_constraints->alignmentOffset() != FloatSize()) {

I think !m_constraints->alignmentOffset() .isEmpty() would be better, if it generates the correct code. Or maybe !m_constraints->alignmentOffset().isZero()?

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp:119
> +    if (m_constraints->viewportRectAtLastLayout() != FloatRect()) {

Ditto.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp:125
> +    if (m_constraints->layerPositionAtLastLayout() != FloatPoint()) {

Ditto.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:31
> +#include "IntRect.h"

This include doesn’t seem to be needed.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:32
> +#include "Region.h"

This include doesn’t seem to be needed.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:36
> +#include <wtf/PassOwnPtr.h>

I think this could include Forward.h instead of PassOwnPtr.h.

Please remove any other unneeded includes.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:60
> +    virtual bool isScrollingStateFixedNode() OVERRIDE { return true; }
> +
> +    virtual bool hasChangedProperties() const OVERRIDE { return m_changedProperties; }
> +    virtual unsigned changedProperties() const OVERRIDE { return m_changedProperties; }
> +    virtual void resetChangedProperties() OVERRIDE { m_changedProperties = 0; }

Can these overrides be private instead of public?

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:64
> +    FixedPositionViewportConstraints* viewportConstraints() const { return m_constraints.get(); }

Can this return a const FixedPositionViewportConstraints* instead of a non-const?

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:66
> +    virtual void dumpProperties(TextStream&, int indent) const;

Please add OVERRIDE.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:72
> +    unsigned m_changedProperties;

The name here seems wrong. Since this variable does not contain properties, it should not be named “changed properties”. It should have the word “number” or “count” in its name.

Same for the getter function.

> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:70
> +    if (isScrollingStateScrollingNode())
> +        clone = adoptPtr(new ScrollingStateScrollingNode(*toScrollingStateScrollingNode(this)));
> +    else if (isScrollingStateFixedNode())
> +        clone = adoptPtr(new ScrollingStateFixedNode(*toScrollingStateFixedNode(this)));

This seems like something that’s best done with a private virtual member function rather than a set of if statements. The node could clone itself.

Also, what does this function do if the node is neither type? It seems like it will just crash later. So the second if could just be an assertion instead of an if.

> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:106
> -    if (size_t index = m_children->find(node)) {
> +    size_t index = m_children->find(node);
> +    if (index != notFound) {

Why this change?

> Source/WebCore/page/scrolling/ScrollingStateNode.h:53
>      virtual bool isScrollingStateScrollingNode() { return false; }
> +    virtual bool isScrollingStateFixedNode() { return false; }

These virtual functions should be const. But also, we should remove them if we can. Such functions typically mean we have a design mistake.

> Source/WebCore/page/scrolling/ScrollingStateNode.h:63
> +    GraphicsLayer* graphicsLayer() const { return m_graphicsLayer; }

Can this return a const GraphicsLayer* or be a non-const member function?

> Source/WebCore/page/scrolling/ScrollingTree.cpp:172
> +            OwnPtr<ScrollingTreeNode> newNode;
> +            if (stateNode->isScrollingStateScrollingNode())
> +                newNode = ScrollingTreeScrollingNode::create(this);
> +            else if (stateNode->isScrollingStateFixedNode())
> +                newNode = ScrollingTreeFixedNode::create(this);
> +            else
> +                ASSERT_NOT_REACHED();

This seems like something best done by a virtual function rather than a set of if statements.

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:53
> +    // Return whether this scrolling coordinator can keep fixed position layers fixed to their
> +    // containers while scrolling.
> +    virtual bool supportsFixedPositionLayers() const OVERRIDE { return true; }

Can this be private instead of public?

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:85
> +    // This function will update the ScrollingStateNode for the given viewport constrained object.
> +    virtual void updateViewportConstrainedNode(ScrollingNodeID, PassOwnPtr<ViewportConstraints>, GraphicsLayer*) OVERRIDE;
> +
> +    // Called to synch the GraphicsLayer positions for child layers when their CALayers have been moved by the scrolling thread.
> +    virtual void syncChildPositions(const IntRect& viewportRect) OVERRIDE;

Can these be private instead of public?

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm:258
> +            PassOwnPtr<ScrollingStateFixedNode> fixedNode = ScrollingStateFixedNode::create(m_scrollingStateTree.get(), newNodeID);

This should be OwnPtr. We don’t use PassOwnPtr for local variables.

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm:260
> +            parent->appendChild(fixedNode);

Since we’ll be using OwnPtr, this will need to say fixedNode.release().

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm:390
> +    ScrollingStateScrollingNode* rootNode = m_scrollingStateTree->rootStateNode();
> +
> +    Vector<OwnPtr<ScrollingStateNode> >* children = rootNode->children();

Not sure we need the rootNode local variable.

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm:397
> +        FloatPoint newChildPos = child->viewportConstraints()->layerPositionForViewportRect(viewportRect);

Not a big fan of “pos” here. Could be “position”, in fact, could just be “position” or “childPosition” instead of “newChildPos”. I don’t think a longer name makes the super-local-scope variable meaning any clearer.

> Source/WebCore/page/scrolling/mac/ScrollingTreeFixedNode.h:47
> +    virtual void update(ScrollingStateNode*) OVERRIDE;
> +    virtual void parentScrollPositionDidChange(const IntRect& viewportRect) OVERRIDE;

Can we make these virtual function overrides private?

> Source/WebCore/page/scrolling/mac/ScrollingTreeFixedNode.mm:77
> +    CGPoint newPosition = CGPointMake(layerPosition.x() - m_constraints->alignmentOffset().width() + anchorPoint.x * layerBounds.size.width,
> +                                    layerPosition.y() - m_constraints->alignmentOffset().height() + anchorPoint.y * layerBounds.size.height);

The second line could just be indented by two. Or maybe we can find a way to write this so that the math is done on both dimensions at once rather than having to write the expression twice. I wish this just worked:

    layerPosition - m_constraints->alignmentOffset() + anchorPoint * layerBounds.size

It seems like we could make it work with C++ operator overloading, even if the overloaded operators were just in this file.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2523
> +    // Also walk our our stacking contexts looking for a composited layer which is itself fixed.

This comment says what the code does, but not why. I think I understand why given the function name, but comment should say why rather than what.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:2562
>> +    if (layer->renderer()->isStickyPositioned()) {
> 
> An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]

stylebot is right, although the code will look a little strange. Maybe factor this into two functions since there is almost no shared code.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:2565
>> +        RenderBoxModelObject *renderer = toRenderBoxModelObject(layer->renderer());
> 
> Declaration has space between type name and * in RenderBoxModelObject *renderer  [whitespace/declaration] [3]

stylebot is right

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2606
> +    return nullptr;

This may cause a compilation error on some platforms (Windows?) because it’s unreachable code.

[Simon Fraser (smfr)]

Comment on attachment 171959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=171959&action=review

> Source/WebCore/page/FrameView.cpp:1860
> +    // If the scrolling thread is updating the fixed elements, then we shouldn't update them again here.
> +    if (Page* page = m_frame->page()) {
> +        if (page->mainFrame() == m_frame) {
> +            if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) {
> +                if (scrollingCoordinator->supportsFixedPositionLayers() && !scrollingCoordinator->shouldUpdateScrollLayerPositionOnMainThread()
> +                    && !inProgrammaticScroll())
> +                    return;
> +            }
> +        }
> +    }

Quite a mouthful. Can we package that up into a one-liner somehow? Can we pass in the info?

Should it be outside the #if USE(ACCELERATED_COMPOSITING) block?

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.cpp:93
> +    if (constraints->anchorEdges() != m_constraints->anchorEdges()) {
> +        m_constraints->setAnchorEdges(constraints->anchorEdges());
> +        m_changedProperties |= AnchorEdges;
> +        m_scrollingStateTree->setHasChangedProperties(true);
> +    }
> +
> +    if (constraints->alignmentOffset() != m_constraints->alignmentOffset()) {
> +        m_constraints->setAlignmentOffset(constraints->alignmentOffset());
> +        m_changedProperties |= AlignmentOffset;
> +        m_scrollingStateTree->setHasChangedProperties(true);
> +    }
> +
> +    if (constraints->viewportRectAtLastLayout() != m_constraints->viewportRectAtLastLayout()) {
> +        m_constraints->setViewportRectAtLastLayout(constraints->viewportRectAtLastLayout());
> +        m_changedProperties |= ViewportRectAtLastLayout;
> +        m_scrollingStateTree->setHasChangedProperties(true);
> +    }
> +
> +    if (constraints->layerPositionAtLastLayout() != m_constraints->layerPositionAtLastLayout()) {
> +        m_constraints->setLayerPositionAtLastLayout(constraints->layerPositionAtLastLayout());
> +        m_changedProperties |= LayerPositionAtLastLayout;
> +        m_scrollingStateTree->setHasChangedProperties(true);
> +    }

Can't we just make a FixedPositionViewportConstraint operator == and use that?

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:56
> +    virtual bool isScrollingStateFixedNode() OVERRIDE { return true; }

I think isFixedNode() would be sufficient.

> Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:71
> +    OwnPtr<FixedPositionViewportConstraints> m_constraints;

Just store by value. These are small. I think you could pass them around by value/reference in most places too.

> Source/WebCore/page/scrolling/ScrollingStateNode.h:53
>      virtual bool isScrollingStateScrollingNode() { return false; }
> +    virtual bool isScrollingStateFixedNode() { return false; }

isScrollingNode(), isFixedNode()

> Source/WebCore/page/scrolling/ScrollingStateNode.h:108
> +    GraphicsLayer* m_graphicsLayer;

I think this is the most worrisome change in this patch. The guarantee that that this GraphicsLayer outlines the scrolling node is weak.

> Source/WebCore/page/scrolling/ScrollingTreeNode.h:58
> +    OwnPtr<Vector<OwnPtr<ScrollingTreeNode> > > m_children;

Might be nice to have  typedef for Vector<OwnPtr<ScrollingTreeNode> >

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:82
> +    virtual void updateViewportConstrainedNode(ScrollingNodeID, PassOwnPtr<ViewportConstraints>, GraphicsLayer*) OVERRIDE;

const ViewportConstraints&

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.h:85
> +    virtual void syncChildPositions(const IntRect& viewportRect) OVERRIDE;

We should probably start thinking about the viewport rect as a LayoutRect or FloatRect, given subpixel layout and scrolling in device pixels.

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm:289
> +    m_scrollingStateTree->removeNode(node);
> +
> +    // ScrollingStateTree::removeNode() will destroy children, so we have to make sure we remove those children
> +    // from the HashMap.
> +    const Vector<ScrollingNodeID>& removedNodes = m_scrollingStateTree->removedNodes();
> +    size_t size = removedNodes.size();
> +    for (size_t i = 0; i < size; ++i)
> +        m_stateNodeMap.remove(removedNodes[i]);

I'm a bit confused about this apparently split of responsibility when removing nodes; why don't nodes remove themselves from the m_stateNodeMap?

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm:399
> +    size_t size = children->size();
> +    for (size_t i = 0; i < size; ++i) {
> +        ScrollingStateFixedNode* child = toScrollingStateFixedNode(children->at(i).get());
> +        FloatPoint newChildPos = child->viewportConstraints()->layerPositionForViewportRect(viewportRect);
> +        child->graphicsLayer()->setPosition(newChildPos);
> +    }

Needs a FIXME that this will have to go deep in the tree at some point.

> Source/WebCore/page/scrolling/mac/ScrollingTreeFixedNode.h:52
> +    OwnPtr<FixedPositionViewportConstraints> m_constraints;

By value.

> Source/WebCore/page/scrolling/mac/ScrollingTreeFixedNode.h:53
> +    RetainPtr<CALayer> m_scrollLayer;

m_scrollLayer needs renaming.

> Source/WebCore/page/scrolling/mac/ScrollingTreeFixedNode.mm:67
> +    FixedPositionViewportConstraints* updatedConstraints = state->viewportConstraints();
> +    if (state->changedProperties() & ScrollingStateFixedNode::AnchorEdges)
> +        m_constraints->setAnchorEdges(updatedConstraints->anchorEdges());
> +    if (state->changedProperties() & ScrollingStateFixedNode::AlignmentOffset)
> +        m_constraints->setAlignmentOffset(updatedConstraints->alignmentOffset());
> +    if (state->changedProperties() & ScrollingStateFixedNode::ViewportRectAtLastLayout)
> +        m_constraints->setViewportRectAtLastLayout(updatedConstraints->viewportRectAtLastLayout());
> +    if (state->changedProperties() & ScrollingStateFixedNode::LayerPositionAtLastLayout)
> +        m_constraints->setLayerPositionAtLastLayout(updatedConstraints->layerPositionAtLastLayout());

Just copy the whole thing over.

> Source/WebCore/rendering/RenderLayerBacking.cpp:1019
> +    if (renderer()->style()->position() == FixedPosition)
> +        nodeType = FixedNode;
> +    else
> +        nodeType = ScrollingNode;

what about (in future) position:fixed; overflow: scroll ?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2530
> +static bool isRootmostFixedOrStickyLayer(RenderLayer* layer)
> +{
> +    if (layer->renderer()->isStickyPositioned())
> +        return true;
> +
> +    if (layer->renderer()->style()->position() != FixedPosition)
> +        return false;
> +
> +    // Also walk our our stacking contexts looking for a composited layer which is itself fixed.
> +    for (RenderLayer* stackingContext = layer->stackingContext(); stackingContext; stackingContext = stackingContext->stackingContext()) {
> +        if (stackingContext->isComposited() && stackingContext->renderer()->style()->position() == FixedPosition)
> +            return false;
> +    }
> +
> +    return true;
> +}

This should share code with requriesCompositingForPosition()

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2538
> +void RenderLayerCompositor::updateViewportConstraintStatus(RenderLayer* layer)
> +{
> +    if (isRootmostFixedOrStickyLayer(layer))
> +        addViewportConstrainedLayer(layer);
> +    else
> +        removeViewportConstrainedLayer(layer);
> +}

I don't really like how add/remove/updateFoo methods are sprinkled throughout the entire file. We should figure out a cleaner way to do this.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2622
> +    RenderLayer* ancestor = layer->parent();
> +    while (ancestor) {
> +        if (RenderLayerBacking* backing = ancestor->backing()) {
> +            if (backing->scrollLayerID())
> +                return backing;
> +        }
> +        // If the fixed element is inside an overflow region, then we can't scroll it with the ScrollingCoordinator.
> +        // We will be able to fix this when we get overflow regions scrolling with the ScrollingCoordinator.
> +        if (layer->scrollsOverflow())
> +            return 0;
> +        ancestor = ancestor->parent();
> +    }

The conflict between compositing being based on z-order, but clipping being based on parent order is apparent here. In z-order, your parent can be your sibling.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2653
> +    // FIXME: We should support sticky position here!
> +    if (layer->renderer()->isStickyPositioned())
> +        return;
> +
> +    ScrollingCoordinator* scrollingCoordinator = this->scrollingCoordinator();
> +    if (!scrollingCoordinator)
> +        return;
> +
> +    if (!scrollingCoordinator->supportsFixedPositionLayers())
> +        return;
> +
> +    // We will need to get non-main frames scrolling with the ScrollingCoordinator before we can get
> +    // fixed position elements inside those frames to scroll with ScrollingCoordinator.
> +    if (m_renderView->document()->ownerElement())
> +        return;
> +
> +    ASSERT(layer->renderer()->style()->position() == FixedPosition);
> +    ASSERT(m_viewportConstrainedLayers.contains(layer));
> +    ASSERT(layer->isComposited());
> +    if (!layer->parent())
> +        return;
> +
> +    RenderLayerBacking* backing = layer->backing();
> +    if (!backing)
> +        return;

That all seems a bit verbose. Maybe collapse some of those tests onto one line.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2687
> +void RenderLayerCompositor::registerAllViewportConstrainedLayers()
> +{
> +    HashSet<RenderLayer*>::const_iterator end = m_viewportConstrainedLayers.end();
> +    for (HashSet<RenderLayer*>::const_iterator it = m_viewportConstrainedLayers.begin(); it != end; ++it)
> +        registerOrUpdateViewportConstrainedLayer(*it);
> +}
> +
> +void RenderLayerCompositor::unregisterAllViewportConstrainedLayers()
> +{
> +    HashSet<RenderLayer*>::const_iterator end = m_viewportConstrainedLayers.end();
> +    for (HashSet<RenderLayer*>::const_iterator it = m_viewportConstrainedLayers.begin(); it != end; ++it)
> +        unregisterViewportConstrainedLayer(*it);
> +}

Should figure out if we really need these.

> LayoutTests/platform/mac/tiled-drawing/tile-coverage-slow-scrolling.html:9
> +            background-image: url('resources/64x32-red.png');

Why do you need an image resource here?

[Beth Dakin]

Thanks, Darin! I fixed almost everything you mentioned. For anything that I only partially changed or did not change, I made a note here:


(In reply to comment #3)
> > Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:60
> > +    virtual bool isScrollingStateFixedNode() OVERRIDE { return true; }
> > +
> > +    virtual bool hasChangedProperties() const OVERRIDE { return m_changedProperties; }
> > +    virtual unsigned changedProperties() const OVERRIDE { return m_changedProperties; }
> > +    virtual void resetChangedProperties() OVERRIDE { m_changedProperties = 0; }
> 
> Can these overrides be private instead of public?
> 

changedProperties() needs to be public (the complier complains otherwise), but the others can be private.

> > Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:64
> > +    FixedPositionViewportConstraints* viewportConstraints() const { return m_constraints.get(); }
> 
> Can this return a const FixedPositionViewportConstraints* instead of a non-const?
> 

Yes. I need an ugly const_cast in one place after this change, but that will go away when I fix the FixedPositionViewportConstraints copy constructor, which you have already requested, and which I plan to do very very soon.

> 
> > Source/WebCore/page/scrolling/ScrollingStateFixedNode.h:72
> > +    unsigned m_changedProperties;
> 
> The name here seems wrong. Since this variable does not contain properties, it should not be named “changed properties”. It should have the word “number” or “count” in its name.
> 
> Same for the getter function.
>

Hmm, okay. I'll have to think this one over.
 
> > Source/WebCore/page/scrolling/ScrollingStateNode.cpp:106
> > -    if (size_t index = m_children->find(node)) {
> > +    size_t index = m_children->find(node);
> > +    if (index != notFound) {
> 
> Why this change?
> 

Ah yes, this is a tricky bit. I added a comment. The bug was easy to encounter now that m_children might be anything other than null. The index will be notFound if the node to remove is a deeper-than-1-level descendant or if node is the root state node.

> > Source/WebCore/page/scrolling/ScrollingStateNode.h:53
> >      virtual bool isScrollingStateScrollingNode() { return false; }
> > +    virtual bool isScrollingStateFixedNode() { return false; }
> 
> These virtual functions should be const. But also, we should remove them if we can. Such functions typically mean we have a design mistake.
>

I think we might still need them in ScrollingTree::updateTreeFromStateNode(). Where we run through the state tree and use it to build up the scrolling tree. How else will we know which type of scrolling tree node to make?

> > Source/WebCore/page/scrolling/ScrollingTree.cpp:172
> > +            OwnPtr<ScrollingTreeNode> newNode;
> > +            if (stateNode->isScrollingStateScrollingNode())
> > +                newNode = ScrollingTreeScrollingNode::create(this);
> > +            else if (stateNode->isScrollingStateFixedNode())
> > +                newNode = ScrollingTreeFixedNode::create(this);
> > +            else
> > +                ASSERT_NOT_REACHED();
> 
> This seems like something best done by a virtual function rather than a set of if statements.
> 

I couldn't work out how to make this happen without the if-statements. I don't think we want to add virtual functions to the state nodes to create scrolling tree nodes…

[Beth Dakin]

(In reply to comment #4)
> (From update of attachment 171959 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171959&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1860
> > +    // If the scrolling thread is updating the fixed elements, then we shouldn't update them again here.
> > +    if (Page* page = m_frame->page()) {
> > +        if (page->mainFrame() == m_frame) {
> > +            if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator()) {
> > +                if (scrollingCoordinator->supportsFixedPositionLayers() && !scrollingCoordinator->shouldUpdateScrollLayerPositionOnMainThread()
> > +                    && !inProgrammaticScroll())
> > +                    return;
> > +            }
> > +        }
> > +    }
> 
> Quite a mouthful. Can we package that up into a one-liner somehow? Can we pass in the info?
> 

True. I changed it…my new code is still a mouthful, but it is at least in a separate function.

> Should it be outside the #if USE(ACCELERATED_COMPOSITING) block?
> 

Probably not.

> > Source/WebCore/page/scrolling/ScrollingStateNode.h:108
> > +    GraphicsLayer* m_graphicsLayer;
> 
> I think this is the most worrisome change in this patch. The guarantee that that this GraphicsLayer outlines the scrolling node is weak.
>

This is somewhat worrisome…but given that the node will be destroyed whenever the RenderLayerBacking is destroyed, I think we are probably safe. If we do end up finding crashes, we can do something else…but whatever it is will necessarily be less simple. I think we should stick with this simple solution first, especially because it SHOULD be safe. 


> > +    // ScrollingStateTree::removeNode() will destroy children, so we have to make sure we remove those children
> > +    // from the HashMap.
> > +    const Vector<ScrollingNodeID>& removedNodes = m_scrollingStateTree->removedNodes();
> > +    size_t size = removedNodes.size();
> > +    for (size_t i = 0; i < size; ++i)
> > +        m_stateNodeMap.remove(removedNodes[i]);
> 
> I'm a bit confused about this apparently split of responsibility when removing nodes; why don't nodes remove themselves from the m_stateNodeMap?
>

To do that, nodes (or the state tree) would have to know about the ScrollingCoordinator. They do not right now. Perhaps would should change that in the future.


> > Source/WebCore/rendering/RenderLayerBacking.cpp:1019
> > +    if (renderer()->style()->position() == FixedPosition)
> > +        nodeType = FixedNode;
> > +    else
> > +        nodeType = ScrollingNode;
> 
> what about (in future) position:fixed; overflow: scroll ?
> 

Good question! I think we probably will want to make that a scrolling node and not a fixed node. I added a FIXME for now so that we can cross that bridge when we come to it.


> > Source/WebCore/rendering/RenderLayerCompositor.cpp:2530
> > +static bool isRootmostFixedOrStickyLayer(RenderLayer* layer)
> > +{
> > +    if (layer->renderer()->isStickyPositioned())
> > +        return true;
> > +
> > +    if (layer->renderer()->style()->position() != FixedPosition)
> > +        return false;
> > +
> > +    // Also walk our our stacking contexts looking for a composited layer which is itself fixed.
> > +    for (RenderLayer* stackingContext = layer->stackingContext(); stackingContext; stackingContext = stackingContext->stackingContext()) {
> > +        if (stackingContext->isComposited() && stackingContext->renderer()->style()->position() == FixedPosition)
> > +            return false;
> > +    }
> > +
> > +    return true;
> > +}
> 
> This should share code with requriesCompositingForPosition()
> 

I don't see the connection here. Do you think this function should just call requriesCompositingForPosition() and return false if that is true before walking the stacking contexts?

[Beth Dakin]

Created attachment 172144 [details]
Patch

[Beth Dakin]

Created attachment 172370 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 172370 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=172370&action=review

> Source/WebCore/page/FrameView.cpp:1853
> +    if (!page)
> +        return false;

Return true?

> Source/WebCore/page/scrolling/mac/ScrollingCoordinatorMac.mm:397
> +        child->graphicsLayer()->setPosition(position);

Shouldn't this be a syncPosition()?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2665
> +        // If the fixed element is inside an overflow region, then we can't scroll it with the ScrollingCoordinator.
> +        // We will be able to fix this when we get overflow regions scrolling with the ScrollingCoordinator.

Add a test for this?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2677
> +    // FIXME: We should support sticky position here! And we should eventuall support fixed/sticky elements
> +    // that are inside non-main frames once we get non-main frames scrolling with the ScrollingCoordinator.

Would be good to have some tests with fixed/sticky things in subframes.

[Beth Dakin]

Thanks, Simon! Simon and I discussed his remaining comments in person.

http://trac.webkit.org/changeset/133536