Summary:  LayoutTests/fast/canvas/pointInPath.html passed, actually it failed  

Product:  WebKit  Reporter:  qi <qi.2.zhang>  
Component:  New Bugs  Assignee:  qi <qi.2.zhang>  
Status:  CLOSED FIXED  
Severity:  Normal  CC:  commitqueue, cshu, hausmann, krit, laszlo.gombos  
Priority:  P3  Keywords:  Qt  
Version:  528+ (Nightly build)  
Hardware:  PC  
OS:  Linux  
Bug Depends on:  
Bug Blocks:  35784  
Attachments: 

Description
qi
20100408 08:43:54 PDT
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/pointInPathexpected.txt > =================================================================== >  LayoutTests/platform/qt/fast/canvas/pointInPathexpected.txt (revision 57339) > +++ LayoutTests/platform/qt/fast/canvas/pointInPathexpected.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/pointInPathexpected.txt becomes the same as LayoutTests/fast/canvas/pointInPathexpected.txt (as it should be). Can we just delete the qt specific expected results and go with the crossplatform 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 n1 to 0. 2. i think the algorithm can be simplified to ((x0<=x<=x1)(x0>=x>=x1))&&((y1y0)*(xx0)==(yy0)(x1x0)). 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 (y2y1)*(xx1)==(yy1)*(x2x1) true.
3> We don't need to care point n1 to 0, because path will force n1 is same with 0 to close the path.
4> Remove pointInPathexpected.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 cherrypicked into qtwebkit2.0 with commit 0d9e7dc77517ffdc19506525a0eea9e01d16d7fa 