[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 #17 from Grace Kloba <klobag at gmail.com>  2010-06-11 09:42:06 PST ---
(In reply to comment #14)
> (From update of attachment 57980 [details])
> 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.
> 

This is only done when we test two rectangle intersection. If updateHitTestResult is called as a result of nodeAtPoint() return true or similar, we won't do this.

Also we need to do rectangle containedBy test to decide whether it will continue the traverse or break. The rect is not available in all the cases.

> > +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);
> 

Sure, inline private method is fine.

> > +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.
> 

Done.

> 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?
> 

Merge is combination while "=" is replacement.

> > +void HitTestResult::addRawNode(Node* node)
> > +{
> > +    if (!node)
> > +        return;
> 
> bail out here to if not region test?
> 

Done.

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

Which IntRect do you refer to?

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