[webkit-reviews] review requested: [Bug 101001] Pages with position:fixed elements should still be able to scroll on the scrolling thread : [Attachment 171959] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 1 18:09:18 PDT 2012
Simon Fraser (smfr) <simon.fraser at apple.com> has asked for review:
Bug 101001: Pages with position:fixed elements should still be able to scroll
on the scrolling thread
https://bugs.webkit.org/show_bug.cgi?id=101001
Attachment 171959: Patch
https://bugs.webkit.org/attachment.cgi?id=171959&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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->viewportRectAtLastLayou
t());
> + m_changedProperties |= ViewportRectAtLastLayout;
> + m_scrollingStateTree->setHasChangedProperties(true);
> + }
> +
> + if (constraints->layerPositionAtLastLayout() !=
m_constraints->layerPositionAtLastLayout()) {
> +
m_constraints->setLayerPositionAtLastLayout(constraints->layerPositionAtLastLay
out());
> + 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->viewportRectAtLa
stLayout());
> + if (state->changedProperties() &
ScrollingStateFixedNode::LayerPositionAtLastLayout)
> +
m_constraints->setLayerPositionAtLastLayout(updatedConstraints->layerPositionAt
LastLayout());
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?
More information about the webkit-reviews
mailing list