[webkit-reviews] review denied: [Bug 37276] LayoutTests/fast/canvas/pointInPath.html passed, actually it failed : [Attachment 53169] pointInPath

[bugzilla-daemon at webkit.org]

Laszlo Gombos <laszlo.1.gombos at nokia.com> has denied qi
<qi.2.zhang at nokia.com>'s request for review:
Bug 37276: LayoutTests/fast/canvas/pointInPath.html passed, actually it failed
https://bugs.webkit.org/show_bug.cgi?id=37276

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

------- Additional Comments from Laszlo Gombos <laszlo.1.gombos at nokia.com>
Overall the approach and the algorithm looks good to me.

I think this code could use some comments and/or refactoring; specific
suggestions below..

> +    if (!contains) {
> +	   // check whether the point is on the bound

As Chang suggested, I think it make sense to break this out into its own
function.

> +    
> +	   QPolygonF polygon = m_path.toFillPolygon();
> +	   QPointF p1 = polygon.at(0);
> +	   QPointF p2;
> +	   QPointF p = point;

Variable p seems to be used only as an alias to point; this might save some
typing, but it makes a bit harder to read for me. If the function is factored
out p would become the argument, in which case the code is OK.

> +
> +	   for (int i = 1; i < polygon.size(); ++i) {
> +	       p2 = polygon.at(i);

Comment explaining why special casing is needed if the x coordinates are the
same would be helpful (e.g. slope can not be calculated if the x coordinates
are the same).

> +	       if (p.x() == p1.x() || p.x() == p2.x()) {
> +		   if (p.x() == p1.x() && p.x() == p2.x()
> +		       && p.y() <= qMax(p1.y(), p2.y()) && p.y() >=
qMin(p1.y(), p2.y())) {
> +		       contains = true;
> +		       break;
> +		   

A simple comment like "If the slopes between p and p1 is equal to that between
p and p2 then the three points lie on the same line" would help.

> +	       } else if ((p.y()-p1.y()) / (p.x()-p1.x()) == (p2.y()-p.y()) /
(p2.x()-p.x())) {
> +		   if (p.x() <= qMax(p1.x(), p2.x()) && p.x() >= qMin(p1.x(),
p2.x())
> +		       && p.y() <= qMax(p1.y(), p2.y()) && p.y() >=
qMin(p1.y(), p2.y())) {
> +		       contains= true;
> +		       break;
> +		   }
> +	       }
> +	       p1 = p2;
> +	   }
> +    }
>  
>      const_cast<QPainterPath*>(&m_path)->setFillRule(savedRule);
>      return contains;
> Index: LayoutTests/platform/qt/fast/canvas/pointInPath-expected.txt
> ===================================================================
> --- LayoutTests/platform/qt/fast/canvas/pointInPath-expected.txt     
(revision 57339)
> +++ LayoutTests/platform/qt/fast/canvas/pointInPath-expected.txt     
(working copy)
> @@ -37,8 +37,8 @@ Translate context (10,20)
>  Transform context (1, 0, 0, -1, 0, 0)
>  Rectangle at (0,0) 20x20
>  PASS ctx.isPointInPath(5, 5) is false
> -FAIL ctx.isPointInPath(10, 0) should be true. Was false.
> -FAIL ctx.isPointInPath(29, 0) should be true. Was false.
> +PASS ctx.isPointInPath(10, 0) is true
> +PASS ctx.isPointInPath(29, 0) is true
>  PASS ctx.isPointInPath(10, 19) is true
>  PASS ctx.isPointInPath(21, 10) is true
>  PASS ctx.isPointInPath(29, 19) is true

With this change LayoutTests/platform/qt/fast/canvas/pointInPath-expected.txt
becomes the same as LayoutTests/fast/canvas/pointInPath-expected.txt (as it
should be). Can we just delete the qt specific expected results and go with the
cross-platform one ? 

This does not quite deserve the r-, but I'd like to see if we can address some
of this feedback, so r- for now.