[webkit-reviews] review denied: [Bug 176914] Async iframes in the scrolling tree should be in the z-index order : [Attachment 356603] Patch 327061, rebased on top of bug 192398 (untested)

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied  review:
Bug 176914: Async iframes in the scrolling tree should be in the z-index order
https://bugs.webkit.org/show_bug.cgi?id=176914

Attachment 356603: Patch 327061, rebased on top of bug 192398 (untested)

https://bugs.webkit.org/attachment.cgi?id=356603&action=review




--- Comment #21 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 356603
  --> https://bugs.webkit.org/attachment.cgi?id=356603
Patch 327061, rebased on top of bug 192398 (untested)

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

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:167
> +    virtual ScrollingNodeID attachToStateTree(ScrollingNodeType,
ScrollingNodeID newNodeID, ScrollingNodeID /*parentID*/, size_t /*childIndex*/)
{ return newNodeID; }

Maybe pass a default for childIndex of "notFound" to allow the current code to
say "don't care" and get an append.

> Source/WebCore/page/scrolling/ScrollingStateNode.h:236
> +    void setChildIndex(size_t);
> +    size_t childIndex() const { return m_childIndex; }

I don't think we should ever set the index. I think if a node moves, we remove
and re-add.

> Source/WebCore/page/scrolling/ScrollingStateNode.h:240
> +    void insertChild(size_t position, Ref<ScrollingStateNode>&&);

I would put the size_t argument last.

> Source/WebCore/page/scrolling/ScrollingStateNode.h:251
> +    void prepareNewChild(ScrollingStateNode& childNode);

Not sure why you need a separate prepare function.

> Source/WebCore/page/scrolling/ScrollingStateNode.h:261
> +    size_t m_childIndex { 0 };

Storing childIndex here is wrong; child order is controlled by the order of the
parent's m_children.

> Source/WebCore/page/scrolling/ScrollingStateTree.cpp:103
> +	   if (nodeTypeAndParentAndChildIndexMatch(*node, nodeType, parentID,
childIndex))
>	       return newNodeID;

I did this a little differently in my patch, allowing a simple re-order here.

> Source/WebCore/page/scrolling/ScrollingStateTree.cpp:141
>	   if (!newNode) {
>	       auto stateNode = createNode(nodeType, newNodeID);
>	       newNode = stateNode.ptr();
> -	       parent->appendChild(WTFMove(stateNode));
> +	       parent->insertChild(childIndex, WTFMove(stateNode));
>	   }

Maybe deal with childIndex == notFound like I did.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3806
> +	   size_t childIndex = 0; // TODO

Yeah, I wasn't sure how to deal with this part. That's why I allowed a default
value of notFound.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3915
> +    size_t childIndex = 0; // TODO

This is why I allowed a default value of notFound.

>
Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:
86
> +    encoder << node.childIndex();

You shouldn't have to encode childIndex; the order of children in the child
array governs child order.