[webkit-reviews] review granted: [Bug 192529] Add nodes to the scrolling tree in z-index order. : [Attachment 360510] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 192529: Add nodes to the scrolling tree in z-index order.
https://bugs.webkit.org/show_bug.cgi?id=192529

Attachment 360510: Patch

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




--- Comment #9 from Dean Jackson <dino at apple.com> ---
Comment on attachment 360510
  --> https://bugs.webkit.org/attachment.cgi?id=360510
Patch

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

> Source/WebCore/ChangeLog:21
> +	   was hard because the backing was already disconnect from its owning
RenderLayer, so I added RenderLayerBacking::willBeDestroyed()

typo: disconnected

> Source/WebCore/rendering/RenderLayerCompositor.cpp:627
> +	   return { };

Is this the new nullopt?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3773
> +    ASSERT_IMPLIES(nodeType == ScrollingNodeType::MainFrame,
!treeState.parentNodeID.value());

oooh. I didn't know ASSERT_IMPLIES existed :|

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3807
> +	   // FIXME

oops!

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3837
> +    bool isViewportConstained =
roles.contains(ScrollCoordinationRole::ViewportConstrained);

I don't know what a con stain is.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3890
> +	   ASSERT(layer.renderer().isFixedPositioned());

huh. i assumed this wouldn't compile in release.

>
LayoutTests/scrollingcoordinator/scrolling-tree/scrolling-tree-is-z-order.html:
45
> +	       z-index: 3;

It might be worth explaining in a comment (assuming I'm correct) that this
z-index value causes the third Fixed Node in the results to be the one with the
top-left-most layout position.

i.e. the result shows that the scrolling layers are #second, #third, #first,
because z-index: 3, auto, 1.

Also, I guess you could make this value 2 :)