[webkit-reviews] review granted: [Bug 89454] Store hit-test rect in HitTestPoint as Rect : [Attachment 149033] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Allan Sandfeld Jensen
<allan.jensen at nokia.com>'s request for review:
Bug 89454: Store hit-test rect in HitTestPoint as Rect
https://bugs.webkit.org/show_bug.cgi?id=89454

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=149033&action=review


These changes all look OK, but things seem a bit mixed up. And there are coding
style mistakes and typos that should be fixed before landing.

> Source/WebCore/rendering/HitTestResult.cpp:69
>      // If all padding values passed in are zero then it is not a rect based
hit test.
>      m_isRectBased = topPadding || rightPadding || bottomPadding ||
leftPadding;
> +    m_boundingRect = rectForPoint(centerPoint, topPadding, rightPadding,
bottomPadding, leftPadding);

Why are these not done as initializers rather than assignment? The code
generated will be slightly inferior this way and it seems unnecessary.

> Source/WebCore/rendering/HitTestResult.cpp:105
> +template<typename SomeRect>
> +bool hitTestPointIntersects(const HitTestPoint& hitTestPoint, const SomeRect
&rect)
> +{
> +    // FIXME: When the hit-test is not rect-based we should use
rect.contains(m_point).
> +    // That does change some corner case tests though.
> +
> +    // First check if rect even intersects our bounding rect.
> +    if (!rect.intersects(hitTestPoint.boundingBox()))
> +	   return false;
> +
> +    // FIXME: Implement quad-based interesection test to handle transformed
hittest points.
> +    return true;
> +
> +}

The name SomeRect here is not conventional terminology. Maybe RectType would be
a better name.

The & is positioned wrong and should be const SomeRect& rect.

The word intersection is spelled wrong as “interesection”. Since hittest is not
a word it should be “hit test”, and it should not be “hit-test” above, and it
should be “rect based”, not “rect-based”.

> Source/WebCore/rendering/HitTestResult.cpp:107
> +bool HitTestPoint::intersects(const LayoutRect &rect) const

Same formatting mistake.

> Source/WebCore/rendering/HitTestResult.cpp:109
> +    return hitTestPointIntersects<LayoutRect>(*this, rect);

There is no reason to specify the type here explicitly as <LayoutRect>. The
compiler selects the appropriate type for the template automatically.

> Source/WebCore/rendering/HitTestResult.cpp:112
> +bool HitTestPoint::intersects(const FloatRect &rect) const

Ditto.

> Source/WebCore/rendering/HitTestResult.cpp:114
> +    return hitTestPointIntersects<FloatRect>(*this, rect);

Ditto.

> Source/WebCore/rendering/HitTestResult.h:63
> -    void setPoint(const LayoutPoint& p) { m_point = p; }
> +    void setPoint(const LayoutPoint& p) {
m_boundingRect.move(roundedIntPoint(p) - roundedIntPoint(m_point)); m_point =
p; }

Is it correct to round twice here rather than doing the math and then rounding?
Is this covered by a test case? Perhaps this function is too long to have it
inside the class definition. We could put it somewhere below.

> Source/WebCore/rendering/HitTestResult.h:81
>      LayoutPoint m_point;
>  
> -    int m_topPadding;
> -    int m_rightPadding;
> -    int m_bottomPadding;
> -    int m_leftPadding;
> +    IntRect m_boundingRect;

I think it’s confusing to have a LayoutPoint named m_point and a IntRect named
m_boundingRect. Because of the strangely different naming and types it’s
unclear how these two relate to each other. Further, it’s unclear how
m_isRectBased works. Is one of these ignored completely in one of the two
cases?