Bug 42188

Summary: Failing 2d.path.stroke.prune.arc philip canvas test
Product: WebKit Reporter: Matthew Delaney <mdelaney7>
Component: Layout and RenderingAssignee: Matthew Delaney <mdelaney7>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, commit-queue, dglazkov, gustavo, kling, sam, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 42190    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
oliver: review+
Previous + Cairo fix none

Matthew Delaney
Reported 2010-07-13 13:25:12 PDT
Safari is failing 2d.path.stroke.prune.arc
Attachments
Patch (5.26 KB, patch)
2010-07-14 14:27 PDT, Matthew Delaney
no flags
Patch (9.77 KB, patch)
2010-07-14 17:42 PDT, Matthew Delaney
no flags
Patch (9.77 KB, patch)
2010-07-14 18:00 PDT, Matthew Delaney
no flags
Patch (10.19 KB, patch)
2010-07-16 13:01 PDT, Matthew Delaney
oliver: review+
Previous + Cairo fix (10.19 KB, patch)
2010-07-16 14:21 PDT, Matthew Delaney
no flags
Matthew Delaney
Comment 1 2010-07-14 14:27:06 PDT
Darin Adler
Comment 2 2010-07-14 15:25:59 PDT
Comment on attachment 61566 [details] Patch > + if ((p1 == m_path.currentPoint()) || (p1 == p2) || !r) No need for the extra parentheses here since == binds more tightly than || and it's customary to not include these. > Index: WebCore/platform/graphics/cg/PathCG.cpp > =================================================================== > --- WebCore/platform/graphics/cg/PathCG.cpp (revision 63275) > +++ WebCore/platform/graphics/cg/PathCG.cpp (working copy) > @@ -249,6 +249,11 @@ bool Path::hasCurrentPoint() const > { > return !isEmpty(); > } > + > +FloatPoint Path::currentPoint() const > +{ > + return CGPathGetCurrentPoint(m_path); > +} I believe adding this for CG only will break the build for all platforms that don’t use CG. That’s probably why the EWS failed on Qt. So you’ll need to add this for other platforms as well. review- because we need this to at least compile on the non-CG platforms
WebKit Review Bot
Comment 3 2010-07-14 15:50:30 PDT
Matthew Delaney
Comment 4 2010-07-14 17:42:29 PDT
Darin Adler
Comment 5 2010-07-14 17:44:14 PDT
Comment on attachment 61589 [details] Patch > +FloatPoint Path::currentPoint() const > +{ > + // FIXME: implement safe way to return current point of subpath. > + return 0; > +} I don’t think this will compile. There’s no way to convert the number 0 into a FloatPoint. You probably need to return something like FloatPoint(NAN, NAN). You should test by actually compiling the code, perhaps in PathCG.cpp. Not sure what "safe" means in the comment.
Matthew Delaney
Comment 6 2010-07-14 18:00:06 PDT
Darin Adler
Comment 7 2010-07-14 18:10:58 PDT
Comment on attachment 61592 [details] Patch Come to think of it, I’m not sure NAN works on all the platforms. We might need to use "numeric_limits<float>::quiet_NaN()" instead. The reason I suggested NAN is that is safe. The optimization won't do any harm if you return a point that can never be equal.
Andreas Kling
Comment 8 2010-07-14 18:59:55 PDT
Good stuff :-) The Qt implementation of Path::currentPoint() would be: FloatPoint Path::currentPoint() const { return m_path.currentPosition(); }
Sam Weinig
Comment 9 2010-07-15 22:36:57 PDT
The cairo equivelent is probably. FloatPoint Path::currentPoint() const { double x; double y; cairo_get_current_point(platformPath()->m_cr, x, y) return FloatPoint(x, y); }
Matthew Delaney
Comment 10 2010-07-16 13:01:26 PDT
Oliver Hunt
Comment 11 2010-07-16 13:28:00 PDT
Comment on attachment 61839 [details] Patch r=me
WebKit Review Bot
Comment 12 2010-07-16 14:08:12 PDT
Matthew Delaney
Comment 13 2010-07-16 14:21:55 PDT
Created attachment 61847 [details] Previous + Cairo fix Ooops on the Cairo impl slip. This should be good. I checked their code use below to double/triple check.
WebKit Commit Bot
Comment 14 2010-07-16 17:08:49 PDT
Comment on attachment 61847 [details] Previous + Cairo fix Clearing flags on attachment: 61847 Committed r63599: <http://trac.webkit.org/changeset/63599>
WebKit Commit Bot
Comment 15 2010-07-16 17:08:55 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.