[webkit-reviews] review canceled: [Bug 46336] Make Document::nodesFromRect more flexible : [Attachment 68644] patch v1

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has canceled Antonio Gomes
<tonikitoo at webkit.org>'s request for review:
Bug 46336: Make Document::nodesFromRect more flexible
https://bugs.webkit.org/show_bug.cgi?id=46336

Attachment 68644: patch v1
https://bugs.webkit.org/attachment.cgi?id=68644&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
> > WebCore/ChangeLog:26
> > +	     In practive, the horizontal padding was being used to expand a
given center point in both
> 
> practice*

Fixed.
 
> > WebCore/ChangeLog:54
> > +	     (WebCore::EventHandler::hitTestResultAtPoint): For simplicity, I
did not change the signature
> > +							    but the way the
padding value passed in as parameter
> > +							    is being used to
construct a HitTestResult.
> > +	     * rendering/HitTestResult.cpp: Changed the rect-based bits from
using IntSize (padding) to separated paddings
> > +					    for each direction.
> 
> Why not write the comment on the line below... this looks really strange!

Fixed.

> > WebCore/dom/Document.cpp:1072
> > +	 else if
(!frameView->visibleContentRect().intersects(HitTestResult::rectFromPoint(IntPo
int(centerX, centerY), topPadding, rightPadding, bottomPadding, leftPadding)))
> 
> Is this the order we normally use? top, right, bottom, left? Just checking

To match mozilla's signature for the their similar method.

> > WebCore/rendering/HitTestResult.cpp:555
> > +IntRect HitTestResult::rectFromPoint(const IntPoint& point, unsigned
topPadding, unsigned rightPadding, unsigned bottomPadding, unsigned
leftPadding)
> 
> ForPoint?

ForPoint sounds good. Can I keep this as is for now, and fix in a follow up?
Reason: there are other two method overloaded in this class that would have to
change, and many call sites of these methods that would need change. It would
make the patch too bloat.

> 
> > WebCore/rendering/HitTestResult.cpp:560
> > +	 IntSize realPadding(leftPadding + rightPadding, topPadding +
bottomPadding);
> 
> Is there a fake padding :) ? Maybe actualPadding? or something more
descriptive.

'Actual' sounds good. Changed.

> > WebCore/rendering/HitTestResult.cpp:561
> > +	 realPadding += IntSize(1, 1);
> 
> Maybe add a comment why you are doing this

Comment added.
 
> > WebCore/rendering/HitTestResult.h:107
> > +	 int topPadding() const { return m_topPadding; }
> > +	 int rightPadding() const { return m_rightPadding; }
> > +	 int bottomPadding() const { return m_bottomPadding; }
> > +	 int leftPadding() const { return m_leftPadding; }
> 
> Is it useful to know the padding in the result? This is the result of a hit
test

See we using them in RenderLineBoxList.cpp