Bug 199514 – Clean up ScrollingTreeFixedNode::applyLayerPositions

• Status: NEW
• Component: Scrolling

[Antti Koivisto]

Avoid is<FooNode> tests.

[Antti Koivisto]

Created attachment 373490 [details]
patch

[EWS Watchlist]

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


ERROR: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:60:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 11 files


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

[Antti Koivisto]

Created attachment 373491 [details]
patch

[EWS Watchlist]

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


ERROR: Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:60:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 1 in 11 files


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

[Simon Fraser (smfr)]

Comment on attachment 373491 [details]
patch

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

Isn't it a bit late to be making refactoring changes like this?

> Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.mm:74
> +        if (!children() || children()->size() != 1)
> +            return true;

I don't understand this test. Why does the number of children affect behavior; can't a positioned node have any number of descendant fixed or sticky or scrolling nodes?

> Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.mm:76
> +        // In there is a child node for the same layer then this node exits for scroll propagationg

"If there". propagationg -> propagation?

> Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.mm:83
> +        if (is<ScrollingTreeStickyNode>(child))
> +            return downcast<ScrollingTreeStickyNode>(child).layer() != m_layer;
> +        if (is<ScrollingTreeFixedNode>(child))
> +            return downcast<ScrollingTreeFixedNode>(child).layer() != m_layer;

Whether it's the same layer seems like implementation detail. I might make more layers in RenderLayerBacking and break this.

[Antti Koivisto]

> I don't understand this test. Why does the number of children affect
> behavior; can't a positioned node have any number of descendant fixed or
> sticky or scrolling nodes?

This check...

> > Source/WebCore/page/scrolling/cocoa/ScrollingTreePositionedNode.mm:83
> > +        if (is<ScrollingTreeStickyNode>(child))
> > +            return downcast<ScrollingTreeStickyNode>(child).layer() != m_layer;
> > +        if (is<ScrollingTreeFixedNode>(child))
> > +            return downcast<ScrollingTreeFixedNode>(child).layer() != m_layer;
> 
> Whether it's the same layer seems like implementation detail. I might make
> more layers in RenderLayerBacking and break this.

...along with these matches the tower build in RenderLayerCompositor::updateScrollCoordinationForLayer where we can have multiple scrolling tree nodes for the same layer. In this case the PositionedNode doesn't do any positioning, the sticky/fixed child node does. However it is still needed so that the scroll updates propagate correctly and that we can establish scrolling relations in UI process.

This is obviously not a good way to do things. I think we should combine Fixed/Sticky/Absolute into a single class and never have more than one node per layer.

[Antti Koivisto]

Comment on attachment 373491 [details]
patch

Lets do this later.

[Antti Koivisto]

Comment on attachment 373491 [details]
patch

How about now?

[Simon Fraser (smfr)]

Comment on attachment 373491 [details]
patch

My previous comments are still relevant.

[Antti Koivisto]

> My previous comments are still relevant.

I thought I answered it. Which part is still unclear?