[webkit-reviews] review requested: [Bug 44089] [Qt] Making effective use of Document::nodesFromRect : [Attachment 68010] patch v4 - addressed kenneth's comments (it will fail until bug 44088 is in)

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has asked	for review:
Bug 44089: [Qt] Making effective use of Document::nodesFromRect
https://bugs.webkit.org/show_bug.cgi?id=44089

Attachment 68010: patch v4 - addressed kenneth's comments (it will fail until
bug 44088 is in)
https://bugs.webkit.org/attachment.cgi?id=68010&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
Comments addressed.

> > WebKit/qt/Api/qwebpage.cpp:1584
> > +IntPoint QWebPagePrivate::TouchAdjuster::findCandidatePointForTouch(const
IntPoint& touchPoint, Document* document) const
> ForTouchPoint? 

Hum ... Not sure I left as is as a personal preference, but I can change that
if you really prefer.

> > WebKit/qt/Api/qwebpage.cpp:1614
> > +	     if (area(boundingRect) > area(biggerIntersectionRect)) {
> I would remove the area methods and just do "int area = width * height;"
above. It is a bit overkill and makes the code less obvious

Done.

> > WebKit/qt/Api/qwebpage.cpp:1616
> > +		 biggerIntersectionRect = boundingRect;
> largestIntersectionRect ? It is bigger than what?
> 
> > WebKit/qt/Api/qwebpage.cpp:1629
> > +	     // Adjust client coordinates' origin to be top left of iframe
viewport.
> How do you know it is an iframe here?

I meant inner frame, so either a frame or a iframe. nice catch.

> > WebKit/qt/Api/qwebpage.cpp:1632
> > +	     IntSize offset =  IntSize(rect->left(), rect->top());
> offset_s_? how can an offset be a size?
> 
> > WebKit/qt/Api/qwebpage.cpp:1635
> > +	     HTMLFrameOwnerElement* owner =
static_cast<HTMLFrameOwnerElement*>(clickedElement);
> This will always work?

Yes, because we test isFrameOwner() before.

> 
> > WebKit/qt/Api/qwebpage.cpp:1637
> > +	     return findCandidatePointForTouch(newTouchPoint, subDocument);
> childDocument?

Renamed.

> > WebKit/qt/Api/qwebpage.cpp:1644
> > +bool QWebPagePrivate::TouchAdjuster::isElementClickable(Element* element,
RefPtr<NodeList> list) const
> isClickableElement?
> 
> > WebKit/qt/Api/qwebpage.cpp:1653
> > +	     for (int i = 0; i < count && parent; i++) {
> unsigned?

Done.