[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 #25 from Grace Kloba <klobag at gmail.com>  2010-06-14 08:41:50 PST ---
(In reply to comment #24)
> (From update of attachment 58617 [details])
> Good progresses! Please see some more questions and comments below.
> 
> > +bool HitTestResult::addRawNodeForRegionTest(Node* node, int x, int y, const IntRect& rect)
> > +{
> > +    if (!isRegionTest() || !node)
> > +        return false;
> > +
> > +    m_rawNodeList.add(node);
> > +
> > +    return !rect.contains(pointRect(x, y));
> > +}
> 
> I'd add a comment here explaning why it is important to return true only if rect encloses pointRect(x,y): because call side can stop hittest in such cases.
> 

I was thinking of adding a comment in the header file. But I noticed that most of the WebKit methods do not have comments. Not sure whether it is style or not. Anyway, I will add one in the header file for this. 

> > +    IntSize pointPadding() const { return m_pointPadding; }
> 
> maybe just padding()?
> 

I assume you meant both variable and method name. Sure I can change it if it makes more sense.

> > +    IntRect pointRect(int x, int y) const;
> > +    IntRect pointRect(const IntPoint&) const;
> 
> maybe rename these to hit{Area,Region}FromPoint ? pointRect is kinda meaningless.
> 

Done.

> > +inline IntRect HitTestResult::pointRect(int x, int y) const
> > +{
> > +    return IntRect(x - m_pointPadding.width(), y - m_pointPadding.height(), 2 * m_pointPadding.width() + 1, 2 * m_pointPadding.height() + 1);
> > +}
> 
> hum, I missed the point on why +1 here. Could you explain? If that for cases when padding is IntSize(0,0)
> 

As IntRect is left inclusive and right exclusive, seeing IntRect::contains(x, y), I think we should do (2 * padding + 1).

> if that is the case, just set it to IntSize(1,1) in the point-only constructor.
> 
> HitTestResult::HitTestResult(const IntPoint& point)
>     : m_point(point)
>     , m_padding(1, 1)
>     , m_isOverWidget(false)
> {
> }
> 
> 
> I will work on tests on top of this one for now.

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