[webkit-reviews] review granted: [Bug 188809] [IntersectionObserver] Implement intersection logic for the explicit root case : [Attachment 347684] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Ali Juma
<ajuma at chromium.org>'s request for review:
Bug 188809: [IntersectionObserver] Implement intersection logic for the
explicit root case
https://bugs.webkit.org/show_bug.cgi?id=188809

Attachment 347684: Patch

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




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

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

> Source/WebCore/dom/Document.cpp:7443
> +static void computeIntersectionObservation(IntersectionObserver& observer,
Element* target, FloatRect& absTargetRect, FloatRect& absIntersectionRect,
FloatRect& absRootBounds)

I'm not a big fan of the name. Maybe just computeIntersections() or
computeIntersectionRects()? Make target an Element& since you don't null-check
it.

> Source/WebCore/dom/Document.cpp:7454
> +    RenderBlock* rootRenderer =
downcast<RenderBlock>(observer.root()->renderer());

Blank line above this please.

> Source/WebCore/dom/Document.cpp:7485
> +    absTargetRect =
targetRenderer->localToAbsoluteQuad(FloatRect(localTargetBounds)).boundingBox()
;
> +    absRootBounds =
rootRenderer->localToAbsoluteQuad(localRootBounds).boundingBox();

It's a shame that these two localToAbsoluteQuad() calls will both pay the cost
of traversing the common ancestors of 'target' and 'root'. Not worth optimizing
now, though.