[webkit-reviews] review denied: [Bug 78801] Select best target for tap gesture. : [Attachment 131334] Patch

[bugzilla-daemon at webkit.org]

Antonio Gomes <tonikitoo at webkit.org> has denied Allan Sandfeld Jensen
<allan.jensen at nokia.com>'s request for review:
Bug 78801: Select best target for tap gesture.
https://bugs.webkit.org/show_bug.cgi?id=78801

Attachment 131334: Patch
https://bugs.webkit.org/attachment.cgi?id=131334&action=review

------- Additional Comments from Antonio Gomes <tonikitoo at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=131334&action=review


It looks generally good to me. A few comments and we are good to go. I would
like see a final version with better name, then the r-. Logic seems fine.

Mainly, the term 'HitTest' spread around in many methods does not read well to
me all the time. Same with "Quad".

> Source/WebCore/page/EventHandler.cpp:2449
> +    IntRect touchRect(touchCenter - touchRadius, IntSize(touchRadius.width()
* 2, touchRadius.height() * 2));
> +    targetNode = 0;
> +
> +    HitTestResult result = hitTestResultAtPoint(touchCenter,
/*allowShadowContent*/ false, /*ignoreClipping*/ false, DontHitTestScrollbars,
hitType, touchRadius);

Are sure you do not want to move this line down below to "HitTestResult result
= ..." line, and use HitTestResult::rectForPoint(const LayoutPoint& point)?

Also note a "1px" difference in your calculation that I explained in this
method body.

> Source/WebCore/page/TouchAdjustment.cpp:41
> +class QuadForHitTest {

rename it as per kenneth suggestion. Maybe something like TouchTargetGeometry
or something like this?

> Source/WebCore/page/TouchAdjustment.cpp:117
> +    const unsigned int length = intersectedNodes.length();

'unsigned' would be enough here.

> Source/WebCore/page/TouchAdjustment.cpp:173
> +float distanceSquaredToQuadCenterLine(const IntPoint& touchHotspot, const
IntRect&, const QuadForHitTest& quadForHitTest)

remove the unused parameter or name it and add PARAM_UNUSED(xxx) with a comment
about any follow up action.

> Source/WebCore/page/TouchAdjustment.cpp:186
> +bool findNodeWithLowestMetric(Node*& targetNode, IntPoint& targetPoint,
const IntPoint& touchHotspot, const IntRect& touchArea, QuadNodeList&
hitTestList, Metric metric)

Even with the comment I still found 'metrics' is not the best word here.  No
better suggestion though.

> Source/WebCore/testing/Internals.cpp:584
> +    document->frame()->eventHandler()->bestClickableNodeForTouchPoint(point,
radius, adjustedPoint, targetNode);
> +
> +    return WebKitPoint::create(adjustedPoint.x(), adjustedPoint.y());

I would remove the blank line here

> Source/WebCore/testing/Internals.cpp:602
> +    document->frame()->eventHandler()->bestClickableNodeForTouchPoint(point,
radius, adjustedPoint, targetNode);
> +
> +    return adoptRef(targetNode);

ditto

> LayoutTests/touchadjustment/nested-touch.html:37
> +    function findAbsolutePosition(node) {

I am sure there a helper for it. Could you be sure we are not duplicating.