[webkit-reviews] review denied: [Bug 176914] Allow control over child order when adding nodes to the scrolling tree : [Attachment 356615] Simon's patch (bug 192395)

[bugzilla-daemon at webkit.org]

Frédéric Wang (:fredw) <fred.wang at free.fr> has denied  review:
Bug 176914: Allow control over child order when adding nodes to the scrolling
tree
https://bugs.webkit.org/show_bug.cgi?id=176914

Attachment 356615: Simon's patch (bug 192395)

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




--- Comment #24 from Frédéric Wang (:fredw) <fred.wang at free.fr> ---
Comment on attachment 356615
  --> https://bugs.webkit.org/attachment.cgi?id=356615
Simon's patch (bug 192395)

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

OK, I checked again the code this morning and your approach looks better. I've
done some minor changes to your initial patch (see comments inline).

> Source/WebCore/page/scrolling/ScrollingStateNode.h:-230
> -    ScrollingNodeID scrollingNodeID() const { return m_nodeID; }

I think I should have included this change in the refactoring of bug 192398 (I
thought it was intended to change the visibility of the member, but it's just
moving code around) :-/ I can keep this in the patch if you want.

> Source/WebCore/page/scrolling/ScrollingStateTree.cpp:148
> +		   // FIXME: use childIndex

I've addressed this FIXME, as I guess it was not intended to stay in the final
patch.

>
Source/WebKit/Shared/RemoteLayerTree/RemoteScrollingCoordinatorTransaction.cpp:
416
> +	   m_scrollingStateTree->attachNode(nodeType, nodeID, parentNodeID, i);

Using i is incorrect as it's the node index for some ordering of the tree, not
the child index. As I see, we only need to append the child.