[webkit-reviews] review granted: [Bug 89448] Use HitTestPoint instead of LayoutPoint for nodeAtPoint : [Attachment 148293] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Allan Sandfeld Jensen
<allan.jensen at nokia.com>'s request for review:
Bug 89448: Use HitTestPoint instead of LayoutPoint for nodeAtPoint
https://bugs.webkit.org/show_bug.cgi?id=89448

Attachment 148293: Patch
https://bugs.webkit.org/attachment.cgi?id=148293&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=148293&action=review


Thank you again for hte long explanations both on the bugs, and over IRC.

I have real concern about the long-term readability of this code due to the
really port class, function and variable namings.  These new objects are more
than points, and naming them HitTestPoint is just confusing.

HitRegion would be one possible suggested replacement.

In eithe rcase, this patch is OK, but we really need to fix this code to read
more clearly before we're done here.

> Source/WebCore/rendering/InlineFlowBox.cpp:969
> -    if (!overflowRect.intersects(result.rectForPoint(pointInContainer)))
> +    if (!pointInContainer.intersects(overflowRect))

So we're going to remove rectForPoint in this new model?

> Source/WebCore/rendering/RenderLayer.cpp:3631
> -    if (bgRect.intersects(hitTestArea) && isSelfPaintingLayer()) {
> +    if (bgRect.intersects(hitTestPoint) && isSelfPaintingLayer()) {

Seems like a sad name change. :(

> Source/WebCore/rendering/RenderLayer.h:118
> +    bool intersects(const HitTestPoint&);

Unclear to me if this what it's going to look like in the end.	It seems that
clipRect and HitTestPoint both represent possibly non-rectilinear shapes.

> Source/WebCore/rendering/RenderLineBoxList.cpp:287
> +    LayoutPoint point = pointInContainer.point();

This and many similar renames could hav been avoided by changing the argument
to something else and using a pointInContainer local which was actually a
LayoutPOint.