[webkit-reviews] review denied: [Bug 189624] [IntersectionObserver] Handle zero-area intersections : [Attachment 349778] 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 189624: [IntersectionObserver] Handle zero-area intersections
https://bugs.webkit.org/show_bug.cgi?id=189624

Attachment 349778: Patch

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




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

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

> Source/WebCore/dom/Document.cpp:7500
> +static bool computeIntersectionRects(FrameView& frameView,
IntersectionObserver& observer, Element& target, FloatRect& absTargetRect,
FloatRect& absIntersectionRect, FloatRect& absRootBounds, bool& isIntersecting)

Please add a comment saying what the return value means. Or maybe you can use
an optional<> return value?

> Source/WebCore/dom/Document.cpp:7541
> +    FloatRect rootLocalIntersectionRect =
targetRenderer->computeRectForRepaint(localTargetBounds, rootRenderer, {false
/* hasPositionFixedDescendant */, false /* dirtyRectIsFlipped */, true /*
useEdgeInclusiveIntersection */}, &isIntersecting);

I know we were using computeRectForRepaint before, but it's wrong to complexify
all the repaint logic with edge intersection for this purpose. The name
"computeRectForRepaint" makes it clear that the function is about repaint; we
should not be piggy-backing on it for intersection observer (we may want to
change how repaint works in future, so we should not make intersection observer
dependent on the repaint code).

You need to factor the "compute repaint rect" code into functions that you can
call for intersection observer, and can be used by our current repaint logic.

> Source/WebCore/dom/Document.cpp:7589
> +		   intersectionRatio = 1;

This is really confusing. Is this the "touching" rects case? Why would
intersectionRatio be 1 if they just touch?

> Source/WebCore/dom/Document.cpp:7606
> +		       clientIntersectionRect = isIntersecting ?
frameView->absoluteToClientRect(absIntersectionRect) : FloatRect();

": FloatRect()" can be ":  { }"

> Source/WebCore/platform/graphics/LayoutRect.cpp:77
> +    LayoutPoint newLocation(std::max(x(), other.x()), std::max(y(),
other.y()));
> +    LayoutPoint newMaxPoint(std::min(maxX(), other.maxX()), std::min(maxY(),
other.maxY()));

Shame that this code looks different than the FloatRect code.