[Webkit-unassigned] [Bug 40197] Enhance the hit testing to take a rectangle instead of a point

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40197





--- Comment #14 from Antonio Gomes (tonikitoo) <tonikitoo at webkit.org>  2010-06-11 07:24:22 PST ---
(From update of attachment 57980)
patch looks promissing, as I mentioned earlier. Some comments and questions below.

> +        if (result.isRegionTest()) {
> +            ASSERT(renderer()->node() || renderer()->isAnonymous());
> +            result.addRawNode(renderer()->node());
> +            if (!result.containedBy(x, y, boundsRect))
> +                return false;
> +        }

You seem to been doing this in 6 or 7 other places. Code looks exactly the same. Why not put this code in updateHitTestResult? Even less code modified in the end.

> +bool HitTestResult::intersects(int x, int y, const IntRect& other) const
> +{
> +    IntRect pointRect(x - m_pointPadding.width(), y - m_pointPadding.height(), 2 * m_pointPadding.width() + 1, 2 * m_pointPadding.height() + 1);
> +    return other.intersects(pointRect);
> +}
> +
> +bool HitTestResult::containedBy(int x, int y, const IntRect& other) const
> +{
> +    IntRect pointRect(x - m_pointPadding.width(), y - m_pointPadding.height(), 2 * m_pointPadding.width() + 1, 2 * m_pointPadding.height() + 1);
> +    return other.contains(pointRect);
> +}
> +

I would make an inline or a static method for this line:
IntRect pointRect(x - m_pointPadding.width(), y - m_pointPadding.height(), 2 * m_pointPadding.width() + 1, 2 * m_pointPadding.height() + 1);

> +void HitTestResult::merge(const HitTestResult& other)
> +{
> +    if (!m_innerNode && other.innerNode()) {
> +        m_innerNode = other.innerNode();
> +        m_innerNonSharedNode = other.innerNonSharedNode();
> +        m_localPoint = other.localPoint();
> +        m_innerURLElement = other.URLElement();
> +        m_scrollbar = other.scrollbar();
> +         m_isOverWidget = other.isOverWidget();
> +    }

as far as I could see, merge is only used on Region test, right? If so, i would bail out earlier here if not region test or ASSERT for safety.

Also, HitTestResult overwrites the "=" operator. Why not just do use it? Is it because you do not want other's m_rawNodeList to be assigned to it?

> +    const Vector<RefPtr<Node> >& list = other.rawNodeList();
> +    Vector<RefPtr<Node> >::const_iterator last = list.end();
> +    for (Vector<RefPtr<Node> >::const_iterator it = list.begin(); it != last; ++it)
> +        addRawNode(it->get());
> +}
> +
> +void HitTestResult::addRawNode(Node* node)
> +{
> +    if (!node)
> +        return;

bail out here to if not region test?

Also, current logic only needs height and width of the padding rect . Why not use a IntSize instead of a IntRect?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.