Bug 42188 – Failing 2d.path.stroke.prune.arc philip canvas test

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Matthew Delaney]

Safari is failing 2d.path.stroke.prune.arc

[Matthew Delaney]

Created attachment 61566 [details]
Patch

[Darin Adler]

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]

Attachment 61566 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3447295

[Matthew Delaney]

Created attachment 61589 [details]
Patch

[Darin Adler]

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]

Created attachment 61592 [details]
Patch

[Darin Adler]

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]

Good stuff :-)

The Qt implementation of Path::currentPoint() would be:

FloatPoint Path::currentPoint() const 
{
    return m_path.currentPosition();
}

[Sam Weinig]

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]

Created attachment 61839 [details]
Patch

[Oliver Hunt]

Comment on attachment 61839 [details]
Patch

r=me

[WebKit Review Bot]

Attachment 61839 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3396494

[Matthew Delaney]

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 on attachment 61847 [details]
Previous + Cairo fix

Clearing flags on attachment: 61847

Committed r63599: <http://trac.webkit.org/changeset/63599>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.