[webkit-reviews] review requested: [Bug 40197] Enhance the hit testing to take a rectangle instead of a point : [Attachment 62175] patch v4 - feature + tests

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has asked	for review:
Bug 40197: Enhance the hit testing to take a rectangle instead of a point
https://bugs.webkit.org/show_bug.cgi?id=40197

Attachment 62175: patch v4 - feature + tests
https://bugs.webkit.org/attachment.cgi?id=62175&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
Adressing Kenneth's comments.

> WebCore/dom/Document.cpp:1055
>  +	  float zoomFactor = frameView->pageZoomFactor();
> What about scale? The QGraphicsWebView can have a scale set which will be
applied to the QPainter - will that work?

I do not know we have to scale the padding area based on the current zoom
factor. The centerPoint has to consider scale, not the padding (since our thumb
and index finger do not enlarge :)
 
> WebCore/dom/Document.cpp:1056
>  +	  IntPoint point = roundedIntPoint(FloatPoint(x * zoomFactor  +
view()->scrollX(), y * zoomFactor + view()->scrollY()));
> Is there any need to specify that it is an int point?

All hit testing system is based on int value, so we cast back it from
FroatPoint to IntPoint.

> WebCore/page/EventHandler.h:108
>  +	  HitTestResult hitTestResultAtPoint(const IntPoint&, bool
allowShadowContent, bool ignoreClipping = false, HitTestScrollbars scrollbars =
DontHitTestScrollbars, const IntSize& padding = IntSize(-1, -1));
> I believe that we have something like IntPoint::zero() or zeroPoint or so,
maybe we want a special constructor for this? Darin?

I tried to convince Darin Adler about the usage of a static IntSize::invalid {
return IntSize(-1, -1); } and IntSize::isValid() const { return m_width >= 0 &&
m_height >= 0; } without success. See bug 42653.

I left it as it was.
 
> WebCore/rendering/HitTestResult.h:59
>  +	  bool isRegionTest() const { return m_isRegionTest &&
hasValidPadding(); }
> You talk about rect based hit testing, and here you call it region? Is it
region or is it rect?

You are right. I change any reference to region to rectBased. For example:
isRegionTest became isRectBasedTest
 
> WebCore/rendering/HitTestResult.h:97
>  +	  inline bool hasValidPadding() const { return m_padding.width() >= 0
&& m_padding.height() >= 0; }
> wouldn't hasPadding be sufficient? Padding, region, rect... it is a bit
confusing

This method has gone.

> WebCore/rendering/RenderBlock.cpp:3847
>  +		      hitTestContents(request, result, x, y, finalX, finalY,
hitTestAction);
> no return needed here? 

No return needed here, since the caller site handles it.

Furthers changes:

1) Renamed HitTestResult::addRawNodeForRegionTest to
addNodeToRectBasedTestResult
2) Renamed HitTestResult::isRegionTest to isRectBasedTest
3) Renamed HitTestResult::rawNodeList to rectBasedTestResult
4) Renamed HitTestResult::regionFromPoint to rectFromPoint
5) Made addRawNodeForRegionTest no-op in case of non rect-based hit tests.