[webkit-reviews] review denied: [Bug 189833] [IntersectionObserver] Factor out rect mapping and clipping logic from computeRectForRepaint : [Attachment 351409] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 11 16:19:19 PDT 2018
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<>
More information about the webkit-reviews
mailing list