[webkit-reviews] review granted: [Bug 235420] Position:fixed layers shouldn't allocate a backing buffer if all children are offscreen : [Attachment 449774] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Matt Woodrow <m_woodrow at apple.com>'s
request for review:
Bug 235420: Position:fixed layers shouldn't allocate a backing buffer if all
children are offscreen
https://bugs.webkit.org/show_bug.cgi?id=235420

Attachment 449774: Patch

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




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 449774
  --> https://bugs.webkit.org/attachment.cgi?id=449774
Patch

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

Says Simon already reviewed this. I do have a couple comments.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2795
> +    const RenderLayer* layer = &child;

This can and should be written as a for loop:

    for (auto* layer = &child; layer != &ancestor; layer = layer->parent()) {
	If (!layer->canUseOffsetFromAncestor())
	    return std::nullopt;
    }

Writing it this way also makes it even clear erhow strong the dependency is on
the fact that ancestor is actually an ancestor of child. Otherwise we’ll crash
with a null pointer dereference.

It also is a little confusing. The function name is “canUseOffsetFromAncestor”,
but for some reason it has to be called along the entire ancestor chain. It’s
as if the function only really answers “can use offset from parent”.

Finally, I think this could use a comment explaining why this check is so
important. If any ancestors use transforms, then the clipping computation below
could be inaccurate, basically. That’s the kind of “why” thing that could go in
a comment. The function name is good, but not enough alone to explain that.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2802
> +    auto delta = child.convertToLayerCoords(&ancestor, { },
RenderLayer::AdjustForColumns);

To me this seems like “offset” more than “delta”.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2805
> +    return overlap.intersects(ancestorCompositedBounds);

I would have thought we could write:

    return (child.overlapBounds()+
offset).intersects(ancestorCompositedBounds);

But perhaps we don’t have that operator+ defined.

> Source/WebCore/rendering/RenderLayerBacking.cpp:2815
> +	       std::optional<bool> mayIntersect = intersectsWithAncestor(layer,
m_owningLayer, compositedBounds());

I would use value_or(true) here rather than having a local variable of type
std::optional<bool>. The optional<bool> answers the question “does intersect”.
Using value_or(true) turns into a boolean that answers the question “may
intersect”. So the local variable name is better if you use value_or(true).

    // If the function returns nullopt because it can’t determine if this
intersects with the ancestor, then it may intersect, which is why we do
value_or(true).
    bool mayIntersect = intersectsWithAncestor(layer, m_owningLayer,
compositedBounds()).value_or(true);