[webkit-reviews] review denied: [Bug 189833] [IntersectionObserver] Factor out rect mapping and clipping logic from computeRectForRepaint : [Attachment 351409] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Ali Juma
<ajuma at chromium.org>'s request for review:
Bug 189833: [IntersectionObserver] Factor out rect mapping and clipping logic
from computeRectForRepaint
https://bugs.webkit.org/show_bug.cgi?id=189833

Attachment 351409: Patch

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




--- Comment #19 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 351409
  --> https://bugs.webkit.org/attachment.cgi?id=351409
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:973
> +bool RenderBox::applyCachedClipAndScrollPosition(LayoutRect& rect, const
RenderLayerModelObject* container, bool applyCompositedClips, bool
applyCompositedContainerScrolls, bool useEdgeInclusiveIntersection) const

I think this needs a better name, or maybe shouldn't return a bool, since only
some callers care about intersection. Perhaps the intersection-related stuff
should be in a 'context' object pass as an argument?

> Source/WebCore/rendering/RenderBox.cpp:2138
> +LayoutRect RenderBox::computeAbsoluteVisibleRectUsingPaintOffsetCache(const
LayoutRect& rect) const

I think this is relative to the repaint container, not "absolute". The comments
say that layoutState->paintOffset() is offset from the layout root, but I think
it's probably the repaint root.

Maybe just computeVisibleRectUsingPaintOffset()

> Source/WebCore/rendering/RenderBox.cpp:2259
> +	   bool isEmpty =
!containerBox.applyCachedClipAndScrollPosition(adjustedRect, container,
context.m_applyCompositedClips, context.m_applyCompositedContainerScrolls,
context.m_useEdgeInclusiveIntersection);

Maybe just pass VisibleRectContext down

> Source/WebCore/rendering/RenderBox.h:574
> +    bool applyCachedClipAndScrollPosition(LayoutRect&, const
RenderLayerModelObject* container, bool applyCompositedClips, bool
applyCompositedContainerScrolls, bool useEdgeInclusiveIntersection) const;

A comment should say what the return value means.

> Source/WebCore/rendering/RenderInline.cpp:849
> +	   containingBlock->applyCachedClipAndScrollPosition(repaintRect,
repaintContainer, false /* applyCompositedClips */,
shouldApplyCompositedContainerScrollsForRepaint(), false /*
useEdgeInclusiveIntersection */);

Ick!

> Source/WebCore/rendering/RenderInline.cpp:874
> +LayoutRect
RenderInline::computeAbsoluteVisibleRectUsingPaintOffsetCache(const LayoutRect&
rect) const

Needs same rename.

> Source/WebCore/rendering/RenderObject.h:699
> +    // Given a rect in the object's coordinate space, compute the location
in container space where this rect is visible,
> +    // when clipping and scrolling as specified by the context. When using
edge-inclusive intersection, return std::nullopt
> +    // rather than an empty rect if the rect is completely clipped out in
container space.
> +    struct VisibleRectContext {
> +	   VisibleRectContext(bool hasPositionFixedDescendant = false, bool
dirtyRectIsFlipped = false, bool useEdgeInclusiveIntersection = false, bool
applyCompositedClips = true, bool applyCompositedContainerScrolls = true)
>	       : m_hasPositionFixedDescendant(hasPositionFixedDescendant)
>	       , m_dirtyRectIsFlipped(dirtyRectIsFlipped)
> +	       , m_useEdgeInclusiveIntersection(useEdgeInclusiveIntersection)
> +	       , m_applyCompositedClips(applyCompositedClips)
> +	       ,
m_applyCompositedContainerScrolls(applyCompositedContainerScrolls)
>	       {
>	       }
>	   bool m_hasPositionFixedDescendant;
>	   bool m_dirtyRectIsFlipped;
> +	   bool m_useEdgeInclusiveIntersection;
> +	   bool m_applyCompositedClips;
> +	   bool m_applyCompositedContainerScrolls;

I think this could just be an enum and an OptionSet<>