pointInPath passed becuase of the pointInPath-expected.txt (LayoutTests/platform/qt/fast/canvas/): FAIL ctx.isPointInPath(10, 0) should be true. Was false. FAIL ctx.isPointInPath(29, 0) should be true. Was false.
HTML5 spec: Points on the path itself are considered to be inside the path. Qt's API (QPainterPah::contains) doesn't count the point at the path.
Created attachment 53169 [details] pointInPath QPainterPath::Contains doesn't count the point on the bound, but isPointInPath should count the point on the bound (based on the HTML5 spec)
(In reply to comment #2) > Created an attachment (id=53169) [details] > pointInPath > > QPainterPath::Contains doesn't count the point on the bound, but isPointInPath > should count the point on the bound (based on the HTML5 spec) I would suggest to put the code in a separate function like isPointOnPathBorder. But you can make the change after any reviewers commented on the patch.
Comment on attachment 53169 [details] pointInPath 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.
Actually, I am not sure if the algorithm is right. It looks to me when p == p1 (but != p2), the function returns false.
a couple of more issues after I looked into the details: 1. the algorithm didn't seem to check the last edge, i.e., from point n-1 to 0. 2. i think the algorithm can be simplified to ((x0<=x<=x1)||(x0>=x>=x1))&&((y1-y0)*(x-x0)==(y-y0)(x1-x0)). 3. i am also concerned about precisions introduced by floating number. i.e., the above equation might need to be adjusted a bit.
Created attachment 54187 [details] patch2 Based on Laszlo and Chang's comments, I did some change: 1> Take the code for checking point on the border to a function 2> Used a simplified formula, but I did a little bit change. Because we do need to check if the point.y is between p1.y and p2.y. For example: if p1.x==p2.x==p.x, but p1.y!=p2.y, any p.y will still make (y2-y1)*(x-x1)==(y-y1)*(x2-x1) true. 3> We don't need to care point n-1 to 0, because path will force n-1 is same with 0 to close the path. 4> Remove pointInPath-expected.txt from qt as Laszlo said. 5> Put some comments about algorithm.
Created attachment 54191 [details] patch3 1. Add some new comments 2. Remove png and checknum files
Comment on attachment 54191 [details] patch3 All review comments addresses, it looks good to me. r+. Thanks.
Comment on attachment 54191 [details] patch3 Clearing flags on attachment: 54191 Committed r58207: <http://trac.webkit.org/changeset/58207>
All reviewed patches have been landed. Closing bug.
Revision r58207 cherry-picked into qtwebkit-2.0 with commit 0d9e7dc77517ffdc19506525a0eea9e01d16d7fa