[Webkit-unassigned] [Bug 235420] Position:fixed layers shouldn't allocate a backing buffer if all children are offscreen

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=235420

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |darin at apple.com
 Attachment #449774|review?                     |review+
              Flags|                            |

--- 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);

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220124/cfbab55c/attachment.htm>